Group MODE field changes with disconnect events into single undo entry#438
Open
tracygardner wants to merge 1 commit intomainfrom
Open
Group MODE field changes with disconnect events into single undo entry#438tracygardner wants to merge 1 commit intomainfrom
tracygardner wants to merge 1 commit intomainfrom
Conversation
…orrect undo When the user changes an if_clause block's MODE dropdown (e.g. IF → ELSE) while a following block needs to be disconnected, validateAndDisconnectInvalidChain_ was firing BLOCK_MOVE events before Blockly fired the BLOCK_CHANGE for the field. With no active event group, these landed in separate undo entries, requiring two Ctrl+Z presses to undo a single user action. Fix: the dropdown validator now opens an event group when none is already active, and the onchange handler closes it once the corresponding BLOCK_CHANGE fires. This ensures the disconnect and the field change are always a single undo entry. https://claude.ai/code/session_013VxihhypLXT63EyPWiBk6j
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant User
participant ModeHandler as MODE Handler
participant EventSys as Blockly Event System
participant ValidateDisconnect as validateAndDisconnectInvalidChain_
participant OnChange as onchange Handler
User->>ModeHandler: Change MODE dropdown
ModeHandler->>EventSys: Open event group (store _modeChangeGroupId)
ModeHandler->>ValidateDisconnect: Call validateAndDisconnectInvalidChain_
ValidateDisconnect->>EventSys: Emit BLOCK_MOVE events
ModeHandler->>EventSys: Emit BLOCK_CHANGE event for MODE field
EventSys->>OnChange: Trigger onchange with event details
OnChange->>EventSys: Detect matching group id
OnChange->>EventSys: Close event group
OnChange->>OnChange: Clear _modeChangeGroupId
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
blocks/control.js (1)
476-483: Guard group creation to real MODE changes only.At line 480, a group can be opened before confirming this selection will emit a matching
BLOCK_CHANGEevent. In current Blockly core, if a FieldDropdown validator returns the same value as the current field value, noBLOCK_CHANGEevent is emitted. This means cleanup at lines 603-614 may never run, leaving the event group stale.Suggested hardening
new Blockly.FieldDropdown( [ ["%{BKY_CONTROLS_IF_MSG_IF}", MODE.IF], ["%{BKY_CONTROLS_IF_MSG_ELSEIF}", MODE.ELSEIF], ["%{BKY_CONTROLS_IF_MSG_ELSE}", MODE.ELSE], ], (newValue) => { + const currentValue = this.getFieldValue("MODE"); + const willActuallyChange = newValue !== currentValue; // If there's no active event group, open one so that any // BLOCK_MOVE events fired by validateAndDisconnectInvalidChain_ // land in the same undo entry as the BLOCK_CHANGE that Blockly // fires for this field after the validator returns. - if (!Blockly.Events.getGroup() && !this._modeChangeGroupId) { + if ( + willActuallyChange && + Blockly.Events.isEnabled() && + !Blockly.Events.getGroup() && + !this._modeChangeGroupId + ) { Blockly.Events.setGroup(true); this._modeChangeGroupId = Blockly.Events.getGroup(); } this.updateShape_(newValue); return newValue; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/control.js` around lines 476 - 483, The code currently opens an event group unconditionally (using Blockly.Events.getGroup / setGroup and this._modeChangeGroupId) before confirming a real MODE change; if a FieldDropdown validator returns the same value no BLOCK_CHANGE fires and the group can be left stale. Modify the logic in the mode-changing flow (where validateAndDisconnectInvalidChain_ and the dropdown selection are handled) to only call Blockly.Events.setGroup(...) and assign this._modeChangeGroupId when the new mode/value is different from the current value (compare against this.getValue() or the field's current mode) or after the validator confirms a different value will be applied; keep the existing cleanup code but ensure it only runs when a group was actually opened (i.e., this._modeChangeGroupId is set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@blocks/control.js`:
- Around line 476-483: The code currently opens an event group unconditionally
(using Blockly.Events.getGroup / setGroup and this._modeChangeGroupId) before
confirming a real MODE change; if a FieldDropdown validator returns the same
value no BLOCK_CHANGE fires and the group can be left stale. Modify the logic in
the mode-changing flow (where validateAndDisconnectInvalidChain_ and the
dropdown selection are handled) to only call Blockly.Events.setGroup(...) and
assign this._modeChangeGroupId when the new mode/value is different from the
current value (compare against this.getValue() or the field's current mode) or
after the validator confirms a different value will be applied; keep the
existing cleanup code but ensure it only runs when a group was actually opened
(i.e., this._modeChangeGroupId is set).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change ensures that when the MODE field is changed on a control block, any block disconnections triggered by validation are grouped together with the field change into a single undo entry, providing a better user experience.
Key Changes
_modeChangeGroupIdproperty to track event groups opened during MODE field changesonchangehandler to close the event group once Blockly fires the correspondingBLOCK_CHANGEevent for the MODE fieldImplementation Details
The solution works by:
_modeChangeGroupIdupdateShape_method then executes, potentially triggeringBLOCK_MOVEevents fromvalidateAndDisconnectInvalidChain_BLOCK_CHANGEevent for the field change, theonchangehandler detects it and closes the event groupThis approach respects existing event groups and only creates new ones when necessary, avoiding interference with other event grouping logic.
https://claude.ai/code/session_013VxihhypLXT63EyPWiBk6j
Summary by CodeRabbit