feat: new reviewer menu actions configuration #17587

BrayanDSO · 2024-12-10T20:20:20Z

Approach

In the commits and the learning section.

I probably should do more documentation and tests, but I'm too "in the zone" to see exactly what is missing and it will take at least a week until I got a cleaner mind to look at this with another eyes.

How Has This Been Tested?

Android 14 phone, Samsung SM-S911B, and in the emulator with API 34 as well

Screen_Recording_20241210_165310_AnkiDroid.mp4

Screen_Recording_20241210_165358_AnkiDroid.mp4

Learning (optional, can help others)

The hardest part was not the drag and drop, or the scrollable menu view. It was hiding the Bury and Suspend submenus if the user only could bury a card.

First I tried to hide the submenu arrow and override the click on the menu option. The arrow is changeable with android:submenuArrow, but I couldn't find a way to do that dynamically. Then I thought to recreate the menu, or take bury and suspend in account when first building it, but the result would be too messy. So I went on reading the source code of Android's menus, since the documentation is scarce.

Android menus are a bunch of interfaces with hidden implementations.

MenuItem -> MenuItemImpl
Menu -> MenuBuilder
SubMenu -> SubMenuBuilder

Overall, they are inflexible and you can't do many things with them, like adding a menu item at an specific position (you need to recreate the menu readding things in the order you want), or removing a submenu (again, you need to recreate the whole menu for that).

So if you want more power, you need to do some casting and user their implementantions, or use reflection, like I had to.

A toolbar menu is displayed by a ActionMenuView, which was how I managed to make a toolbar menu scrollable.

