Provide API for ignoring some multi-field updates

[mark-friedman]

The basics

• [x] I validated my changes

The details

Resolves

Fixes #57

Proposed Changes

Allows a user of the plugin to temporarily disable mult-ifield updates within a particular scope.

Reason for Changes

Multi-field updates can cause issues with dependent fields on the same block. See issue #57 anad the updated README file in this PR for some more details.

Test Coverage

Documentation

This PR includes additions to the README file.

Additional Information

[HollowMan6]

I have no strong objection against merging this PR, but here is a checklist before we do so to ensure everything is intended:

• [ ] If we set a new group instead of continuing with the old group, have you checked that this doesn't break the redo/undo (so that we still have both programmatically and multi-field updates reverted together)? AFAIK, we should always include those changes in the same group ID to get this working.
• [ ] This is a helper function that doesn't include any uses of the internals of the plugin, so I doubt if it's necessary to include this in the plugin instead of documenting this and the developer implementing this wapper at their side.

[mark-friedman]

@HollowMan6 While testing redo/undo I discovered something worse, which is that multi-field updates of input fields (like the "math _number" or "text" blocks) was broken! That's because the events for those changes have group IDs set. I'm guessing that that was added when Blockly added the "block_field_intermediatechange" event. Given that, I think that we need to revert your change which added the e.group === '' test to `eventListener`. Unfortunately, it also mean that we'll have to disable the multi-field update option in App Inventor.

[mark-friedman]

Withdrawing this PR. See https://github.com/mit-cml/workspace-multiselect/pull/61#issuecomment-2189527061 for why.

[HollowMan6]

Just pushed https://github.com/mit-cml/workspace-multiselect/commit/698fb5100abd5c91fac9155c475e119faad4e16d to fix the multi update under "block_field_intermediate_change" event. In this case, it looks like we just need to keep track of the group IDs for those events and make sure that either e.group === '' or e.group is within the group IDs of intermediate changes, and there's no need to worry too much about this.

Also, although normally you can get the multi-field group ID by listening to events, to make developer's life easier, now I also expose the group ID through the multiFieldUpdateGroupID field. Now we can still wrap any dependent fields by saving any existing group id, set the group ID here, do our work, reset the group id (in the finally clause), just as mentioned in https://github.com/mit-cml/workspace-multiselect/issues/57#issuecomment-2175980296 and https://github.com/mit-cml/workspace-multiselect/commit/bd32cd1b0f052860a4b6f967cc7fd66e305f8b83#commitcomment-143269534)

[mark-friedman]

Just pushed 698fb51 to fix the multi update under "block_field_intermediate_change" event. In this case, it looks like we just need to keep track of the group IDs for those events and make sure that either e.group === '' or e.group is within the group IDs of intermediate changes

Ok, I'll test that out.

[mark-friedman]

Ok, I'll test that out.

Looks good. Ideally we would see the intermediate changes on the other fields as the user typed, but I think the current behavior is acceptable.

[mark-friedman]

• If we set a new group instead of continuing with the old group, have you checked that this doesn't break the redo/undo (so that we still have both programmatically and multi-field updates reverted together)? AFAIK, we should always include those changes in the same group ID to get this working.

Good catch. I have fixed that.

[mark-friedman]

This is a helper function that doesn't include any uses of the internals of the plugin, so I doubt if it's necessary to include this in the plugin instead of documenting this and the developer implementing this wapper at their side.

I'm going to push back a little on this. While it doesn't use the internals of the plugin, it does depend on those internals by knowing that the way to get the field's events ignored within eventListener_ function is to set a group id.

... now I also expose the group ID through the multiFieldUpdateGroupID field. Now we can still wrap any dependent fields by saving any existing group id, set the group ID here, do our work, reset the group id (in the finally clause)

I think that this approach creates a leaky abstraction. It requires the user of the plugin to be aware of details of the underlying implementation. That's why I created the withoutMultiFieldUpdates function in the plugin (and in the Multiselect class), so that if anything ever changes in the eventListener_ function, we can update withoutMultiFieldUpdates and the user doesn't care.

[HollowMan6]

Looks good. Ideally we would see the intermediate changes on the other fields as the user typed, but I think the current behavior is acceptable.

I don't find any API for doing those intermediate changes, plus #58 applies as well if we want to have this, and this time, we have a random group ID. So, let's not complicate things further unless someone is interested in implementing this.

[mark-friedman]

You can go ahead to remove the multiFieldUpdateGroupID field, which I introduced in my last commit

Done

[mark-friedman]

Looks good. Ideally we would see the intermediate changes on the other fields as the user typed, but I think the current behavior is acceptable.

I don't find any API for doing those intermediate changes, plus #58 applies as well if we want to have this, and this time, we have a random group ID. So, let's not complicate things further unless someone is interested in implementing this.

I think you would just need to change e.type === Blockly.Events.CHANGE to e.type === Blockly.Events.CHANGE || Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE in the eventListener_ function.

[HollowMan6]

I'm not sure what goes wrong, looks like there are no conflicts, but GitHub says it has... Not sure if it's because you closed this PR once, and you might want to supersede it with a new one if this doesn't work out. @mark-friedman

[mark-friedman]

Now LGTM. I would like to merge this one now but the GitHub bug #61 (comment) stops me from doing so. Maybe we can open a new PR to supersede this one.

It now seems to say "This branch has no conflicts with the base branch" so maybe we're ok. If not, let me know and I'll create a new PR.

[HollowMan6]

It now seems to say "This branch has no conflicts with the base branch" so maybe we're ok. If not, let me know and I'll create a new PR.

Not like that at my side. Actually I found the similar thing for Chang Min's newly opened PR, which makes this even stranger, so I'm not sure if it's more about a privilege issue or not. cc @ewpatton to see if he can merge the PR here.

