Skip to content

fix(note-editor): Deck for "new cards: decide by note type"#19734

Merged
mikehardy merged 3 commits intoankidroid:mainfrom
david-allison:19733
Dec 11, 2025
Merged

fix(note-editor): Deck for "new cards: decide by note type"#19734
mikehardy merged 3 commits intoankidroid:mainfrom
david-allison:19733

Conversation

@david-allison
Copy link
Copy Markdown
Member

@david-allison david-allison commented Dec 10, 2025

Purpose / Description

Introduced in 2644a6d

  • decide by note type was broken

Fixes

Approach

How Has This Been Tested?

  • Using user-provided collection
  • Changed deck
  • Added a new note
  • Exited
  • Entered
  • Previously selected deck was maintained

Checklist

  • 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

@david-allison david-allison added Review High Priority Request for high priority review Blocked by dependency Currently blocked by some other dependent / related change labels Dec 10, 2025
@david-allison david-allison added this to the 2.23.1 release milestone Dec 10, 2025
@david-allison
Copy link
Copy Markdown
Member Author

IMO: too close to the release window to block. Save for a quick 2.23.1

@david-allison david-allison added Needs Review Queued for Cherry Pick to Stable Branch and removed Blocked by dependency Currently blocked by some other dependent / related change labels Dec 10, 2025
Copy link
Copy Markdown
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM

@mikehardy mikehardy requested a review from lukstbit December 10, 2025 13:58
@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Dec 10, 2025
@mikehardy
Copy link
Copy Markdown
Member

This one will be cherry-picked and I feel like I understand it but asking for second set of eyes from @lukstbit since they authored the original chunk and a second set of eyes is always good for a cherry pick to a stable release...

Copy link
Copy Markdown
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Much better than my original code because I completely missed this feature and I didn't account for it in any way:(

Things are still not working correctly when using the "Change deck depending on notetype" option. Changing notetypes while inside the editor erases the previously set deck and we revert to Default, desktop changes decks depending on selected notetypes while inside the add screen. This is also a regression from 2.22 where the proper decks were restored based on notetype selection.

Decide by notetype -> Go to add -> select a notetype and add to a deck -> select another notetype and add to another deck -> switch to the first used notetype -> deck is now Default

@lukstbit
Copy link
Copy Markdown
Member

lukstbit commented Dec 10, 2025

I'm fine with this going in, the issue I mentioned existed in the previous(2.23) code as well.

@lukstbit
Copy link
Copy Markdown
Member

Potential fix:

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditorFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditorFragment.kt
index a4141bfed8..40ba4fbd02 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditorFragment.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditorFragment.kt
@@ -2788,7 +2788,7 @@ class NoteEditorFragment :
 
         // Update deck
         if (!getColUnsafe.config.getBool(ConfigKey.Bool.ADDING_DEFAULTS_TO_CURRENT_DECK)) {
-            deckId = noteType.did
+            deckId = getColUnsafe.defaultsForAdding().deckId
         }
 
         refreshNoteData(FieldChangeType.changeFieldCount(shouldReplaceNewlines()))

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Dec 10, 2025
@david-allison
Copy link
Copy Markdown
Member Author

Drafting. Thanks!

I'd like to spend a little more time on correctness/testing before this goes in.

@david-allison david-allison marked this pull request as draft December 10, 2025 23:20
david-allison and others added 2 commits December 11, 2025 22:02
Fixes 19750

Co-authored-by: lukstbit <52494258+lukstbit@users.noreply.github.com>
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Dec 11, 2025
@david-allison david-allison marked this pull request as ready for review December 11, 2025 15:05
@david-allison
Copy link
Copy Markdown
Member Author

I have written two tests and applied @lukstbit's test, which fixes the second issue. Thank you for the thorough review!

Copy link
Copy Markdown
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

This still fixes all linked issues around decide by notetype, both the start and changing notetypes while inside the note editor screen.

Adding tests is even better, thanks!

@lukstbit lukstbit added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Dec 11, 2025
Copy link
Copy Markdown
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

yep - looks even better now

@mikehardy mikehardy added this pull request to the merge queue Dec 11, 2025
Merged via the queue into ankidroid:main with commit 22ac839 Dec 11, 2025
15 checks passed
@github-actions github-actions bot modified the milestones: 2.23.1 release, 2.23 release Dec 11, 2025
@github-actions github-actions bot removed Review High Priority Request for high priority review Needs Second Approval Has one approval, one more approval to merge labels Dec 11, 2025
@david-allison david-allison deleted the 19733 branch December 11, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] 'Change deck depending on notetype' not handled when switching note types "Deck for new cards/Decide by note type" is ignored

3 participants