Implement onboarding for new users with feature tutorials #9092
Conversation
There was a problem hiding this comment.
Had a quick scan:
We should be using .xml drawables, not .png images
- AnkiDroid crashes after the tutorial opens
2021-06-09 23:25:37.261 21086-21086/com.ichi2.anki E/AndroidRuntime: FATAL EXCEPTION: main
Process: com.ichi2.anki, PID: 21086
java.lang.RuntimeException: Unable to resume activity {com.ichi2.anki/com.ichi2.anki.DeckPicker}: java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.View android.view.View.findViewById(int)' on a null object reference
at android.app.ActivityThread.performResumeActivity(ActivityThread.java:4438)
at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:4470)
at android.app.servertransaction.ResumeActivityItem.execute(ResumeActivityItem.java:52)
at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176)
at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:223)
at android.app.ActivityThread.main(ActivityThread.java:7660)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.View android.view.View.findViewById(int)' on a null object reference
at com.ichi2.anki.DeckPicker.showTutorialForDeck(DeckPicker.java:1007)
at com.ichi2.anki.DeckPicker.startDeckAndCountsTutorial(DeckPicker.java:998)
at com.ichi2.anki.DeckPicker.onResume(DeckPicker.java:992)
at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1456)
at android.app.Activity.performResume(Activity.java:8135)
at android.app.ActivityThread.performResumeActivity(ActivityThread.java:4428)
at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:4470)
at android.app.servertransaction.ResumeActivityItem.execute(ResumeActivityItem.java:52)
at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176)
at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:223)
at android.app.ActivityThread.main(ActivityThread.java:7660)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
- The animation for the note editor is distracting, the fields aren't highlighted (no transparency, a user can't see what's being highlighted), and there's no visible prompt to remove the suggestion
I think overall this PR needs more screenshots, and there'll be a lot of discussion on the content of the onboarding. I think our explanations need to be much more targeted at where our users are having problems, rather than explaining obvious (to me) concepts
| @@ -434,6 +441,45 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| } | |||
|
|
|||
| startLoadingCollection(); | |||
|
|
|||
| mSharedPreferences = getSharedPreferences("PERSISTENT_STATE_FILE", 0); | |||
There was a problem hiding this comment.
is there a reason that you're not using AnkiDroidApp.getSharedPreferences?
There was a problem hiding this comment.
I thought that AnkiDroidApp.getSharedPreferences() is only being used for settings related preferences which can be changed by the user.
There was a problem hiding this comment.
It might be good to document when to use one or the other. I personally don't know
There was a problem hiding this comment.
If there is nothing defined about this, then I'll change the implementation to use AnkiDroidApp.getSharedPreferences().
There was a problem hiding this comment.
Hmm - we've always just used a single preferences file (the default one) as far as I know. I believe there is one secondary one related to some module we include (dim memory - I don't think it's ACRA, I think even those keys are in the default one). Anyway, I don't see a reason to split them up, not even with what I know of performance in the preferences system on android, it will load them all up and have them cached anyway, but it's not high traffic or anything so no worries about file write contention on prefs.apply() etc. Just use the default I think
There was a problem hiding this comment.
. I believe there is one secondary one related to some module we include
I think the other one was used by CardBrowser
|
reason your unit tests fails is you need to add intro screen to |
Thanks, I had not checked the reason of the unit tests failure yet. I will update this. |
@david-allison-1 Can you give some examples? |
|
You should create an issues for discussion and ideas. You may take some ideas from here about onboarding. The target audiences are new users. So, When user install the app, then app should tell them exactly what AnkiDroid will do for them in 3-5 onboarding images. There are some of my ideas.
The walkthrough of the app is good. But attach screenshots because it is UI/UX related PR. |
There was a problem hiding this comment.
I'm effraied of the idea of saving one preference entry by boolean. That will be very big quite quickly. I believe we should use a long to save up to 64 boolean flags to indicate which onboarding screen has already been seen or not.
Thanks a lot for this first PR.
I think we may also have to discuss editing a commit instead of pushing new one
| setTransformer(AppIntroPageTransformerType.Zoom) | ||
|
|
||
| addSlide( | ||
| AppIntroFragment.newInstance( |
There was a problem hiding this comment.
I'd like you to extract the AppIntroFragment outside of the onCreate.
If they got assigned to variables, then we can easily try to change the order, or to restrict to only a subset of those, by just changing the variable order instead of doing copy/paste of whole 5 lines
| @@ -434,6 +441,45 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| } | |||
|
|
|||
| startLoadingCollection(); | |||
|
|
|||
| mSharedPreferences = getSharedPreferences("PERSISTENT_STATE_FILE", 0); | |||
There was a problem hiding this comment.
It might be good to document when to use one or the other. I personally don't know
|
Copying from Discord: To me, for onboarding, the most important thing to me is refinement of the current design, rather than expanding to other screens: right from the get-go, we're going to lose a lot of users (via "skip"), and we should spend more time focusing on the best way to provide information in the shortest amount of time I think the onboarding process is going to get a lot of bike shedding. I normally shy away from this, but it needs to be done here, along with user testing. I think our main screen should have a few goals:
If we do the last point right, then we can cut down the list significantly. I don't think we need to showcase features, it slows down getting users into the app, we want people downloading decks/adding as soon as possible. My main ideal on moving this forward would be more screenshots/animations as mentioned in the initial review. In terms of onboarding and a tutorial, I wrote by overall thoughts on Reddit a while ago, which might be worth thinking about. I think a lot of the videos suggesting addons or custom settings do more harm than good, and we want the new user process to be as simple as possible. I'd say that a significant percentage of people customizing the options are doing it wrong, and causing themselves harm. https://old.reddit.com/r/Anki/comments/mn3v30/new_to_anki_didnt_see_a_post_for_newbs/gtvm1du/ |
ShridharGoel
force-pushed
the
onboarding
branch
2 times, most recently
from
a36bbc7 to
73e074b
Compare
@infinyte7 Thanks for the suggestions. Do you also have a suggestion about where should we show such things which might be a little advanced for the new user? For example, should we show the tip about |
ShridharGoel
force-pushed
the
onboarding
branch
3 times, most recently
from
d1b47eb to
7c251bb
Compare
|
There are two parts
Onboarding can be used show the important features that does not needs demo to show. Like the feature Walkthrough of advanced features like |
AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.java
Outdated
Show resolved
Hide resolved
|
I think the main concepts will be again/hard/good/easy/graduation/lapses/"deck completed: where are my cards"/"why can't I study when I want to" |
There was a problem hiding this comment.
I think it would be nice to wait for commands to be available before showing them.
That is:
- wait until there is actually a card to review to let the user know they should click on a deck name
- Wait until there is an action to undo to present the arrow
Also:
"Enter the question part of card in front" (instead of saying "the front part")
The trouble of course being that "front" and "back" has only meaning on paper card. As we follow upstream, we can't really change it. Even if I'd argue that Anki should change it too in the basic note type.
I'd even argue that it would be nice to have similar description for "Basic and reverse" and for all basic note type (that's not trivial at all to detect, but can probably be done using just note type name as a heuristic)
| object OnboardingUtils { | ||
|
|
||
| const val ONBOARDING = "Onboarding" | ||
| const val DEFAULT_VALUE: Long = Long.MAX_VALUE |
There was a problem hiding this comment.
Classically, bit set are initialized at 0.
Also, you should use unsigned long. The max value of long is 2^63-1, and its binary representation is 01111111.11111111.11111111.11111111.11111111.11111111.11111111, so there is a zero, which is not ideal. With unsigned values, the problem would not exists.
I suggest using a BitSet instead. And indicating explicitly indicate that a bit is set when the onboarding screen is seen.
| } | ||
|
|
||
| /* | ||
| Meaning of each index: |
There was a problem hiding this comment.
It's generally a bid idea to use literals directly. We should instead define a constant, so that there is no need to double check the numerical values.
Actually, we should use an Enum. Let's call it "ReviewerOnboardingScreen". Then, instead of using a index: int, you can take as argument an element of the enumeration elt. And use its ordinal
Ideally, it would also be nice if we could have a linter that ensure that such an enum has at most 64 elements (otherwise, they can't be represented as long)
It would also be nice to have another linter rule that ensure that no elements are removed or reordered. I don't think linter can compare to git commits, but what could be done instead is that each time we publish a new stable version, we hard code the content already present in lint. So that we can change the elements in alpha, and even beta, but not once its stable
| return super.onCreateOptionsMenu(menu); | ||
| } | ||
|
|
||
| public void showTutorialForFlag() { |
There was a problem hiding this comment.
I still suggest a helper function that takes a builder and an index, and that show the builder result and set as visited this index. This way, there is some small code duplication that can be avoided. Even if most of the code remains unchanged
There was a problem hiding this comment.
This is still pending,
@Arthur-Milchior this might need elaboration on the design
There was a problem hiding this comment.
@david-allison-1 We are already using a custom builder now, is anything else also required? I had created a method earlier, but during review you had mentioned that the method decreases the readability and hence we should shift that code to a custom builder class.
There was a problem hiding this comment.
This is clearly not what I had in mind. I guess that using inheritance instead of delegation works. I fear it might not work anymore if we want to use another kind of builder in the future however
There was a problem hiding this comment.
@Arthur-Milchior In that case, can you tell what is expected here?
There was a problem hiding this comment.
Oh, what I was trying to state is that this implementation is right. I believe that if needed, it can be reworked to be more general later, but right now it's quite good enough
|
|
||
| object OnboardingUtils { | ||
|
|
||
| const val ONBOARDING = "Onboarding" |
There was a problem hiding this comment.
I think it would be nice to have one entry by activity. One for reviewer, one for deck picker, etc...
If we store 10 longs instead of one, it's not a trouble, and we will then be able to have more than 64 onboarding information. While I know that 64 onboarding slides seems a lot, I actually think that it is not that much if we consider:
- the number of buttons
- we may want to deprecate some slide one day if a feature change too much
- we may add new features
- some slides won't be shown immediately but only after some time
ShridharGoel
force-pushed
the
onboarding
branch
2 times, most recently
from
fbdfb3e to
38253ab
Compare
There was a problem hiding this comment.
Thanks a lot. I sincerely appreciate how nicely the code evolves with each commits.
Were there any difficulties? Do you know what took you time? I understand it's not easy, but I was hoping to see more regular evolution of the PR, so that I can give feedback quickly and end faster the code review.
I've not yet concentrated on the actual content of the onboarding. You know it by know, I care a lot about having a nice library first
| * If the bit at an index is set, then the corresponding tutorial has been seen. | ||
| */ | ||
| fun checkIfNotAlreadyVisited(index: Int, context: Context, prefKey: String): Boolean { | ||
| return (AnkiDroidApp.getSharedPrefs(context).getLong(prefKey, 0) and (1L shl index)) == 0L |
There was a problem hiding this comment.
Can you please also use BitSet here.
There was a problem hiding this comment.
@Arthur-Milchior are we really using a bitset to flag whether onboarding has shown something or not? Isn't that a fixed size thing? How would that map to an API where future features could implement a "walkthrough" hooking into this system and this system would know whether it was shown or not? That's a design I I mentioned recently in Discord, idea taken from VSCode https://code.visualstudio.com/updates/v1_57#_new-getting-started-experience
Here's a great link to the VSCode team's experience with users when they just did the same thing we are doing: microsoft/vscode#106717 (comment)
Key findings:
- users skip because they're really focused on a specific task
- users then are maybe interested later but can't get back to the "getting started" content
Here's how they define a "walkthrough" contributed to the "getting started" content microsoft/vscode#116414 (comment)
|
|
||
| package com.ichi2.anki | ||
|
|
||
| enum class CardBrowserOnboardingEnum { |
There was a problem hiding this comment.
Is it standard in Kotlin to move enum in their own class? I'd have considered that such an enum could be in CardBrowser. And similarly for other enum of onboarding
| @@ -0,0 +1,55 @@ | |||
| package com.ichi2.utils; | |||
|
|
|||
There was a problem hiding this comment.
Please add a header. EIther with your copyright if you wrote it, or with the source if you were inspired by other (I'm surprised that you need to create this class yourself. I would have assumed that it is standard to focus this way. Can you please add in javadoc the difference between DimmedCirciclePromptBackground and what already exists. I.e. CirclePromptBackground and maybe other classes you found while exploring your options)
There was a problem hiding this comment.
DimmedCirclePromptBackground and DimmedRectanglePromptBackground have actually been taken from an example mentioned in the library documentation. So what should be done in this case?
There was a problem hiding this comment.
I assume the library documentation has a license and/or copyright? If it's compatible we should include it, if not, I'm not sure what to do except re-implement ? That seems silly though, hopefully it's compatible
I was mainly waiting for feedback. I had done the required changes in the starting of the week and mentioned on Discord for reviews. Recieved feedback on Friday and then updated with things like there use of enum classes, having separate preferences for each screen, use of BitSet etc. |
Enums are already being used. There was difficulty in passing an enum class directly as parameter so I used the |
ShridharGoel
force-pushed
the
onboarding
branch
3 times, most recently
from
6567e61 to
257806e
Compare
There was a problem hiding this comment.
I'm focusing on the in-context hints with this one.
Design: To discuss with other mentors:
I feel we want to move away from having onboarding related code in the activity itself, and we should extract this to a separate concern
private Onboarding.AbstractFlashcardViewer mOnboarding = new Onboarding.AbstractFlashcardViewer(this);
...
mOnboarding.onCreate()
mOnboarding.onQuestionShown()
mOnboarding.onAnswerShown()Once we're inside the concern, it becomes more easy to:
- Exit early, if a method doesn't need to execute, as there's less unrelated context
- Define an order of transitions and move between them, possibly via data classes
AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.java
Outdated
Show resolved
Hide resolved
| * If the bit at an index is set, then the corresponding tutorial has been seen. | ||
| */ | ||
| fun <T : Enum<T>> checkIfNotAlreadyVisited(enum: Enum<T>, context: Context): Boolean { | ||
| return (AnkiDroidApp.getSharedPrefs(context).getLong(enum.name, 0) and (1L shl enum.ordinal)) == 0L |
There was a problem hiding this comment.
Don't use enum ordinals, if someone removes an element (or reorders the elements into alphabetical order), then it corrupts the preference
There was a problem hiding this comment.
We can create a method inside the enum which would be giving specific values for specific entries in the enum. But, since we are just passing a generic enum as the parameter, how can we access methods from the enums without type casting the enum parameter?
There was a problem hiding this comment.
I don't know how you'd do it in Kotlin, In Java: Add in a constuctor to the enum accepting an integer, and use a fixed value.
There was a problem hiding this comment.
Yes, that's the way but as I mentioned in the previous comment, we are passing a generic enum instead of using the exact enum classes. So in that case, how can we fetch the integer value?
There was a problem hiding this comment.
Enums seem to be able to implement interfaces in Java
There was a problem hiding this comment.
In this card, I was suggesting ordinals. I was also indicating that we should ensure that we never reorder or delete any element appart during alphas maybe. Ensuring that between each successive main version, elements are only added. This keep the code quite simpler, is not hard to enforce (should be indicated in a comment for safety). There is almost the same risk with current method of adding an explicit number and just add a lot of code duplication
There was a problem hiding this comment.
I strongly disagree. I've been stung by this in the past, the javadocs imply that we shouldn't be using .ordinal().
People refactor without thinking about the surrounding context. A bug which can be introduced thoughtlessly by a mass refactoring, and causes subtle data corruption with no defined recovery procedure isn't anywhere near the problem of some code duplication.
| return super.onCreateOptionsMenu(menu); | ||
| } | ||
|
|
||
| public void showTutorialForFlag() { |
For this, you can clear the app data or edit the code to return |
Yes, would it be fine to do it in a separate PR once this is merged and the one related to introduction slides is also complete? |
|
I'm not asking you to pull my commit. Just work the way you prefer. If you prefer to ignore it because I missed an issue, that's fine. If you want to read it and use what you see to change some commit, that's fine. If you want to copy some part of it into a new commit, cool too. I care about the feature and the review, not the way the commit was done |
There was a problem hiding this comment.
Some small nit. Approval once corrected. @david-allison-1 or @mikehardy I think there are many changes since last approval and so require a new second approval
| @@ -1840,6 +1844,8 @@ protected void displayAnswerBottomBar() { | |||
| mFlipCardLayout.setAlpha(1); | |||
| mFlipCardLayout.animate().alpha(0).setDuration(mShortAnimDuration).withEndAction(after); | |||
| } | |||
|
|
|||
| new Onboarding.AbstractFlashcardViewer(this).onAnswerShown(); | |||
There was a problem hiding this comment.
shouldn't this be a variable, or we lose state between the calls
There was a problem hiding this comment.
@Arthur-Milchior wanted this to not be a variable: #9092 (comment)
So I had changed it from a variable to use it like this. What should be done here?
There was a problem hiding this comment.
Which state do we lose ? The only state AbstractFlashcardViewer is this, the activity.
The only state of its parent class, Onboarding, is this and also the list of onboarding tasks, which is never edited anyway.
There was a problem hiding this comment.
Any state in the instance. I didn't check to see whether we have any.
Regardless, the pattern of having a private final variable is much more common than having an allocation for a single method call.
To me, the code would read better that way (especially as we define the dependency on the class at the top of the file in the variables), but this is just style and isn't worth blocking the review over. I'd push lightly to change, but implementer's choice.
There was a problem hiding this comment.
I agree on this. Implementer choice.
| @@ -666,6 +666,10 @@ public boolean onCreateOptionsMenu(Menu menu) { | |||
| undoIcon.setTitle(getResources().getString(R.string.studyoptions_congrats_undo, getCol().undoName(getResources()))); | |||
| } | |||
|
|
|||
| if (undoEnabled) { | |||
| new Onboarding.Reviewer(this).onUndoButtonEnabled(); | |||
There was a problem hiding this comment.
@Arthur-Milchior wanted this to not be a variable: #9092 (comment)
So I had changed it from a variable to use it like this. What should be done here?
There was a problem hiding this comment.
Your call. See: #9092 (comment)
| @@ -0,0 +1,21 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <string name="fab_tutorial_title">Get started</string> | |||
There was a problem hiding this comment.
For discussion:
While we're working on content, I feel that a few of these should be hardcoded (maybe in a companion object) so we're not adding work for our translators that won't be used in the final project
There was a problem hiding this comment.
That's uselessly complex. Just remove this file from tools/update-localizations.py; and wait until the content is finally decided.
Do you have a plan to choose which will be the final content?
There was a problem hiding this comment.
Discussed on Discord, only fab_tutorial_title for certain
david-allison
dismissed
their stale review
I like this, Arthur asked for a second round of reviews
ShridharGoel
force-pushed
the
onboarding
branch
from
f844f88 to
6dad1c9
Compare
ShridharGoel
force-pushed
the
onboarding
branch
from
6dad1c9 to
c93ad55
Compare
| @@ -0,0 +1,21 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <string name="fab_tutorial_title">Get started</string> | |||
There was a problem hiding this comment.
That's uselessly complex. Just remove this file from tools/update-localizations.py; and wait until the content is finally decided.
Do you have a plan to choose which will be the final content?
ShridharGoel
force-pushed
the
onboarding
branch
2 times, most recently
from
d3b03d2 to
4c37995
Compare
Handler is required to show feature prompt in menu items which is useful for flag option and undo option in Reviewer. Also, it is required in the case of CardBrowser where the feature tutorial has been made without using a library and it needs to be removed after a particular time interval.
ShridharGoel
force-pushed
the
onboarding
branch
from
4c37995 to
b599602
Compare
| @@ -0,0 +1,21 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <string name="fab_tutorial_title">Get started</string> | |||
There was a problem hiding this comment.
Discussed on Discord, only fab_tutorial_title for certain
david-allison
added
Pending Merge
|
Pending CI, doesn't seem to have run. @mikehardy, what was your mechanism for re-running checks? I'd typically close and open the PR, but the "official" method would be better for other peoples PRs |
|
If they did not run at all a close reopen might be only way, I wasn't aware of another way besides re-pushing |
david-allison
removed
the
Pending Merge
|
Another one down! Thanks so much @ShridharGoel |
Purpose / Description
Implement onboarding for new users.
Video
Onboarding.mp4
Checklist
ifstatements)