Anki-Android
Anki-Android copied to clipboard
build(deps): update to ACRA 5.9.x series
Pull Request template
Purpose / Description
Update to current ACRA versions
@jimrod942 got us most of the way there with #9969
This finishes it with some Kotlin-migration changes related to upstream switching to Kotlin and re-organizing how they did config
Fixes
Fixes #7805
Approach
- Use kotlin-style variable references for 6 of the compile errors (trivial, these ones)
- Proposed patch upstream to open the method we used to override in Java - marking this draft until released in ACRA 5.9.x
- Proposed patch upstream to make default exception handler nullable
- Lots of configuration style changes as upstream config style changed again
How Has This Been Tested?
Follow the ACRA testing guidelines in our wiki - https://github.com/ankidroid/Anki-Android/wiki/Crash-Reports#steps-to-test - I just exercised those (and cleaned them up) recently so they should work well
Learning (optional, can help others)
- Kotlin nullable Java interop is finicky https://github.com/ACRA/acra/pull/1053
- Kotlin allows configuration things that seem easy in Kotlin but suck when done in Java :sweat_smile: https://github.com/ACRA/acra/issues/1048
- Kotlin specifies extensibility more precisely then Java if I understand this correctly https://github.com/ACRA/acra/pull/1033
- So you can have "protected" and "final", or "protected" and "open", we need the new "protected open" combo here
Checklist
Please, go through these checks before submitting the PR.
- [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
if
statements) - [x] 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
Upstream merged, will be released in ACRA 5.9, so this will go to sleep for a little bit, but has a bright future...
Something else appears to have changed between 5.8.x and 5.9.x because when I posted my PR, I had this working with the PR's change to ACRA applied locally, and now it doesn't work on the actual release version. Will inspect later.
~~Tracking upstream https://github.com/ACRA/acra/issues/1048~~ config changed mightily, lots more work to do here now
Still working through this one from time to time. Found another item from new releases - apparently robolectric does not set a default uncaught exception handler, and that's acceptable as Thread.defaultUncaughtExceptionHandler is nullable per android docs
But ACRA was expecting one, upstream PR here, hopefully in a future release, maybe 5.9.2 https://github.com/ACRA/acra/pull/1053
Okay, this is ready for review but it won't pass testing until 5.9.2 from ACRA with a fix for null uncaughtExceptionHandler The code here won't change a bit though
Once this is in, a separate task should be to refactor this and pull ACRA config out to a separate object That wasn't possible before because of the annotation style of config, but now that's no longer a factor. Should be relatively easy New config object should be in Kotlin, then configuration gets a bit easier I think
And unit test issues
Now that upstream 5.9.2 is out, let me rework this one, and try to chunk the commits up much better It does pass testing when you have the result of my PR for ACRA in there - without it, it will fail but I wasn't going to commit my temporary patch file in their package directory that I was using to develop the PR for them
The commits should be a lot easier to follow now, they take a little refactoring journey from A (pre-existing Java style config) to B (new more Kotlin-y style config).
The basic point is that you can't easily reach in and modify live configs anymore, you need to re-config the whole system now to change things. So I localized the state and re-gen the config each time. Seems to work
Awesome!
Really minor change requested for performance on the startup path.
Everything else can be ignored or moved to a new issue - implementer's choice. Happy to see this go as soon as the minor change is made.
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
Okay, it's rebased now, but: ~~1- the dialog language is always system language, it does not take the app language pref~~ 2- the toast language takes that app language but does not correctly change it -- will peel to new issue ~~3- the setting to never report is not working, if you trigger a test crash on a debug build (which is "never report" by default) it will trigger a toast about collecting info then show the dialog - it should not~~ https://github.com/ACRA/acra/issues/1113
So, the rebase wasn't too bad but the functionality isn't all there yet
I think I've addressed all the stuff, with some useful-for-future people guidance for future folks on the peel-out of a new issue for dynamic language support
Finally ready! Except dynamic language switching during same app run. Does take dynamic language after app restarts.
Just added a final commit final commit that should correctly show the app version number in Acrarium, instead of the library version number. That's going to be a huge help during the beta cycle.
Also changed our init style just a bit to work around a bug I found in ACRA in a way that works with current ACRA release and the pending/unreleased bugfix
I'll leave this out for a little while hoping for review but I want this in quickly so will merge by default in a few days if not. Always happy to take issues or work on PRs in the future though in case review is post-merge, it's still useful...
I've peeled the two possible cleanups off for follow-on issues and pushed a cleaner version of our usage of the limiter data to throttle exceptions, those were the only items from first review.
Ready for merge pending second review
going to merge, I want to clean out second review queue and get a release out, then focus on PMing of scoped storage with time available, but I need this crash reporting fix in so we can start focusing on acrarium with purpose (and with good version numbers)
Hi there @mikehardy! This is the OpenCollective Notice for PRs merged from 2022-10-01 through 2022-10-31
If you are interested in compensation for this work, the process with details is here:
https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid
We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month
Please note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself.
Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.
Thanks!