google / blockly

The web-based visual programming editor.
https://developers.google.com/blockly/
Apache License 2.0
12.5k stars 3.71k forks source link

Unused variables are no longer saved in xml #1914

Closed NAllred91 closed 6 months ago

NAllred91 commented 6 years ago

Problem statement

When you add a variable to your workspace, and then retrieve the workspace xml to save it, only variables that are actively being used are saved in the xml.

Expected Behavior

The previous behavior was that all variables that existed in the workspace (used and unused) were included in the workspace xml. I would expect to be able to use the workspace xml to recreate the workspace exactly as it was before.

NAllred91 commented 6 years ago

I can understand that the generated code does not include the unused variables. But I think it is reasonable to expect that the XML can be used to recreate the workspace exactly as before?

I can think of some examples where this is useful.

1) I create something in blockly and prepopulate it with some variables with specific names. I then somehow distribute the xml in a class room setting and provide students with instructions telling them to use only the variables I've defined to solve a problem.

2) Its possible that a user's first step in solving a problem could be to define all the variables they expect to use. A user might spend time thinking about what variables they need, create a handful of variables, and then save. Now, when they come back all the variables they created are gone.

AnmAtAnm commented 6 years ago

The current behavior may be a side effect of emulating the behavior of the original variable behavior (variables had a major rewrite last year). The requested behavior is now supported at the XML level. For example:

<xml xmlns="http://www.w3.org/1999/xhtml">
  <variables>
    <variable type="">abc</variable>
    <variable type="">def</variable>
    <variable type="">ghi</variable>
  </variables>
</xml>

Open question: Is the "delete variable" option visible enough to users to support the requested behavior? A user with a workspace that they revisit (save/reload) currently has the benefit of removing such unused variables automatically. If the variables are preserved, we are expecting the user to manual remove such variables over the lifetime of the project. (Admittedly, this is an advanced use case.)

NAllred91 commented 6 years ago

For what its worth, this seems to be the commit that changed the behavior. (in core/xml.js)

I doubt this causes issues for most people, but it is important in my environment so I've temporarily worked around it by just overwriting Blockly.Xml.workspaceToDom

I do think it would be beneficial if the option to delete the variable was more visible, but I'm not sure if that justifies the save/reload action having side effects that change the state of the workspace?

Would it make sense to have this behavior configurable when you create the workspace using Blockly.inject?

rachel-fenichel commented 6 years ago

The original variable behaviour did not preserve unused variables, because it inspected the entire workspace every time it needed a list of variables, and only used ones found on the workspace.

The intermediate behaviour, when we first added variable models and typed variables, preserved all variables.

The current behaviour is a reversion to the original behaviour: unused variables disappear.

The current behaviour is intentional. Deleting variables is currently not nearly as clean as creating variables: you have to drag out a block that uses variables, open the dropdown, select the variable you want to delete, reopen the dropdown, and select the delete option. The current behaviour removes unused variables, decluttering the variable dropdowns and flyout, while never touching anything that is actively in use.

@NAllred91 your example is a case when "in use" means "declared" rather than "set or read". I will think about the tradeoff between auto-cleanup and preserving that information.

For your particular use case, I would consider tracking your device variables independently by name (since you need to do that anyway to keep track of the actual devices) and then calling workspace.createVariable per device, both on workspace load and on device connection/creation.

NAllred91 commented 6 years ago

@rachel-fenichel Thanks, in my specific case I have been slightly concerned that my management of device variables is so reliant on Blockly's variable management. We do plan on adding text programming support to our environment, at which point I think it'll be important to stop using the workspace xml to maintain all of the state. When I initially implemented this I thought I was making a good decision by leveraging Blockly's variable management, as it does simplify things a bit on my end. Now I'm not as convinced of that decision..

Independent of my specific situation, it seems like it would make more sense to just have some method for cleaning up unused variables that developers could decide when to call. To me, it was unexpected that the act of saving/loading would have the effect of modifying the workspace in any way.

For the moment my workaround is fine though, and I doubt this is a common issue at all. Thanks!

AnmAtAnm commented 6 years ago

One solution might be to support this as a save option to workspaceToDom(...). As noted above, the loading code already supports this.

I see a trend of such options: currently with/without block IDs; a few projects are hacking in realigning the code to <0,0> top left; and now this variables request. We can deprecate opt_noId, supporting the boolean forever, but preferring an object map of options (testing typeof).

BeksOmega commented 6 months ago

In JSON we serialize all variables. It's also much easier to delete variables, because you can right click letters in the flyout to delete them.