Skip to content
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

fix: handle database-related errors / import in AnkiActivity #17512

Merged

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Nov 27, 2024

Purpose / Description

this removes all the casting to DeckPicker that causes crashes at the worst possible time (like when you lost collection permission and the PermissionActivity is not a DeckPicker type)

Fixes

Approach

We've got hierarchy problems, and the problem is that we have multiple sub-types that think they can blindly refer to things as DeckPicker through various mechanisms.

But they can't, and they crash.

So I've moved the things that were crashing on up in the hierarchy so that all the types had them, and hopefully they won't crash.

By commits:

1- sendExceptionReport in our helper method for showing errors to users, even if the dialog show failed (non-controversial? but perhaps using showError is controversial and we just want a direct sendExceptionReport)

2- the main fix: move all the DialogHandler.asyncMessage / import-related stuff up to AnkiActivity so now PermissionsActivity can safely host it

3- add a dev preference to set your database path to pre-scoped storage. This lets you trigger the PermissionsActivity dialog fragment which let's you choose restore from backup which lets you verify it still works in this context. Unfortunately it isn't possible to test all the permissions fragments (like the PermanentlyRevoked one...) without manual hackery in testing

4- quiet a couple error messages that weren't really errors. Can drop this, but I was in there and it seemed like the right thing to do so tacked it on

How Has This Been Tested?

After this change, I went to the DeckPicker and I exported a package, then I imported a package, and it worked, both .apkg and colpkg types

A more involved test - use the new "set collection path" dev option to set my collection to the old path - re-open the app and get the permissions dialog - then try to restore from backup, it works

Learning (optional, can help others)

I have clearly learned nothing about clean architecture 😆 but I'm laser-focused on not crashing right now.

Checklist

Please, go through these checks before submitting the PR.

  • 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

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I don't know here. It's a big change for a patch release. I'd want to see it sit in beta for a little while

Implementation-wise: I feel this makes sense. A user tapping a dialog should be moved to the main screen, if this can't occur, the dialog should still be functional

  • Rename ankiActivity -> activity in param names
  • If something went wrong, also send an exception report

I'd approve after the nits are picked

@mikehardy
Copy link
Member Author

I don't know here. It's a big change for a patch release. I'd want to see it sit in beta for a little while

The decoded statement here is, are we sure this all works? That is, it needs more testing, right? I think a request for directed testing vs a mild hope that beta users will magically check everything is more valid. And I can do that

It's a lot of diff but conceptually - given type safety etc - it isn't that big a move IMHO. Only distasteful because "more stuff in AnkiActivity"...

@mikehardy mikehardy force-pushed the database-import-moved-to-anki-activity branch from 5109273 to 264de93 Compare November 28, 2024 15:13
@mikehardy
Copy link
Member Author

I've added 3 commits nestled around the "real" fix, and implemented the nits - updated the description in Approach to describe same

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

One nit I feel is blocking (naming the key). The rest looks great. Pre-approved in the labels given that they're all nits

Great catch on the BadWindowToken fix!

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Nov 29, 2024
previously we would ignore our sendExceptionReport param if a dialog
post fails, meaning that we would not get exception reports from ACRA
in circumstances we really want them
this patch has been floating around a lot, and it is very useful
you can use it to test the permanently inaccessible storage dialog,
which you can then also use to test the restore from backup dialog
…om them

so we do not need the full stack trace and they shouldn't be warn
@mikehardy mikehardy force-pushed the database-import-moved-to-anki-activity branch from 3853a91 to 36851ed Compare November 29, 2024 13:11
@mikehardy mikehardy removed the Needs Author Reply Waiting for a reply from the original author label Nov 29, 2024
@mikehardy mikehardy dismissed david-allison’s stale review November 29, 2024 13:12

fixed the 2 nits possible - stated as pre-approved given those + appropriate tags

this removes all the casting to DeckPicker that causes crashes at
the worst possible time (like when you lost collection permission
and the PermissionActivity is not a DeckPicker type)
@mikehardy
Copy link
Member Author

I wanna do 2.19.3 basically immediately to get it out for a soak with limited distribution so I'm going to accept responsibility and ask for apologies if there is a need for any follow-ons, but here we go

@mikehardy mikehardy added this pull request to the merge queue Nov 29, 2024
Merged via the queue into ankidroid:main with commit d0acd30 Nov 29, 2024
9 checks passed
@github-actions github-actions bot removed the Review High Priority Request for high priority review label Nov 29, 2024
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Nov 29, 2024
@github-actions github-actions bot modified the milestones: 2.19.3 release, 2.20 Release Nov 29, 2024
@mikehardy mikehardy deleted the database-import-moved-to-anki-activity branch November 29, 2024 17:03
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.

PermissionsUntil29Fragment: PermissionsActivity cannot be cast to com.ichi2.anki.DeckPicker
2 participants