Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

build(deps): update to ACRA 5.9.x series

Open mikehardy opened this issue 2 years ago • 12 comments

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

mikehardy avatar Mar 25 '22 16:03 mikehardy

Upstream merged, will be released in ACRA 5.9, so this will go to sleep for a little bit, but has a bright future...

mikehardy avatar Mar 25 '22 18:03 mikehardy

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.

mikehardy avatar Apr 11 '22 13:04 mikehardy

~~Tracking upstream https://github.com/ACRA/acra/issues/1048~~ config changed mightily, lots more work to do here now

mikehardy avatar Apr 11 '22 14:04 mikehardy

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

mikehardy avatar Apr 20 '22 20:04 mikehardy

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

mikehardy avatar Apr 20 '22 21:04 mikehardy

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

mikehardy avatar Apr 20 '22 21:04 mikehardy

And unit test issues

david-allison avatar Apr 22 '22 23:04 david-allison

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

mikehardy avatar Apr 22 '22 23:04 mikehardy

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

mikehardy avatar Apr 28 '22 03:04 mikehardy

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.

david-allison avatar Apr 28 '22 15:04 david-allison

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 Jun 27 '22 17:06 github-actions[bot]

Ill-be-back

mikehardy avatar Jun 27 '22 22:06 mikehardy

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

mikehardy avatar Oct 09 '22 23:10 mikehardy

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

mikehardy avatar Oct 11 '22 00:10 mikehardy

Finally ready! Except dynamic language switching during same app run. Does take dynamic language after app restarts.

mikehardy avatar Oct 11 '22 00:10 mikehardy

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...

mikehardy avatar Oct 12 '22 04:10 mikehardy

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

mikehardy avatar Oct 12 '22 23:10 mikehardy

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)

mikehardy avatar Oct 13 '22 12:10 mikehardy

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!

github-actions[bot] avatar Nov 01 '22 16:11 github-actions[bot]