Using two ActionMenuViews is a bit hacky, but the alternatives would be doing my own implementation of a toolbar menu, either with a RecyclerView or a HorizontalScrollView (the two options for scrolling horizontally that I know of). That would be a hell of a job, specially in recreating the popups (another dark Android API that I don't want to touch again) and the tooltips. Another problem with doing that is that it wouldn't look exactly how Android toolbars look, and making things familiar to the users is important.

Checklist

Please, go through these checks before submitting the PR.

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

david-allison

This is excellent! All comments are optional

UX: I would have dragging anywhere on the item move the item, rather than just the handle

I would have a 'tap' suggest dragging to the user

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/MenuDisplayType.kt

david-allison · 2024-12-10T22:03:55Z

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/MenuDisplayType.kt

+import androidx.core.content.edit
+import com.ichi2.anki.R
+
+enum class MenuDisplayType(


Should this be sent to ACRA? I'd consider removing it

What do you mean?

Implementers choice:

We send all preferences barring sensitive ones to ACRA, and the list is getting long.

Do you feel knowing these values would provide help when debugging an issue? I don't feel they'd be useful and would consider adding these to the list of ignored properties

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/ReviewerMenuSettingsAdapter.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/ReviewerMenuSettingsFragment.kt

AnkiDroid/src/main/java/com/ichi2/anki/utils/Reflection.kt

AnkiDroid/src/test/java/com/ichi2/anki/preferences/SettingsSearchBarTest.kt

...src/main/java/com/ichi2/anki/preferences/reviewer/ReviewerMenuSettingsTouchHelperCallback.kt

AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt

lukstbit

LGTM, left some minor questions.

I have to say I'm not enthusiastic about the horizontal scrollable menu container because I don't see the value. As long as menu items are hidden, I'll have to make the same effort to get to them, in one case I scroll and click menu item vs. the standard I click(overflow menu) and click menu item. The difference being that the second option is simpler to implement.

When I fixed the menu between the DeckPicker and StudyOptionsActivity I was thinking that is possible and great if we allow the user to customize the menu for each screen by calculating the available screen size(into buckets) and manipulating the menu items with showAsAction (always/never) based on settings.

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/ViewerAction.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/MenuDisplayType.kt

...src/main/java/com/ichi2/anki/preferences/reviewer/ReviewerMenuSettingsTouchHelperCallback.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/reviewer/ReviewerMenuView.kt

AnkiDroid/src/test/java/com/ichi2/testutils/ReflectionUtils.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/ReviewerOptionsFragment.kt

BrayanDSO · 2024-12-11T17:20:09Z

I'm not enthusiastic about the horizontal scrollable menu container because I don't see the value. As long as menu items are hidden, I'll have to make the same effort to get to them, in one case I scroll and click menu item vs. the standard I click(overflow menu) and click menu item.

I can see the point and I won't even going to use the feature. But, scrollable toolbars are actually quite common, as seen in many text editors and even in AnkiDroid's note editor (just to be clear, I mean toolbars in general, not only Android top bars).

About the effort to do an action, it can be less than tapping the menu, since a person needs to do an extra tap the menu every time they use an overflown action, but if it is already scrolled to the action they want, they can keep using the action without the extra tap.

If they want to keep using the same behavior of a default toolbar, they can. If they want to ditch the overflow menu, they can. If they want to use both, they can. All of that without making the app more complex to the users.

For us developers, it is indeed more complex than directly using a simple toolbar menu, but it isn't hard to understand or to work with at all. It's just a toolbar menu embedded in a horizontal scroll view and a separate one to show the overflow icons.

BrayanDSO · 2024-12-13T01:22:49Z

UX: I would have dragging anywhere on the item move the item, rather than just the handle

Replaced it with a long press in the whole item, which avoids moving something when scrolling. It also makes the code simpler and the list more compact, which looks better IMO

Screenshot

I think I addressed all of the feedback. Thank y'all for the reviews, it helped a lot.

I'll wait to hear if anyone has more considerations to do.

ViewerAction is similar to ActionButtonStatus, but in an Enum It also is similar to ViewerCommand and will replace it when gestures are implemented in the new reviewer --- MenuDisplayType is a set of three display options for an action in the reviewer menu. The legacy `If room` setting isn't being added because it is a setting for the *developers* to use while designing a menu. For a *user* it is bad, because: - It isn't obvious how it works - It only makes a difference when the app window is resized on bigger screens. In phones it won't make a difference, which represents most of our users. - The difference of that resize is mostly 1-2 icons to be added or removed, which doesn't help much - The reviewer ignores `screenSize` configuration changes, so all of that only apply if the reviewer is opened and closed So I removed it to simplify things

just a blank screen for now. Split from the other commits to make the reviewing process easier Why using an Activity instead of launching it like the other preferences screens? Because of the toolbar. The new screen for setting the reviewer menu actions will have a toolbar that serves as a preview of how it will look in the reviewer. Differently from the Preferences' toolbar, it isn't collapsible, so I can't simply do the preview there. Since all of the options that I thought required a big amount of work, I decided to simply use a standalone activity for now and fix it later. For phone users, there won't be any visible differences. For tablet users, it just won't shown the lateral navigation bar of the preferences page.

this allows handling Companion objects as well

will be used in the next commit refactor: move getJavaFieldAsAccessible to main I also changed the method to not throw by default if the field name could not be found

david-allison

The long tap threshold is too long if the user is using the controls on the right, I feel they should instantly start a drag action. In general, the current threshold feels like "nothing is happening" (but I feel this is a bad customizable Android default setting)

I would consider adding a little more vertical padding between the elements, I had a few mistaps

BrayanDSO · 2024-12-13T09:57:57Z

Okay, min height is now listPreferredItemHeight, the drag handle start dragging instantly, and you can still drag by tapping somewhere else if you do a long click (can't make it instant, otherwise it's impossible to scroll the screen)

lukstbit · 2024-12-13T10:05:56Z

Still fine for me 👍

david-allison

Animation transition from the preferences is a little wonky. Implementer's choice. Merge at will!

I'm more than happy though, UX of this screen is excellent!

Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc)

Compared to the legacy reviewer, it: - Allows to reorder actions - Has a preview in the settings screen of how the menu looks - It is easier to setup with drag and drop gestures - Allows to scroll the `Always show` actions instead of hiding them if the user set many of them

mikehardy · 2024-12-17T22:22:11Z

Fantastic @BrayanDSO

BrayanDSO changed the title ~~Toolbar buttons~~ feat: new reviewer menu actions configuration Dec 10, 2024

BrayanDSO added the Needs Review label Dec 10, 2024

BrayanDSO force-pushed the toolbar-buttons branch 3 times, most recently from a00f32c to 3d772f6 Compare December 10, 2024 20:42

david-allison approved these changes Dec 10, 2024

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Dec 10, 2024

BrayanDSO force-pushed the toolbar-buttons branch from 3d772f6 to 44f11a9 Compare December 11, 2024 08:22

BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Dec 11, 2024

lukstbit approved these changes Dec 11, 2024

View reviewed changes

lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Dec 11, 2024

BrayanDSO force-pushed the toolbar-buttons branch 3 times, most recently from a67b807 to 1eedd8e Compare December 13, 2024 01:18

BrayanDSO added Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Dec 13, 2024

BrayanDSO force-pushed the toolbar-buttons branch from 1eedd8e to 016aac2 Compare December 13, 2024 01:27

BrayanDSO added 2 commits December 12, 2024 22:28

BrayanDSO force-pushed the toolbar-buttons branch 2 times, most recently from c5e0b86 to 6c9d371 Compare December 13, 2024 01:29

BrayanDSO added 2 commits December 12, 2024 22:29

refactor(test): assertConstantUniqueness

b592514

this allows handling Companion objects as well

refactor: add MenuItemImpl utils

a8fdd13

will be used in the next commit refactor: move getJavaFieldAsAccessible to main I also changed the method to not throw by default if the field name could not be found

david-allison reviewed Dec 13, 2024

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Dec 13, 2024

BrayanDSO force-pushed the toolbar-buttons branch from 6c9d371 to e84589e Compare December 13, 2024 09:54

BrayanDSO force-pushed the toolbar-buttons branch from e84589e to 86437ee Compare December 13, 2024 09:56

BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Dec 13, 2024

david-allison approved these changes Dec 13, 2024

View reviewed changes

david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs reviewer reply Waiting for a reply from another reviewer labels Dec 13, 2024

BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Dec 13, 2024

BrayanDSO force-pushed the toolbar-buttons branch from 86437ee to 4933b07 Compare December 13, 2024 10:19

BrayanDSO enabled auto-merge December 13, 2024 10:20

BrayanDSO added this pull request to the merge queue Dec 13, 2024

Merged via the queue into ankidroid:main with commit cbc9424 Dec 13, 2024
9 checks passed

github-actions bot added this to the 2.21 release milestone Dec 13, 2024

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Dec 13, 2024

BrayanDSO deleted the toolbar-buttons branch December 13, 2024 11:29

BrayanDSO mentioned this pull request Dec 14, 2024

[Bug] Degrading performance with each card reviewed #12980

Open

4 tasks

david-allison mentioned this pull request Dec 18, 2024

Widget/shortcut should allow to sync #17084

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new reviewer menu actions configuration #17587

feat: new reviewer menu actions configuration #17587

BrayanDSO commented Dec 10, 2024 •

edited

Loading

david-allison left a comment

david-allison Dec 10, 2024

BrayanDSO Dec 12, 2024

david-allison Dec 13, 2024

lukstbit left a comment •

edited

Loading

BrayanDSO commented Dec 11, 2024 •

edited

Loading

BrayanDSO commented Dec 13, 2024 •

edited

Loading

david-allison left a comment •

edited

Loading

BrayanDSO commented Dec 13, 2024

lukstbit commented Dec 13, 2024

david-allison left a comment •

edited

Loading

mikehardy commented Dec 17, 2024

feat: new reviewer menu actions configuration #17587

feat: new reviewer menu actions configuration #17587

Conversation

BrayanDSO commented Dec 10, 2024 • edited Loading

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

david-allison left a comment

Choose a reason for hiding this comment

david-allison Dec 10, 2024

Choose a reason for hiding this comment

BrayanDSO Dec 12, 2024

Choose a reason for hiding this comment

david-allison Dec 13, 2024

Choose a reason for hiding this comment

lukstbit left a comment • edited Loading

Choose a reason for hiding this comment

BrayanDSO commented Dec 11, 2024 • edited Loading

BrayanDSO commented Dec 13, 2024 • edited Loading

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

BrayanDSO commented Dec 13, 2024

lukstbit commented Dec 13, 2024

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

mikehardy commented Dec 17, 2024

BrayanDSO commented Dec 10, 2024 •

edited

Loading

lukstbit left a comment •

edited

Loading

BrayanDSO commented Dec 11, 2024 •

edited

Loading

BrayanDSO commented Dec 13, 2024 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison left a comment •

edited

Loading