{"action":"created","issue":{"url":"https://api.github.com/repos/google/blockly/issues/557","repository_url":"https://api.github.com/repos/google/blockly","labels_url":"https://api.github.com/repos/google/blockly/issues/557/labels{/name}","comments_url":"https://api.github.com/repos/google/blockly/issues/557/comments","events_url":"https://api.github.com/repos/google/blockly/issues/557/events","html_url":"https://github.com/google/blockly/pull/557","id":171545290,"number":557,"title":"Integrate variable management into develop.","user":{"login":"rachel-fenichel","id":13686399,"avatar_url":"https://avatars.githubusercontent.com/u/13686399?v=3","gravatar_id":"","url":"https://api.github.com/users/rachel-fenichel","html_url":"https://github.com/rachel-fenichel","followers_url":"https://api.github.com/users/rachel-fenichel/followers","following_url":"https://api.github.com/users/rachel-fenichel/following{/other_user}","gists_url":"https://api.github.com/users/rachel-fenichel/gists{/gist_id}","starred_url":"https://api.github.com/users/rachel-fenichel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rachel-fenichel/subscriptions","organizations_url":"https://api.github.com/users/rachel-fenichel/orgs","repos_url":"https://api.github.com/users/rachel-fenichel/repos","events_url":"https://api.github.com/users/rachel-fenichel/events{/privacy}","received_events_url":"https://api.github.com/users/rachel-fenichel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"picklesrus","id":15675877,"avatar_url":"https://avatars.githubusercontent.com/u/15675877?v=3","gravatar_id":"","url":"https://api.github.com/users/picklesrus","html_url":"https://github.com/picklesrus","followers_url":"https://api.github.com/users/picklesrus/followers","following_url":"https://api.github.com/users/picklesrus/following{/other_user}","gists_url":"https://api.github.com/users/picklesrus/gists{/gist_id}","starred_url":"https://api.github.com/users/picklesrus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/picklesrus/subscriptions","organizations_url":"https://api.github.com/users/picklesrus/orgs","repos_url":"https://api.github.com/users/picklesrus/repos","events_url":"https://api.github.com/users/picklesrus/events{/privacy}","received_events_url":"https://api.github.com/users/picklesrus/received_events","type":"User","site_admin":false},"assignees":[{"login":"picklesrus","id":15675877,"avatar_url":"https://avatars.githubusercontent.com/u/15675877?v=3","gravatar_id":"","url":"https://api.github.com/users/picklesrus","html_url":"https://github.com/picklesrus","followers_url":"https://api.github.com/users/picklesrus/followers","following_url":"https://api.github.com/users/picklesrus/following{/other_user}","gists_url":"https://api.github.com/users/picklesrus/gists{/gist_id}","starred_url":"https://api.github.com/users/picklesrus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/picklesrus/subscriptions","organizations_url":"https://api.github.com/users/picklesrus/orgs","repos_url":"https://api.github.com/users/picklesrus/repos","events_url":"https://api.github.com/users/picklesrus/events{/privacy}","received_events_url":"https://api.github.com/users/picklesrus/received_events","type":"User","site_admin":false}],"milestone":null,"comments":1,"created_at":"2016-08-16T23:57:25Z","updated_at":"2016-08-17T18:33:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/google/blockly/pulls/557","html_url":"https://github.com/google/blockly/pull/557","diff_url":"https://github.com/google/blockly/pull/557.diff","patch_url":"https://github.com/google/blockly/pull/557.patch"},"body":"Followup work includes updating all documentation when this hits master."},"comment":{"url":"https://api.github.com/repos/google/blockly/issues/comments/240505084","html_url":"https://github.com/google/blockly/pull/557#issuecomment-240505084","issue_url":"https://api.github.com/repos/google/blockly/issues/557","id":240505084,"user":{"login":"picklesrus","id":15675877,"avatar_url":"https://avatars.githubusercontent.com/u/15675877?v=3","gravatar_id":"","url":"https://api.github.com/users/picklesrus","html_url":"https://github.com/picklesrus","followers_url":"https://api.github.com/users/picklesrus/followers","following_url":"https://api.github.com/users/picklesrus/following{/other_user}","gists_url":"https://api.github.com/users/picklesrus/gists{/gist_id}","starred_url":"https://api.github.com/users/picklesrus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/picklesrus/subscriptions","organizations_url":"https://api.github.com/users/picklesrus/orgs","repos_url":"https://api.github.com/users/picklesrus/repos","events_url":"https://api.github.com/users/picklesrus/events{/privacy}","received_events_url":"https://api.github.com/users/picklesrus/received_events","type":"User","site_admin":false},"created_at":"2016-08-17T18:33:57Z","updated_at":"2016-08-17T18:33:57Z","body":"\n\n\n\nReview status: 0 of 98 files reviewed at latest revision, 13 unresolved discussions.\n\n---\n\n*[core/block.js, line 199 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOSpNe4zf7Nv63KvpI:-KPOSpNfY6qY8VTLL3Fw:b-pbwet1) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/block.js#L199)):*\n> ```JavaScript\n>   if (!this.workspace) {\n>     // Already deleted.\n>     return;\n> ```\n\nI'm curious why we try and delete it again? \n\n---\n\n*[core/flyout.js, line 721 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPObI9K-9M5lHFRRIDZ:-KPObI9LVWjUqN9kKSKH:b-4pixdz) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout.js#L721)):*\n> ```JavaScript\n>  */\n> Blockly.Flyout.prototype.layout_ = function(contents, gaps) {\n>   this.workspace_.scale = this.targetWorkspace_.scale;\n> ```\n\nwhy set the scale here?   Change seems unrelated to variables, but then maybe this is to get the button to scale properly?  I vaguely remember having a conversation about this exact line of code just in a different place before :)\n\n---\n\n*[core/flyout.js, line 746 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOc2RRAbzpXaNBKkPG:-KPOc2RSKySspyXsSugE:bw1f6le) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout.js#L746)):*\n> ```JavaScript\n>         cursorX += tab;\n>       }\n>       block.moveBy((this.horizontalLayout_ && this.RTL) ? -cursorX : cursorX,\n> ```\n\nThis change seems unrelated too.\n\n---\n\n*[core/flyout.js, line 801 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOc_XdPW97zSF4RMNf:-KPOc_XdPW97zSF4RMNg:b-g89r03) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout.js#L801)):*\n> ```JavaScript\n> \n>   for (var i = 0, button; button = this.buttons_[i]; i++) {\n>     button.dispose();\n> ```\n\nwhat type is this list of buttons?  I wanted to take a quick look at what the dispose to make sure the event handler wasn't left around, but I don't know where to look. Thanks javascript.\n\n---\n\n*[core/flyout_button.js, line 103 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOeypPBXR2DWDmlNAX:-KPOeypPBXR2DWDmlNAY:b-7yykgs) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout_button.js#L103)):*\n> ```JavaScript\n>   svgText.textContent = this.text_;\n> \n>   this.width = svgText.getComputedTextLength() +\n> ```\n\nthis only happens once on flyout creation, right? or is it every time the variable flyout category opens?  getComputedTextLength is now on my evil list...\n\n---\n\n*[core/flyout_button.js, line 107 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOfMUdKJsfOxyFuIlw:-KPOfMUe7HjjicS1D82A:b-ql9udw) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout_button.js#L107)):*\n> ```JavaScript\n>   this.height = 20; // Can't compute it :(\n> \n>   rect.setAttribute('width', this.width);\n> ```\n\nactually, if this were a div instead, would you still need to pre-calculate this width?  Maybe yes, because you need to calculate the width of the flyout?\n\n---\n\n*[core/flyout_button.js, line 152 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOfzHUlv0u6Z9_4yGD:-KPOfzHV2mHgW6NkA9Ve:b-ykokj8) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/flyout_button.js#L152)):*\n> ```JavaScript\n>     this.svgGroup_ = null;\n>   }\n>   this.workspace_ = null;\n> ```\n\nshould targetWorkspace, text, position, and any other members be set to null?\n\n---\n\n*[core/variables.js, line 47 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOgeCZyEjtaNCWRWGc:-KPOgeCZyEjtaNCWRWGd:b-sowr0z) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/variables.js#L47)):*\n> ```JavaScript\n> Blockly.Variables.allUsedVariables = function(root) {\n>   var blocks;\n>   if (root instanceof Blockly.Block) {\n> ```\n\nhow come you changed this?\n\n---\n\n*[core/variables.js, line 134 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOiIIgPhH4_3ngoTar:-KPOiIIgPhH4_3ngoTas:b-brnopo) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/variables.js#L134)):*\n> ```JavaScript\n>       block.setAttribute('type', 'variables_set');\n>       if (Blockly.Blocks['math_change']) {\n>         block.setAttribute('gap', 8);\n> ```\n\nwhy is the gap different for math_change vs the rest? Just because it isn't a standard setter so you want the visual separation?\n\n---\n\n*[core/variables.js, line 175 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOjMYwW63b41gqpiWd:-KPOjMYwW63b41gqpiWe:b44izkt) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/variables.js#L175)):*\n> ```JavaScript\n>     }\n> \n>     for (var i = 0; i < variableList.length; i++) {\n> ```\n\nshould this start at 1? IIUC, you did everything for the variable at index 0 in the if above. \n\n---\n\n*[core/variables.js, line 309 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOjx43YePjgFCO9F2j:-KPOjx43YePjgFCO9F2k:b-c59hrr) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/variables.js#L309)):*\n> ```JavaScript\n>     workspace.variableList.splice(variableIndex, 1);\n>   }\n> ```\n\nextra line\n\n---\n\n*[core/variables.js, line 322 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOkHGgma6Pz47d8qQC:-KPOkHGgma6Pz47d8qQD:b-qa6ub7) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/variables.js#L322)):*\n> ```JavaScript\n> Blockly.Variables.createVariable = function(workspace) {\n>   var text = Blockly.Variables.promptName(Blockly.Msg.NEW_VARIABLE_TITLE, '');\n>   // Since variables are case-insensitive, ensure that if the new variable\n> ```\n\nShould this comment be here?\n\n---\n\n*[core/workspace.js, line 121 \\[r1\\]](https://reviewable.io:443/reviews/google/blockly/557#-KPOl2EwyufBn9V2mlIz:-KPOl2Ex4W4ENqkmX94D:bfrhnda) ([raw file](https://github.com/google/blockly/blob/eba075b5b6ba5f8b4d7cb3741773ce30532ec9ee/core/workspace.js#L121)):*\n> ```JavaScript\n> Blockly.Workspace.prototype.addTopBlock = function(block) {\n>   this.topBlocks_.push(block);\n>   if (this.isFlyout) {\n> ```\n\nIs this here to catch cases like undo/redo, load from xml, etc where a variable is introduced to the workspace not through the create new variable button?  Maybe throw a comment in.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/google/blockly/557)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"picklesrus"}}