Refactored DeckPickerWidget to handle empty state visibility.
#17041
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. |
xenonnn4w
force-pushed
the
update_deck_picker_widget
branch
from
d260808
to
195965a
Compare
|
Fixed! |
mikehardy
added
Needs Author Reply
…ing users to reconfigure the widget by clicking the empty state message.
xenonnn4w
force-pushed
the
update_deck_picker_widget
branch
from
8d767e1
to
c681094
Compare
xenonnn4w
added
Needs Review
and removed
Needs Author Reply
There was a problem hiding this comment.
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...
mikehardy
added
Needs Second Approval
| return@launch | ||
| } | ||
|
|
||
| showDeck(context, appWidgetManager, appWidgetId, remoteViews, deckIds) |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
lukstbit
added
the
Needs Author Reply
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.