[mark-friedman]

Further weirdness is that now my GitHub page for this PR says "This branch has conflicts that must be resolved" despite nothing in the code having changed since when I reported that it said there were no conflicts.

[mark-friedman]

And now it says "This branch has no conflicts with the base branch". Very very weird.

[HollowMan6]

I think you would just need to change e.type === Blockly.Events.CHANGE to e.type === Blockly.Events.CHANGE || Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE in the eventListener_ function.

Further weirdness is that now my GitHub page for this PR says "This branch has conflicts that must be resolved" despite nothing in the code having changed since when I reported that it said there were no conflicts.

And now it says "This branch has no conflicts with the base branch". Very very weird.

Ah, acutally I have just got this supported and pushed one commit into main, and I have helped you resolved the conflit, but still it shows "This branch cannot be rebased due to conflicts" at my side, even now your side seems to be normal. I'll open a ticket at GitHub support to find out what's going on

[mark-friedman]

I think we've got some bad merging happening. The eventListener_ function looks wrong now. The existence and use of the fieldIntermediateChangeGroupIds_ set is gone, the finally block has changed and the big conditional near the beginning is incorrect.

[HollowMan6]

Now I know what's going on, I just can't use rebase and merge here in this case since we have several merge commits in the commit history, so I'll use Squash and merge to merge

[mark-friedman]

I think that this is what we want the eventListener_ function to look like:

  eventListener_(e) {
    if (e.type === Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE) {
      // Keep track of the group ids for intermediate changes.
      this.fieldIntermediateChangeGroupIds_.add(e.group);
    // on Block field changed
    } else if (this.multiFieldUpdate_ &&
        (e.type === Blockly.Events.CHANGE || e.type === Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE) &&
        e.element === 'field' && e.recordUndo &&
        this.blockSelection_.has(e.blockId) &&
        (e.group === '' ||
          this.fieldIntermediateChangeGroupIds_.has(e.group))) {
      const currentGroup = Blockly.Events.getGroup();
      if (e.group !== '' && e.group !== currentGroup) {
        // Intermediate changes are finished, remove the group id.
        this.fieldIntermediateChangeGroupIds_.delete(e.group);
      }
      const inGroup = !!currentGroup;
      if (!inGroup) {
        Blockly.Events.setGroup(true);
        e.group = Blockly.Events.getGroup();
      }
      try {
        const blockType = this.workspace_.getBlockById(e.blockId).type;
        // Update the fields to the same value for
        // the selected blocks with same type.
        this.blockSelection_.forEach((id) => {
          if (id === e.blockId) {
            return;
          }
          const block = this.workspace_.getBlockById(id);
          if (block.type === blockType) {
            block.setFieldValue(e.newValue, e.name);
          }
        });
      } catch (err) {
        // Avoid errors when changing a block if it is
        // no longer in the workspace.
        // https://github.com/mit-cml/workspace-multiselect/issues/33
        console.warn(err);
      } finally {
        if (!inGroup) {
          Blockly.Events.setGroup(false);
        } else {
          Blockly.Events.setGroup(currentGroup);
        }
      }
    }
  }

And we want the constructor to be:

constructor(workspace) {
    this.workspace_ = workspace;
    this.origHandleWsStart_ = Blockly.Gesture.prototype.handleWsStart;

    blockSelectionWeakMap.set(this.workspace_, new Set());
    this.blockSelection_ = blockSelectionWeakMap.get(this.workspace_);
    inMultipleSelectionModeWeakMap.set(this.workspace_, false);
    BaseBlockDraggerWeakMap.set(this.workspace_, Blockly.BlockDragger);
    this.fieldIntermediateChangeGroupIds_ = new Set();
    this.useCopyPasteCrossTab_ = true;
    this.useCopyPasteMenu_ = true;
    this.multiFieldUpdate_ = true;
  }

[HollowMan6]

I think we've got some bad merging happening. The eventListener_ function looks wrong now. The existence and use of the fieldIntermediateChangeGroupIds_ set is gone, the finally block has changed and the big conditional near the beginning is incorrect.

Yes, this is intended, we don't need fieldIntermediateChangeGroupIds_ any more now. The finally block is intended (we still have an empty group if we are not in a group, so these can be merged and are intended

[mark-friedman]

I think we've got some bad merging happening. The eventListener_ function looks wrong now. The existence and use of the fieldIntermediateChangeGroupIds_ set is gone, the finally block has changed and the big conditional near the beginning is incorrect.

Yes, this is intended, we don't need fieldIntermediateChangeGroupIds_ any more now. The finally block is intended (we still have an empty group if we are not in a group, so these are intended

Oh, ok. I think that the if statement still needs to be fixed, though.

[HollowMan6]

I think that this is what we want the eventListener_ function to look like:

We don't have e.element field in Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE, and e.recordUndo is false, so we don't check these for BLOCK_FIELD_INTERMEDIATE_CHANGE

[HollowMan6]

Anyway, there's nothing unintentional when I check https://github.com/mit-cml/workspace-multiselect/pull/61/files, so I'll just merge this one. If we do find anything for the if statement, then it's introduced in https://github.com/mit-cml/workspace-multiselect/commit/698fb5100abd5c91fac9155c475e119faad4e16d I'll merge this PR now.

[mark-friedman]

I think that this is what we want the eventListener_ function to look like:

We don't have e.element field in Blockly.Events.BLOCK_FIELD_INTERMEDIATE_CHANGE, and e.recordUndo is false, so we don't check these for BLOCK_FIELD_INTERMEDIATE_CHANGE

Ah, ok. Sorry for the panic. That's the danger when multiple people are changing parts of the same code at the same time, and thinking that there were, perhaps, weird merge conflicts.