-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(note-editor): retain note type when copying and preserve field data #20030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(note-editor): retain note type when copying and preserve field data #20030
Conversation
|
This looks wrong. How did "copy" work previously, as it works if you don't change the note type back and forth |
| val fiveFieldNoteType = col.notetypes.byName(fiveFieldNoteTypeName)!! | ||
| val basicNoteType = col.notetypes.basic | ||
|
|
||
| withNoteEditorAdding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the bug I listed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still isn't the bug I listed, could you also add a test for the bug I provided.
This test is fine if there's an additional bug while adding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test checks for field text retention in the add mode by switching from 3 fields -> 2 fields -> 3 fields. I have verified that:
When switching 3 -> 2, the first two fields are preserved.
When switching back 2 -> 3, the third field is restored from memory: assertThat("Field ${i + 1} should be restored from memory", fields[i], equalTo(text))
Since the bug report mentions the final field being blanked during these steps, this test was designed to catch that exact bug. I might be missing something small in how the bug was reported, could you clarify if you're looking for a specific test case for the edit mode as well even if the failure is in the add mode only?
edit: I’ve confirmed this test passes with my fix and fails on the current codebase
4daffb5 to
815f127
Compare
|
Thanks for the feedback. I have changed the caching logic as it was needlessly complex. I also changed the 5 field note type test to 3 field as you originally listed. I also thought of doing note type change the same way is done in EditNoteTypeListener but that would require a lot of work as in the "add mode" the data is only stored in the UI and there is no source of truth as there is in the "edit mode", at last I decided on using a simple cache which will be used to rewrite the lost fields as it is the easiest fix and will not increase the complexity much. I'm happy to adjust if there's a better approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still looks wrong. The cache shouldn't be necessary
How did "copy" work previously, as it works if you don't change the note type back and forth?
|
The cache isn't used for the "copy note type bug". It fixes the field blanking bug |
|
The bug doesn't occur when in 'change note type' mode, field data is retained. I suspect the same solution would work here. If there is a bug, add a test |
815f127 to
d77f0f7
Compare
|
I have removed the cache now. The field blanking was caused because in the add mode, (1)editorNote is not synced with the UI when note type is changed and (2) when the note type is actually changed all the fields in the editorNote are never copied. |
d77f0f7 to
4caf1aa
Compare
Co-authored-by: Aryan171 [email protected] Co-authored-by: David Allison [email protected] source: card type retention when copying ankidroid#19384
4caf1aa to
5cf2848
Compare
Purpose / Description
This PR ensures that the Note Type is correctly retained when copying a note in the Note Editor. Additionally, this PR addresses the field blanking bug where fields were blanked when changing to a note type with less number of fields and then changing back to the original note type
Fixes
Approach
This implementation incorporates the architectural refactor provided by david-allison via a patch in the previously closed pr #19384. To solve the field blanking bug, I implemented a caching mechanism that preserves field content during Note Type transitions.
How Has This Been Tested?
Unit Tests:
copyNoteCopiesNoteType: Verifies that the new note retains the original note's type.
changing note type preserves and restores field contents: Verifies that switching to a 3-field Note Type and back to a 5-field Note Type does not clear the 3rd, 4th and 5thfield's text.Manual Verification done on android 15 phone
Screen recordings
https://github.com/user-attachments/assets/f3c317b1-d2d9-47df-b61b-7e8b939164db
https://github.com/user-attachments/assets/85310e7f-8c31-459c-8e81-0e25862bf00e
Checklist
Please, go through these checks before submitting the PR.