Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

Update FAB UI for more intuitive double tapping

Open oyeraghib opened this issue 2 years ago • 10 comments

Purpose / Description

Updating FAB UI for more intuitive double tapping

Fixes

Fixes #11121

Approach

  1. Removed all instances of mAddNoteLayout as we don't need that view anymore. We will handle that functionality inside the FAB.

  2. Create new variables for drawable images to show in FAB (one for showing add icon and one for showing add note icon

  3. Create animations for FAB when it is pressed to show Add Note Icon and changed it's background color. Also when FAB is closed it's icon will change back from Add Note to Add

  4. Use attributes to set the background color of FAB depending upon different themes

  5. Create a new function to handle the FAB animations in case a user selects Add Decks or Get Shared Deck

  6. Removes instances of Add note layout from the DeckPickerFloatingActionMenuTest so unit tests don't fail

How Has This Been Tested?

Manually tested on my Physical Device

  • Before

https://user-images.githubusercontent.com/40427461/187999753-8305ecad-b309-41ac-b93b-4cbcf0632bad.mp4

  • After

https://user-images.githubusercontent.com/40427461/187999843-d8db89da-e046-409c-bb64-bb94ae1d6095.mp4

Checklist

Please, go through these checks before submitting the PR.

  • [x] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [x] You have commented your code, particularly in hard-to-understand areas
  • [x] You have performed a self-review of your own code
  • [x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • [x] UI Changes: You have tested your change using the Google Accessibility Scanner

oyeraghib avatar Sep 01 '22 19:09 oyeraghib

I suggest making the Fab only add new notes. Other actions can be moved to the menu or to the bottom of the deck list.

Alternatively, remove the fab entirely and put the Add note action into the navigation drawer. If we do that, we could also make it possible to leave the Add note activity without saving changes; you could leave it and come back and see your unsaved input restored.

oakkitten avatar Sep 02 '22 15:09 oakkitten

I suggest making the Fab only add new notes. Other actions can be moved to the menu or to the bottom of the deck list.

Alternatively, remove the fab entirely and put the Add note action into the navigation drawer. If we do that, we could also make it possible to leave the Add note activity without saving changes; you could leave it and come back and see your unsaved input restored.

Sounds like a decent new change. Should make it as a new issue, so that everyone can have their views before someone even start to work on it.

oyeraghib avatar Sep 10 '22 09:09 oyeraghib

Unit Tests are failing because of removal of Add Note Layout from DeckPickerFloatingActionMenu

oyeraghib avatar Sep 10 '22 13:09 oyeraghib

This is how it looks currently :

https://user-images.githubusercontent.com/40427461/189486678-910613cb-8e40-4093-a937-3199a54365c5.mp4

oyeraghib avatar Sep 10 '22 13:09 oyeraghib

Only saw the video / haven't looked at the code deeply.

  1. David's request about keeping the label wasn't addressed
  2. Tests fail
  3. Indigo doesn't look good to me and won't match Plain, Dark and Black themes. I suggest creating an attribute and set it on each. themePrimaryDark should be good for the day themes and themePrimaryLight for the night themes should be good. If any of them doesn't exist, you can create it or propose another

BrayanDSO avatar Sep 10 '22 23:09 BrayanDSO

@BrayanDSO I am sorry but I didn't understood it "We need label for the action". Can you explain like what has to be done for this or what does it mean ?

oyeraghib avatar Sep 11 '22 09:09 oyeraghib

@oyeraghib. See the "Get shared decks" and "Add deck" text the buttons' side? That's it. There was one for "Add" as well, but this PR is currently removing it

BrayanDSO avatar Sep 12 '22 13:09 BrayanDSO

This is how it looks currently :

https://user-images.githubusercontent.com/40427461/190522960-a8f72a0b-ebdb-4393-b0b0-07a9107bc539.mp4

oyeraghib avatar Sep 15 '22 23:09 oyeraghib

It is a little bugged on Plain theme. A white halo/ripple persists after tapping the button. Tested on API 31 device and API 25 emulator

image

BrayanDSO avatar Sep 17 '22 11:09 BrayanDSO

@BrayanDSO I checked it on my API 29 device and it doesn't seem to be there.

plain theme

Can you try checking on the build without this PR. As far as I know, I did not introduced anything which might have caused something like (halo) this. But still I would like to know if anyone else can also check this up and let me know.

oyeraghib avatar Sep 20 '22 22:09 oyeraghib

My apologies for being late on this one. So finally fixed the halogen artifact being shown on Plain themes ( I found it while running the app on Android 13 physical device ).

Before :

https://user-images.githubusercontent.com/40427461/196524715-77954d8c-683b-4ae4-8489-7255d843282d.mp4

After :

https://user-images.githubusercontent.com/40427461/196523959-f7d34d8b-fb4f-4ded-a52e-be2bcf03af2f.mp4

The reason : That halogen icon was Android's material shadow artifact, which comes on translucent backgrounds ( therefore it was only visible on the Plain themes - because on pressing the FAB for plain themes we use a different color for background which has lower opacity.

One of the solutions for it on SO mentioned the use of libraries : https://stackoverflow.com/questions/70227950/how-can-we-fix-the-elevation-shadows-on-transparent-translucent-views-without-ch which I thought would not be a good decision considering we only need it inside FAB. So the most ideal solution which I found for it was to make the background color of FAB opaque (in plain themes - on pressing the FAB) which fixes the issue.

Also added couple of animation tweaks ( by updating the durations, translations etc) to make animation look more smoother.

oyeraghib avatar Oct 18 '22 19:10 oyeraghib

Question: do we still need to have a DoubleTapListener set up after this PR?

BrayanDSO avatar Oct 30 '22 17:10 BrayanDSO

Question: do we still need to have a DoubleTapListener set up after this PR?

IMO it is nice to have DoubleTapListener for the reason that it could handle the case if the user double click the FAB quickly ( before the animation is completed ).

It also separates the functionality of the double tapping FAB to open note from animation. So if in future someone wants to change animation for FAB ( work on it's duration - increase/decrease ) it won't have affect on the functionality to open the note. For someone who has habit of double tapping the FAB they won't have any issues because of animation duration.

oyeraghib avatar Oct 31 '22 14:10 oyeraghib

One meta advice, @oyeraghib: you can update your initial PR message. That would be great so that it always has the most up to date video, so that a new reader, or someone who forgot what this PR contains, know in what state the issues currently is.

Regarding the potential action in the FAB: I really want to keep "shared deck" in it. Because the FAB is probably the most obvious things on a first openining of the application, when the user is trying anki* for the first time with us. And so, ensuring they get an easy way to find some shared deck, and try the app without having to create their own cards immediately seems very important to me. Plus, if we were to remove the FAB, we should change the message shown when the collection is empty

Arthur-Milchior avatar Nov 05 '22 06:11 Arthur-Milchior

@Arthur-Milchior Thanks for the feedback. I have updated the PR message with updated before and after videos. Regarding the FAB, I agree with your view

The FAB is probably the most obvious things on a first opening of the application, when the user is trying anki* for the first time with us. And so, ensuring they get an easy way to find some shared deck, and try the app without having to create their own cards immediately seems very important to me.

oyeraghib avatar Nov 05 '22 08:11 oyeraghib

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

github-actions[bot] avatar Jan 04 '23 08:01 github-actions[bot]

Hello 👋, this PR has been opened for more than 1 month with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

github-actions[bot] avatar Feb 21 '23 15:02 github-actions[bot]

needs rebasing onto main & de-conflicting

david-allison avatar Feb 22 '23 17:02 david-allison

needs rebasing onto main & de-conflicting

made the required changes.

oyeraghib avatar Feb 26 '23 20:02 oyeraghib

Tested on API 21 emulator, looks great!

Implementer's choice: I don't like the instant change in color if the user has disabled animations and would rather it didn't occur

To me it makes bit of sense in a way - that it tells the user that functionality of that button has been changed. For users who have been using AnkiDroid for long time might press it again (thinking it's a cross symbol) to close it . A bit of change in color can be a reason of focus and make them see that pressing it again will open the note instead of closing it.

oyeraghib avatar Mar 03 '23 07:03 oyeraghib

I missed the change in color. Thanks David for catching it. I'll agree with David here.

Some people, in particular on e-ink device, really care about the lack of animation. I believe that noting a change in behaviour does not justify the cost for them. After all, it's really not clear they'll notice the color started changing. And in the worst case, they may learn very quickly that the behavior changed

Arthur-Milchior avatar Mar 04 '23 16:03 Arthur-Milchior

Updated with discussed changes.

After updating :

https://user-images.githubusercontent.com/40427461/222978128-655dd0e2-c194-4875-be8e-92cf96075eec.mp4

oyeraghib avatar Mar 05 '23 18:03 oyeraghib

I have animations disabled in the Developer settings, this doesn't handle that case.

david-allison avatar Mar 05 '23 18:03 david-allison

@david-allison do you have any idea if there already defined function in AnkiDroid that can be used to check device's animation status ? Or should I be writing a new one.

oyeraghib avatar Mar 05 '23 19:03 oyeraghib

I am not very experienced with testing at the moment. Seems like tests are failing due to adding that function which returns the status of ( if device's animation ) is enabled or not. Any suggestions how can I use it inside the DeckPickerFloatingActionMenuTest.kt to make tests pass.

oyeraghib avatar Mar 05 '23 20:03 oyeraghib

I believe it is a preferences setting, settings -> advanced -> safe display mode

And then there is a key in preferences (safeDisplay I think?) you may check

mikehardy avatar Mar 05 '23 20:03 mikehardy

I don't believe we have the functionality yet.

To enable on your device: https://www.androidpolice.com/how-to-speed-up-animations-on-your-android-phone/

david-allison avatar Mar 05 '23 22:03 david-allison

I don't believe we have the functionality yet.

To enable on your device: https://www.androidpolice.com/how-to-speed-up-animations-on-your-android-phone/

In developer options we have 3 animations. "Window animation", "Transition animation" and "animation duration". @david-allison may I know which of the following you have disabled so you are seeing the color change?

oyeraghib avatar Mar 06 '23 19:03 oyeraghib

All 3

EDIT: Tried again. Failed with the same error as in the unit tests:


2023-03-04 03:49:22.072 19988-19988 MessageQueue-JNI        com.ichi2.anki.debug                 E  android.provider.Settings$SettingNotFoundException: window_animation_scale
                                                                                                    	at android.provider.Settings$Global.getInt(Settings.java:15261)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.deviceAnimationEnabled(DeckPickerFloatingActionMenu.kt:69)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.showFloatingActionMenu(DeckPickerFloatingActionMenu.kt:88)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.access$showFloatingActionMenu(DeckPickerFloatingActionMenu.kt:33)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu$1.onUnconfirmedSingleTap(DeckPickerFloatingActionMenu.kt:279)
                                                                                                    	at com.ichi2.anki.ui.DoubleTapListener$detector$2.onSingleTapUp(DoubleTapListener.kt:86)

david-allison avatar Mar 06 '23 20:03 david-allison

All 3

EDIT: Tried again. Failed with the same error as in the unit tests:


2023-03-04 03:49:22.072 19988-19988 MessageQueue-JNI        com.ichi2.anki.debug                 E  android.provider.Settings$SettingNotFoundException: window_animation_scale
                                                                                                    	at android.provider.Settings$Global.getInt(Settings.java:15261)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.deviceAnimationEnabled(DeckPickerFloatingActionMenu.kt:69)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.showFloatingActionMenu(DeckPickerFloatingActionMenu.kt:88)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu.access$showFloatingActionMenu(DeckPickerFloatingActionMenu.kt:33)
                                                                                                    	at com.ichi2.anki.DeckPickerFloatingActionMenu$1.onUnconfirmedSingleTap(DeckPickerFloatingActionMenu.kt:279)
                                                                                                    	at com.ichi2.anki.ui.DoubleTapListener$detector$2.onSingleTapUp(DoubleTapListener.kt:86)

@david-allison please try with latest commits if possible and let me know if it is having any issues

oyeraghib avatar Mar 11 '23 18:03 oyeraghib