Anki-Android
Anki-Android copied to clipboard
ankidroid
Fix crash on updating template when full sync is required
Purpose / Description
Prevent crash on updating template in case when full sync is required.
Fixes
Fixes #9688
Approach
The previous dialog mentioning that This will create %1$d card. Proceed? needs to be closed. If it remains open even after full sync dialog is displayed and then user clicks on "OK' again then it leads to a crash.
How Has This Been Tested?
Tested on Pixel 2 API 30 emulator.
Checklist
- [x] You have not changed whitespace unnecessarily (it makes diffs hard to read)
- [x] You have a descriptive commit message with a short title (first line, max 50 chars).
- [x] Your code follows the style of the project (e.g. never omit braces in
ifstatements) - [ ] You have commented your code, particularly in hard-to-understand areas
- [x] 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
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@david-allison-1 Can you help by telling how should I proceed with the unit test here?
@david-allison-1 Can you help by telling how should I proceed with the unit test here?
The first step is to determine which commit introduced the problem.
This can either be done via a test which triggers the bug, followed by git bisect, or manually.
I'm fairly certain that it'll be one of my "Rust" changes, in which case I'd be happy to revert the change, but I'd want regression cover to ensure that this bug doesn't occur in the future.
This is definitely a new bug in 2.16, and we should be fixing and investigating rather than working around the symptoms
To elaborate, I feel the problem is that the number of dialogs which are shown has changed, rather than this being a "dismiss context" bug
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
First bad commit f5ab2dd6dd3ad1400f0cc3b28239e10c53278e2c (got by git bisect)
Thankfully it is a dialog problem, not a Rust one. Does it still need a regression test?
Nice find @BrayanDSO !
Thankfully it is a dialog problem, not a Rust one. Does it still need a regression test?
So I looked at the commit here and the commit from the bisect, and I'm not sure I understand the dialog problem or how it leads to the crash. How exactly does the crash happen and how exactly does this fix it? If we thoroughly understand it then a test is not strictly necessary, however if it is possible to add a test that's always good when it's around crash issues or data issues so that we can release more confidently in the future
Got it. I'll try to take a look into it when I get the time. @ShridharGoel, if you want to keep with the PR, please let me know
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
Hi all, I had a look at the issue, and it's indeed a dialog problem. Actually, the situation is almost similar between adding and removing a card template, except from the changes originally proposed in this PR. I think the changes from this PR provides a valid solution, and we could make the situation even better by factoring the code between both cases. I've made a proposal with #11862, what do you think about it?
