-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Show toast when importing without the app being started at least once #15692
Conversation
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
@@ -351,6 +351,9 @@ | |||
<string name="intro_get_started" comment="Action to start AnkiDroid for the first time without syncing from AnkiWeb">Get Started</string> | |||
<string name="intro_sync_from_ankiweb">Sync from AnkiWeb</string> | |||
|
|||
<!-- IntentHandler --> | |||
<string name="warning_start_app_before_import">Before importing app must be opened at least once for initialization</string> |
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 feels verbose:
"Please open Anki and try again"? Or similar
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.
code looks good to me as a way to handle the problem
I tried to take the strings suggestion to a prescriptive recommendation for an up/down vote so we can get this in quickly I hope
My approval is pending commit on those string proposals which I can do then squash-merge or can be done via rebase and pushed via lukstbit, either would work for me
If more discussion is needed because you all hate my suggestions (fair enough!) then I'll re-look after
@@ -351,6 +351,9 @@ | |||
<string name="intro_get_started" comment="Action to start AnkiDroid for the first time without syncing from AnkiWeb">Get Started</string> | |||
<string name="intro_sync_from_ankiweb">Sync from AnkiWeb</string> | |||
|
|||
<!-- IntentHandler --> | |||
<string name="warning_start_app_before_import">Before importing app must be opened at least once for initialization</string> |
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.
Using the new string name from first comment, then I prefer a "what" message for users, followed by "prescriptive action". So I'd say:
<string name="warning_start_app_before_import">Before importing app must be opened at least once for initialization</string> | |
<string name="app_not_initialized">AnkiDroid is not initialized yet. Please open AnkiDroid once, then try again</string> |
@david-allison would this work for you? If so and you're +1 on the rest of it, commit+squash these myself to get this in and queue up a 2.17.2 or obviously @lukstbit can (and also, obviously, if either of you disagree on my string-crafting let me know please!)
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.
Went with your suggestion, David's suggestion might sound a bit negative to the user(like some unknown error happened).
The current implementation using messages and handlers for async dialogs means that if the user tries to import before actually starting the app the import dialog ends up being shown in the introduction screen ending up in a crash. These changes prevent that by checking for this scenario and notifying the user that he must start the app at least once before any import can be done. The hasShownAppIntro() was moved to better structure the code(it's related to the introduction screen) and for ease of access in the screens that use it.
a0268a5
to
5bc445f
Compare
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.
Obviously works for me then since you just accepted all my changes 😆 - approving for avoidance of doubt, pending David saying yay/nay
I feel it's a little verbose still, but two votes to one 😁 Let's get this in |
Note to myself more than anything - needs a string sync afterwards, and that strings sync needs a queued for cherry-pick label |
We have a new flaky test after the intent handling changes, this popped up on another PR recently as well and caused merge queue ejection
|
Purpose / Description
Fixes the linked issues by notifying the user through a toast(suggestions for the notice message of course welcome) and aborting the import that he must start the app at least once before he can import. At first I tried to be more precise in DeckPicker to try to remove the import dialog message from the handler and then show the toast, this didn't worked at all so I moved the check in IntentHandler.
Fixes
How Has This Been Tested?
Ran the tests(but hit some OutOfMemoryErrors) and manually installed debug apks(play and full) to first reproduce and then check the patch.
Learning (optional, can help others)
I dislike the handler system used for async dialogs.
Checklist