blockly-samples icon indicating copy to clipboard operation
blockly-samples copied to clipboard

Pure validators and idiomatic support for handling dependent fields such as dependent dropdowns

Open jschanker opened this issue 4 years ago • 7 comments

Category

  • Plugins

Component

  • Field

Is your feature request related to a problem? Please describe.

It is currently possible for valid states to incorrectly be labeled as invalid or invalid states to be loaded from XML/JSON. This can occur when correct validation of fields depends on the values of other fields, block connections, or other workspace state. Related to this, developers sometimes need to resort to creating inelegant and potentially confusing error-prone workarounds to address validators not being explicitly given all the inputs they need to determine whether values are valid as illustrated in this forum post here: https://groups.google.com/g/blockly/c/uilMLN_VojA/m/sKd6LjNNAwAJ.

Describe the solution you'd like

The ability to use pure validators in a standardized way to ensure that fields dependent on other workspace state always have valid values and that valid states can be deserialized from XML/JSON. The following is a quick draft of some of the tasks that could facilitate such a plugin (Suggestions for improvements or refinements are very much welcome!):

  • [ ] Add (protected) instance variables virtualState to the Block and Workspace classes representing "virtual" state that is used for determining/validating actual workspace state such as the values of fields, the descendants of blocks, etc.
  • [ ] Add a setState method to the Block and Workspace classes to update the virtual state, if the change is valid and then update the actual state, accordingly:
    1. Freeze the actual workspace state to prevent inadvertent changes.
    2. Call the relevant pure getNewState listeners, to get additional updates to the state or reject the proposed changes.
    3. Call the relevant pure validateState validators to determine if the resulting virtual state from the final getNewState listener requesting changes is valid.
    4. Unfreeze the workspace.
    5. If all validators agree that the final virtual state is valid, set the workspace or block virtualState to be this new state and call the relevant virtual state => actual state function that makes the necessary changes to the block/workspace to reflect the new virtual state.
  • [ ] Add a Block/Workspace method addStateListener to add a pure function getNewState listener. These listeners would request additional changes through their outputs or defer additional changes until more properties of the desired new state are determined, accordingly. They would do this by receiving the current state and currently constructed new state as inputs and return the new desired state as an output. The resulting output would then be passed as the new state input to the next listener. Alternatively, it can return undefined to defer its request to make changes to the new state, which would put it at the end of the listener queue, or return null to assert that this change is invalid and stop the process. To prevent functions from perpetually deferring, setState would stop this process if a listener was returned to without any requested changes being made by the remaining listeners from its last deferment.
  • [ ] Add a Block/Workspace method addStateValidator to add pure function validators. After setState was called and all listeners were dispatched, the final new state output from a getNewState listener (or the input to setState if there were no listeners requesting changes) would be passed as an an input to each of these validators. If every such validator returns true, the state change is accepted. Otherwise, it is rejected.
  • [ ] Add a Block/Workspace method setVirtualToActualStateConverter to set the function responsible for converting virtual state to actual state. I.e., given a virtual state as an input, the function would call the appropriate methods such as setFieldValue to make the workspace reflect the new final accepted virtual state from setState.
  • [ ] Add a workspace/block method to freeze/unfreeze all actual workspace/block state to ensure the virtual state listeners/validators don't inadvertently change actual state. This should be a developer freeze in that methods such as setFieldValue should throw an error when trying to use it to change a field value, as opposed to a user freeze such as calling setEditable(false) on a block which only prevents a field value from being changed by clicking on the field. This could be done by adding protected Boolean properties isFrozen that are checked by any methods that change state.
  • [ ] Allow fields to have a disableSaving method so that their field values would not be serialized or deserialized (even if they were present). The point is to have these values set separately after they have gone through the validation process as described above.
  • [ ] Extend serialization methods to include this virtual workspace/block state. Extend deserialization methods to call all relevant validators on the saved virtual state before calling the virtual => actual state converters, if validation is successful.

Describe alternatives you've considered

Providing extra documentation to standardize best practices around handling common use cases such as dependent dropdowns or providing plugins for each of these use cases.

Additional context

For current examples in which you can get unavailable or invalid field values after serializing and surrounding discussion, see https://github.com/google/blockly/issues/5787 and https://github.com/google/blockly/issues/5793. If this seems like a viable/useful idea, I could try to have a few of my students work on this next semester.

