Skip to content

feat: add undo and redo shortcuts for deleting blocks#7714

Open
lavjeetrai wants to merge 1 commit into
sugarlabs:masterfrom
lavjeetrai:trash-fix
Open

feat: add undo and redo shortcuts for deleting blocks#7714
lavjeetrai wants to merge 1 commit into
sugarlabs:masterfrom
lavjeetrai:trash-fix

Conversation

@lavjeetrai

Copy link
Copy Markdown
Contributor

Description

This PR upgrades the Ctrl+Z and Ctrl+Y keyboard shortcuts to support a generalized undo/redo history using the Command Pattern. Users can now flawlessly revert and reapply both block movements and block deletions.

Screen.Recording.2026-07-01.233456.mp4

Key Changes

  • Action History Stack: Replaced the primitive trashStacks system with an actionHistory and redoActionHistory array in blocks.js to log generalized { type: "move" | "trash", ... } objects.

  • Coordinate Tracking: Added mousedown coordinate tracking to record precise start/end locations of blocks when dragged across the canvas.

  • Feature

@github-actions github-actions Bot added feature Adds new functionality size/S Small: 10-49 lines changed area/javascript Changes to JS source files labels Jul 1, 2026
@github-actions github-actions Bot added size/M Medium: 50-249 lines changed and removed size/S Small: 10-49 lines changed labels Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 61.42857% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.19%. Comparing base (7dd8361) to head (7b8ddfa).
⚠️ Report is 35 commits behind head on master.

Files with missing lines Patch % Lines
js/blocks.js 69.49% 18 Missing ⚠️
js/activity.js 0.00% 7 Missing ⚠️
js/block.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7714      +/-   ##
==========================================
+ Coverage   54.12%   54.19%   +0.06%     
==========================================
  Files         172      172              
  Lines       57072    57133      +61     
==========================================
+ Hits        30892    30964      +72     
+ Misses      26180    26169      -11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the area/tests Changes to test files label Jul 1, 2026

@Chaitu7032 Chaitu7032 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

the feature is working tested locally 👍. good work .

but Looks like sendStackToTrash() and restoreTrashById() already add the action to actionHistory. Do we also need this.actionHistory.push(action) here ? It seems like this could create duplicate history entries after redo.

@lavjeetrai lavjeetrai force-pushed the trash-fix branch 2 times, most recently from b2a2c79 to 13755b4 Compare July 2, 2026 14:55
@github-actions github-actions Bot added size/XXL XXL: 1000+ lines changed needs-rebase and removed size/M Medium: 50-249 lines changed labels Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

@lavjeetrai lavjeetrai marked this pull request as draft July 2, 2026 14:56
@github-actions github-actions Bot removed the area/tests Changes to test files label Jul 2, 2026
@github-actions github-actions Bot added the area/tests Changes to test files label Jul 2, 2026
@github-actions github-actions Bot removed needs-rebase size/XXL XXL: 1000+ lines changed labels Jul 2, 2026
@github-actions github-actions Bot added the size/M Medium: 50-249 lines changed label Jul 2, 2026
@lavjeetrai

Copy link
Copy Markdown
Contributor Author

@Chaitu7032 thanks for this critical reveiw, i have fixed it ,now take a look .

@lavjeetrai lavjeetrai marked this pull request as ready for review July 2, 2026 15:54
@lavjeetrai

Copy link
Copy Markdown
Contributor Author

@walterbender @zealot-zew what u guys think about this feature?

@walterbender

Copy link
Copy Markdown
Member

I've not tested your code yet. Will do over the weekend. This has been tried numerous times but we've yet to strike the right balance in terms of what undo granularity should be. Each block movement? Only block movements that cause a connection change? Values in arg blocks? etc. I'd be curious as to how you determined what a change consists of.

@walterbender

Copy link
Copy Markdown
Member

This does a good job as far as it goes. But one UX issue and a lot of questions.
My instinct was to repeatedly type ctrl-z to restore multiple changes. But it seems that I could only type one at a time and wait for the message to appear each time. Is there a reason for that?
When restoring from the trash, the block was restored but not put back to the same position it was deleted from.
image

I would expect to see changes in block values restored as well: if I change a pitch, ctrl-z says "nothing to do."

@lavjeetrai lavjeetrai closed this Jul 4, 2026
@lavjeetrai lavjeetrai deleted the trash-fix branch July 4, 2026 15:11
@lavjeetrai lavjeetrai restored the trash-fix branch July 4, 2026 15:11
@lavjeetrai lavjeetrai reopened this Jul 4, 2026
@lavjeetrai

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! @walterbender

Regarding the Ctrl+Z UX issue, there wasn't any specific reason behind requiring users to wait between consecutive undo operations. I'll look into improving that behavior so multiple undo actions can be triggered seamlessly.

I'll also start working on the other points you mentioned, including restoring deleted blocks to their original position and supporting undo for block value changes, and I'll update the PR as I make progress.

@stutijain2006

Copy link
Copy Markdown
Contributor

Hi @lavjeetrai
I raised a PR long time back adding undo redo functionality only in three major cases - out of which one was deleting a block as well, and it was working perfectly fine in all cases and in collaboration with restore from stack as well, you can have a look, and it adds undo/redo to block addition and movement as well. It was closed due to inactivity for 60 days.

#4903

@stutijain2006

Copy link
Copy Markdown
Contributor

Hi @lavjeetrai I raised a PR long time back adding undo redo functionality only in three major cases - out of which one was deleting a block as well, and it was working perfectly fine in all cases and in collaboration with restore from stack as well, you can have a look, and it adds undo/redo to block addition and movement as well. It was closed due to inactivity for 60 days.

#4903

There was only one bug in that PR that adding a new block directly in the existing block and undo that leads to white space. This issue is mentioned in the comments of that PR as well. So if you wish to work on that PR, you can do that, I will merge your commits into the same.

@lavjeetrai

Copy link
Copy Markdown
Contributor Author

Hi @stutijain2006, thanks for letting me know and for sharing your PR!

Sorry, but I'm already familiar with the implementation in my current PR, and I don't have enough time to go through the older PR in detail. I think it's more efficient for me to continue with my current approach.

That said, since you've already worked on this, I'd really appreciate your help if I run into any issues or need guidance while finishing it.

Sorry and Thanks again!

@stutijain2006

stutijain2006 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Hi @stutijain2006, thanks for letting me know and for sharing your PR!

Sorry, but I'm already familiar with the implementation in my current PR, and I don't have enough time to go through the older PR in detail. I think it's more efficient for me to continue with my current approach.

That said, since you've already worked on this, I'd really appreciate your help if I run into any issues or need guidance while finishing it.

Sorry and Thanks again!

That's completely fine. No issues. Keep on working on your PR, and can ask me for help if you face any difficult in between.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files feature Adds new functionality size/M Medium: 50-249 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

4 participants