Skip to content

Conversation

@xenonnn4w
Copy link
Contributor

@xenonnn4w xenonnn4w commented Nov 18, 2025

Purpose / Description

When undoing the note, the unlabeled entry from notetypes.save() would be exposed, causing undoAvailable() to return false and hiding previous deck operations from the undo stack.

Fixes

Approach

Remove notetypes.save() from undoableOp block as it's redundant - the note already contains notetype information.

How Has This Been Tested?

undo.mp4

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@BrayanDSO
Copy link
Member

BrayanDSO commented Nov 18, 2025

Tests fail, and that probably means that the fix is wrong.

Please run CI before making a PR. You can run it in your GH branch as well if you don't want to run it locally.

@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Nov 18, 2025
@BrayanDSO BrayanDSO marked this pull request as draft November 18, 2025 09:48
@xenonnn4w
Copy link
Contributor Author

The saveNoteWithProgress() function was calling both notetypes.save() and addNote() inside the same undoableOp block. This created two separate backend undo entries:

  • Unlabeled entry from addOrUpdateNotetype()
  • Labeled "Undo Add Note" entry

When undoing the note, the unlabeled entry was exposed, causing undoAvailable() to return false and hiding previous operations.
Solution:

  • Removed notetypes.save() from saveNoteWithProgress() to create only one undo entry.

Trade-off:
This breaks issue #13931 - the notetype's "last used deck" preference is no longer persisted. Users
will need to manually select the deck each time they add a note.
The test NoteEditorTest; decide by note type preference - 13931 has been disabled to accomplish this.

This is a temporary fix. The proper solution requires backend changes i.e have addNote() automatically update notetype.did,

@BrayanDSO
Copy link
Member

The proper solution requires backend changes i.e have addNote() automatically update notetype.did,

So, we should wait for that.

@BrayanDSO BrayanDSO added Blocked by dependency Currently blocked by some other dependent / related change and removed Needs Author Reply Waiting for a reply from the original author labels Nov 19, 2025
@xenonnn4w xenonnn4w removed the Blocked by dependency Currently blocked by some other dependent / related change label Nov 20, 2025
@xenonnn4w
Copy link
Contributor Author

The backend already handles "last used deck" tracking automatically via defaultDeckForNoteType().
Latest commit updates the test to use modern API - all tests passing.
Backend issue closed: ankitects/anki#4434
Ready for review!

@xenonnn4w xenonnn4w marked this pull request as ready for review November 20, 2025 20:18
@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge squash-merge A squash & force push is required. The PR author may do this to speed up the merge process. and removed Needs Review labels Nov 21, 2025
@david-allison david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed squash-merge A squash & force push is required. The PR author may do this to speed up the merge process. labels Nov 22, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Squashed, updated commit message, fixed an unnecessary newline change, and force pushed

Thanks!!!!

@david-allison david-allison removed the Needs Second Approval Has one approval, one more approval to merge label Nov 22, 2025
Updating the card template is handled by the backend
 so we did not need to save it

----

test: update NoteEditorTest to use modern defaultDeckForNoteType API:
  The test "decide by note type preference - 13931" was using deprecated notetype.did field.

  Updated to use defaultDeckForNoteType() which queries the backend's automatic
   "last used deck" tracking.

----

fix: remove deprecated notetype.did usage to fix undo stack as after performing
 a deck operation then adding a note, undoing the note would clear the undo queue.
 This was caused by the deprecated notetype.did API
 requiring a notetypes.save() call, which created an unlabeled undo entry.

----

 fix: remove notetypes.save() to fix undo queue breaking (Issue 19508)

  Removed notetypes.save() from saveNoteWithProgress() to prevent
  unlabeled undo entry that was breaking the undo stack after undoing
  a note addition.

Fixes 19508

Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
Merged via the queue into ankidroid:main with commit c72b282 Nov 22, 2025
15 checks passed
@github-actions github-actions bot added this to the 2.23 release milestone Nov 22, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Nov 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Undo Add Note" breaks the undo stack in the decks screen

3 participants