jschanker avatar Dec 28 '21 08:12 jschanker

Hi Jason!

Thanks for taking the time to write up this issue and some of the other issues relating to this!

First off, I think that this would make a great third party plugin. Looking at our criteria for first/third party plugins I believe this fits into the categories 'more complex' and 'more targeted'. Keep in mind for third party plugins we are still very happy to do reviews and consult on the plugin, the plugin would just live in a different repository and we would not be in charge of maintaining/publishing it.

I am also wondering if there is a way to do what you are trying to do without having to add methods on to the Block or the Workspace? We generally advise that plugins use our public APIs and refrain from monkey patching core. We have made exceptions for our team early on so that we could get some plugins out. However, with the new version of Blockly we are experiencing some fun head aches, so I would definitely not recommend. It is not required for third party plugins, but generally it will help you out if you want to keep the plugin up to date with newer versions of Blockly.

Let me know if you have any questions!

alschmiedt avatar Jan 21 '22 17:01 alschmiedt

Hi Abby, thanks for reviewing this. I completely agree. What started out as a simple idea for a plugin to handle dependent dropdowns became more complex when I uncovered these theoretical problems and tried to create a framework for handling all these issues together. I'd have to think about this more, but I think the step of doing a developer freeze of the actual workspace state would be challenging without monkey patching the core. Perhaps an error could be thrown after a change if the actual workspace state that's saved before the freeze is different after unfreezing, but I'm not sure of a clean way to prevent other functions from changing the workspace during validation.

I spoke with the students about this being a more targeted plugin, and they seemed to prefer working on one that would have wider use, which I think is important since they're going to be applying for jobs very soon. So, I definitely want to return to this, but in the meantime, do you have any ideas for plugins with high demand that would be somewhat simple to implement? A sorted search of help wanted issues by the number of 👍 yielded top results of Blockly Workspace Minimap, Blocks from context menus, Screenshot download plugin, and Shift-drag to select multiple blocks and allow moving multiple block group, but I haven't thought about what would be involved in implementing these features.

Thanks again! @alschmiedt

jschanker avatar Feb 04 '22 22:02 jschanker

Hi Jason!

Generally we recommend fields for a first plugin since we have a lot of documentation on creating custom fields and we already have a few plugins as examples. (examples here, here and here). One field plugin that comes to mind is the combobox field.

If a field doesn't interest you/your student then I think the Minimap or the screenshot plugin would probably be the best out of your list. I think both of these would require minimal changes to core.

The context menu one would require rewriting the context menus in core which would be a much larger endeavor and AFAICT the shift drag issue would also require some new APIs in core. Whatever they choose let me know! More than happy to answer more specific questions once they choose a plugin : )

alschmiedt avatar Feb 08 '22 21:02 alschmiedt

Hi Abby, The combobox field plugin sounds like a great one to start with! One student responded that he'd be interested in working on this one, and I'm sure the others would be happy to do the same. Also, given that there still seem to be questions about dependent dropdowns, maybe a more narrowly defined plugin that facilitates their implementation, could be something else the students could work on and could offer a liteweight alternative for now. Do you have any recommendations for getting started with the combobox plugin (e.g., design requirements)? Appreciate all of your help! @alschmiedt

jschanker avatar Feb 11 '22 20:02 jschanker

Sorry for the delay in getting back to you. I have some questions about the combo box that I am hoping to get answered tomorrow that will help me give you more concrete information on next steps. In the mean time I commented on the combo box issue with a few more links that might be useful for your student.

alschmiedt avatar Feb 16 '22 00:02 alschmiedt

Ok, I spent the morning look more into the combobox field that I previously linked and I am 100% going back on what I previously said about it being a good starter plugin. Sorry about that :/ the more I dug into it the more issues I found. Just in case you are curious the biggest problems are that it requires multiple methods from the FieldDropdown that are currently private and the makecode implementation reaches into private methods of the FieldTextInput as well.

Would your student be interested in this plugin instead? It uses some well defined APIs so it will probably be a bit easier to get started on. @jschanker

alschmiedt avatar Feb 16 '22 23:02 alschmiedt

@alschmiedt No worries at all, and thank you very much for spending the time looking into this! The most (recently) used blocks dynamic category sounds like a straightforward plugin to implement, and the students are interested in working on it. I'm asking them to post some clarifying questions on the requirements. Thanks again!

jschanker avatar Feb 17 '22 02:02 jschanker