-
-
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
Refactored DeckPickerWidget
to handle empty state visibility.
#17041
base: main
Are you sure you want to change the base?
Conversation
What does this look like? |
added in the PR description. |
I reviewed the code and noticed an issue with the visibility of empty_deck. I plan to investigate and resolve it when I have more time. |
d260808
to
195965a
Compare
Fixed! |
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.
I think this looks good - after looking through our back stack management though, I'm curious if we are missing a CLEAR_TOP on the Intent for the reviewer though
Otherwise (that is: the bulk of the PR): looks good
…ing users to reconfigure the widget by clicking the empty state message.
8d767e1
to
c681094
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.
Fantastic, I think this looks pretty good and from the video it looks pretty good to me
If you are curious at all - and I mention it because I have seen you do related work so I think you might be interested? - you might look at #17073 to consolidate our usage of these flags...
return@launch | ||
} | ||
|
||
showDeck(context, appWidgetManager, appWidgetId, remoteViews, deckIds) |
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.
At this point deckIds can't be null because you already checked above to show the empty state. This allows you to avoid the null checking in showDeck by declaring the parameter as non null.
appWidgetId: AppWidgetId, | ||
remoteViews: RemoteViews | ||
) { | ||
remoteViews.setViewVisibility(R.id.empty_widget, View.VISIBLE) |
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.
If we are using the empty TextView to show a message for when the collection is empty(by setting a specific text there) than we should also set the missing deck text here.
style="@style/TextAppearance.AppCompat.Medium" | ||
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:layout_marginStart="7dp" |
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.
As material design uses multiples of 4 for dimensions we should also use this for marginstart(8dp).
android:layout_marginStart="7dp" | ||
android:gravity="center" | ||
android:padding="20dp" | ||
android:text="@string/empty_widget" |
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.
The current text is "Missing deck. Please reconfigure". I think it may better to change the last part to add either Touch or Click to tell the user how he can act on the issue:
"Missing deck. Touch to reconfigure"
"Missing deck. Click to reconfigure"
appWidgetId: AppWidgetId, | ||
remoteViews: RemoteViews | ||
) { | ||
remoteViews.setTextViewText(R.id.empty_widget, context.getString(R.string.app_not_initialized_new)) |
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.
I've tested the no collection scenario and the user experience is not great here.
If I have a collection with one deck that I register in the widget and then go to the app and delete the deck, when I return to the widget I see the message: AnkiDroid is not initialized yet. Please open AnkiDroid and try again. The issues:
- text is misleading
- if I'm going to the app and come back I still see the message
- if I'm going to the app and add decks and come back I still see the message
- touching the widget does nothing
I have to long click the widget to bring the system configure button to do anything.
So I would recommend to create a special string for this case(example: Collection is empty, use app to add decks then reconfigure), when the collection is empty and also make that text clickable to show the configure screen.
Purpose / Description
Handle empty state visibility, allowing users to reconfigure the widget by clicking the empty state message in the Deck Picker Widget.
Approach
Handled similar to that Card Analysis Widget.
How Has This Been Tested?
dop.mp4
Checklist
Please, go through these checks before submitting the PR.