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

feat(card-browser): Chip Based Filtering [Tags, Flags & State]

Open david-allison opened this issue 1 year ago • 6 comments

[!IMPORTANT]

  • Commit messages need work, as well as more attribution and thanks to OK
  • The textual descriptions on 'State' need a lot of bikeshedding
  • Some of the PR description should be applied as code comments
  • Hiding the toolbar on scroll needs to be implemented

I feel the rest is ready for review

Purpose / Description

This is an Android-based implementation of the sidebar in Anki Desktop which handles [Tags; Flags; State]

The implementation begins to productionize @oakkitten's proposal for using Material 3 Chips to filter categories in the Card Browser.

  • https://github.com/ankidroid/Anki-Android/issues/12554

A large amount of this Pull Request text is copied from the above. THANK YOU

Anki Desktop: Browser Sidebar Screenshot 2024-08-11 at 17 41 13

Images

Screenshot 2024-08-11 at 19 28 25 Screenshot 2024-08-11 at 19 27 21 Screenshot 2024-08-11 at 19 28 46 Screenshot 2024-08-11 at 19 54 32

new tag icons Screenshot 2024-08-20 at 12 19 29

Behavior

Chip Group

Screenshot 2024-08-11 at 18 02 41

A single-line chip bar, horizontally scrollable, is placed below the toolbar. Tapping on a chip opens the according bottom sheet, where you can select one or multiple items. When you select items, search is instantly re-applied, and the chip change color, label and optionally, icon to reflect the selection.

For example, Flags changes to Red if the red flag is selected or Red +1 if an additional flag is selected. In the latter case, Red would be the flag which the user selected first.

In the case of Flags and State, the icon is updated to match the icon of the first selected flag/state

Scrolling

⚠️ TODO: The toolbar, along with the chip bar and the spinners bellow, is collapsed when the user scrolls down the list of cards.

Bottom Sheets

Screenshot 2024-08-11 at 18 56 26

The bottom sheets show lists of items with icons, text, checkboxes and an optional description. To select items, you can:

  • Tap on the item icon/label:
    • A quick way to select only one thing. The dialog will be closed, a This will select the tapped item and close the dialog. If any other items were selected, those are de-selected.
  • Tap on the checkbox:
    • This does not dismiss the dialog and allows the user select multiple items. The dialog may be dismissed by swiping the sheet downwards, or tapping in the scrim area.

All the bottom sheets have a Clear filter icon without a checkbox. Tapping on it clears the selection and closes the dialog, stopping filtering by this category.

Chevrons / Indenting

Context: Hierarchical decks/tags

The bottom sheets can display directory tree-like items. Items that have subitems can be collapsed to hide those and show a clickable chevron to indicate collapsed state (/). If there are any chevrons, all items are shifted to the right to make place for the leftmost chevrons.

Searching

Screenshot 2024-08-11 at 20 07 54

Context: User-defined categories: [Decks; Tags; Note Types]

If there are more than 10 items, a search input is visible with a hint: Search tags. Tapping on it expands the sheet to full screen (if enough items). A "clear search" button is shown when text is entered. When typing, items that do not contain the input are hidden, with the exception of any parent items, e.g., searching for foo would change

* abc         
* def                     * def
  * hello        -->         * hello
     * foo                     * foo
     * bar                * foo fighters
* foo fighters

The bottom sheet survives rotation, preserving selection & scroll position.

State: Descriptions

Screenshot 2024-08-11 at 18 58 46

[!NOTE] The above screenshot is to demonstrate the UI only, please view the Pull Request for the current strings

The State control provides us with an opportunity to explain a number of Anki's concepts to users. I would strongly encourage us all to discuss these descriptions, as these will resolve a number of onboarding issues.

⚠️ My initial descriptions are not sufficient to be merged, and therefore are hardcoded in CardSheetFragment::State

Feature Removals

This pull request removes the following menu items from the Card Browser. These were available from the Card Browser's menu when not in multi-select mode

  • Filter marked:

    • marked is implemented as a tag, which is displayed as a tag in the tag selection fragment. This tag is displayed with a distinct 'star' icon
  • Filter suspended

    • suspended is an option in the State chip
  • Filter by tag

    • The Tags chip handles all functionality
    • ⚠️ The All cards/New/Due selector is not yet fully implemented
      • New is handled under State
      • ⚠️ Anki Desktop handled is:due under Today, which will be implemented in a later Pull Request
  • Filter by flag

    • Handled, with more functionality (OR) under the Flags chip.
  • Remove: Ctrl+T to filter by tag

  • Remove: Ctrl+M to search for marked notes

  • Remove: Alt+S to Show suspended cards

Future Extensions

The following chips are planned, mirroring Anki Desktop's functionality. These were not implemented as this Pull Request is already extremely unwieldy, and will likely result in a long review cycle.

  • Decks
    • Introducing this would produce two UI elements which modify the same category in different ways: our current ActionView-based UI does not handle multiple decks in an OR
    • We should move to a material SearchView/Bar in the same commit, and this involved a fair amount of design improvements
  • Today
    • Handling parameters: in Again -> rated:1:1 needs a little work
    • These are parameters, BUT we don't display this in the search bar yet
  • Note Type
    • Should be an easy implementation, low-yield and I initially chose categories which replaced menu items in the Browser

Anki Desktop's Backend contains 3 states which are not displayed in Anki Desktop:

  • is:due
    • Exposed somewhat in "Today - Overdue":
    • Overdue -> is:due -prop:due=0

⚠️ I would support handling the following two states in this Pull Request,

  • is:buried-manually
  • is:buried-sibling

  • Long-press menus should be available
    • A user may rename a flag
    • A user may delete a tag
    • A 'Learn More' menu item for states

  • Each category is searched with AND, and items within categories with OR.
    • An advanced search mode will be made available to expose additional filters (nc:, re:`), as well as logical options. This will be focused on changing the search text, to expose all of Anki Desktop's search functionality
    • Using the Ctrl/Alt modifiers in Anki Desktop will likely open up this advanced editor. Displaying logical search combinations using a checkbox-based UI is untenable

Selected tags may want to be moved to the top of the list when the selection dialog is reopened


Visuals

  • The chips can have transparent background which makes them behave just like Gmail chips in the sense that when you tap on them, the ripple effect only happens to the outline. Can be changed to a more usual behavior.

  • When changing the items in the bottom sheet, default RecyclerView animations are run with the exception of the chevron animation. Instead of the default crossfade, when item's subitems are collapsed or shown, chevron is rotated 90°.

  • Oakkitten created new icons, as seen in the screenshots

    • In tag list, the tag marked has a star icon.
  • Text gravity depends on layout RTL regardless of text itself. Stuff like menus work this way.

Caveats & considerations

  • When the contents of the bottom sheet change height, the bottom sheet height change is not animated. Not sure what would be a good way to solve this.

  • In dark mode, the bottom sheet does not expand to the status bar for some reason.

  • Normally, the bottom sheet height is limited by its contents. It would be nice if when the user taps on the search input, the sheet would switch temporarily drop this limit, and keep the search input on top of the screen even if the list has few items. While the bottom sheet API should allow just that, the result is super buggy in many ways. Not sure if there's an easy workaround.

Implementation

  • SearchParameters is a parcelable data class that stores user input and all the filtering information. It makes String queries that can be passed to the backend.

  • Chips.kt has functions that set up and update chips based on changes in searchParameters.

  • BottomSheetDialogFragmentFix is a fix for an issue in BottomSheetDialogFragment; in landscape, the peek height of it is too small.

  • BottomSheetFragment which is a subclass of ↑ is a base for all bottom sheets. It inflates the layout (with a RecyclerView, and with or without the search input) and wires together the views. Its subclasses are responsible to setting up the adapter and reacting to clicks, etc.

  • TreeAdapter is a RecyclerView.Adapter which displays a flat list of TreeAdapter.Item. TreeAdapter is responsible the layout, event handling and display of the items via a ViewHolder. It handles notifying the RecyclerView of modification of the items via DiffCallback. See R.layout.bottom_sheet_item for the layout.

    • In particular, it handles the horizontal padding necessary for the chevrons/indented items
// TreeAdapter.Item
{
    id: Long,
    icon: DrawableRes?,
    text: CharSequence,
    subtitle: String?,
    indent: Int,
    collapsed: { Yes, No, NotCollapsible },
    checked: { Yes, No, NotCheckable }
}
  • TemplatedTreeAdapter is a subclass of ↑. It holds an immutable list of SourceItem, along with the user's selection of expanded and checked items. The combination of (SourceItem, checked, expanded) produces a TreeAdapter.Item
// TemplatedTreeAdapter.SourceItem
{
    id: Long,
//    ... as above
    checkable: Boolean
}

Although we could use a tree structure, we can get away with a flat list. Consider a list of tags:

  foo::bar::baz
  zoo
  foo

Let's call every string in this list “a chain”; a chain is a :: separated string, and let's call what's between ::s a chain link. If we add missing subchains (foo::bar), and case-insensitive sort, we get:

  foo
  foo::bar
  foo::bar::baz
  zoo

And this is something that we can already show! The text would be the leaf chain (foo, bar), the indent would be the number of links in the chain.

This is what Iterable<Row>.toCheckableSourceItems() and the code around it does; it takes a bunch of rows (⚠️ to be renamed, row is a chain with the associated ID and icon) and converts them to TemplatedTreeAdapter.SourceItem. So the path of transformation of data would be like this:

* Get a set of chains from backend
* Add missing subchains and sort
* Convert the list to a list of `Row`s with IDs and icons
* Convert the list of rows to a list of `SourceItem`s

All containers in question are (for the purpose of this) deeply immutable.

Caveats & considerations

Oakkitten's Wizard extensions in the linked issue are not implemented.

If the card list has few items, dragging the empty area does not collapse/uncollapse the toolbar. This is probably a problem with the ListView. Wrapping it in NestedScrollView doesn't seem to work. I hope that changing it to a RecyclerView will resolve the issue.

Currently, bottom sheet fragments are aware of the collection and that's where these get the data in sync. I think they should be able to get that data cached. This is simple and this ensures that the adapter has the latest data. However, this means that on view recreation the data can change. For the most part this is not an issue, but tags are strings and don't have intrinsic IDs, and tag position in tag list is used instead. In theory, if for instance the tags change between activity recreations, you can have RecyclerView restored to a wrong position. I don't think this can lead to any important issues, but just to be safe the IDs can be somewhat stabilized by keeping a static map of tag to id, or interning strings maybe. (Is string interning fast?)

The collapse state of items is not saved and vanishes on activity recreation, and I don't think it's worth saving it.

The new icons should be optimized.

How Has This Been Tested?

  • Night mode
  • Tested filtering by flag, tag and state
  • Tested adding a tag: it appears when the fragment is next opened
  • Tested using a large list of tags: col.tags.all() is fast
  • Tested 'Clear filter'
  • Tested RTL, all but the 'Search Tags' works, and I believe this is due to the String being in LTR
  • Saved Searches include the filters and search string
  • ⚠️ Test with AnKing tags

Checklist

  • [ ] 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 avatar Aug 11 '24 19:08 david-allison

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

github-actions[bot] avatar Aug 11 '24 19:08 github-actions[bot]

  • ⚠️ Double-tapping a chip may open the filter sheet twice
  • We need to decide on singular or plural for the chips:
    • Anki uses [Flags; Tags; Card State]
    • ⚠️ We currently use [Tags, Flag, State ] - obviously wrong
    • Singular is shorter, but plural is likely better here for both consistency and 'feel'

Guidelines (none are relevant):

Chip label text should be 20 characters or fewer, and have the same typography style as buttons.

Chip labels should remain brief for the limited space available. Skip conventional grammar rules, such as articles (take "a" walk), to save space.

https://m3.material.io/components/chips/guidelines#1716ba42-2b8f-40af-9505-e25fc35665fd

Write filter chips with nouns that describe the category to include in the results. Avoid negative phrases like "Exclude images."

https://m3.material.io/components/chips/guidelines#8d453d50-8d8e-43aa-9ae3-87ed134d2e64

david-allison avatar Aug 11 '24 22:08 david-allison

Bottom Sheets

  • Tap on the item icon/label: A quick way to select only one thing. The dialog will be closed, a This will select the tapped item and close the dialog. If any other items were selected, those are de-selected.

Regarding the part above, that automatically closing seems useful as the quick way, but I'm still not sure that de-selecting other items would be useful.

if any other items were selected, those are de-selected.

Is it not possible that the behavior is unexpected for users and that this instant de-selecting (accompanied auto-closing the bottom sheet) is somewhat hard to visually confirm on the spot?

snowtimeglass avatar Aug 20 '24 00:08 snowtimeglass

Is it not possible that the behavior is unexpected for users and that this instant de-selecting (accompanied auto-closing the bottom sheet) is somewhat hard to visually confirm on the spot?

Long-term:

  • One click to select is the most efficient behavior possible, I like it

Short-term:

  • There's enough post-action visual feedback for a user to know that everything is deselected
  • The only thing I don't like is that there's a bug in the visual 'ripple' effect when an item is selected, it's not as prominent as it should be.
  • I feel a "everything was deselected" modal would be annoying (maybe a snackbar with an undo)

I don't feel the comment is prescriptive in a sense of "we should do this instead", and would like a path to resolution

david-allison avatar Aug 20 '24 11:08 david-allison

If a user imports a deck of suspended cards, the first time they come into the browser, it's not 'obvious' that cards are suspended, and we need some way of better explaining what a suspended card is. None of these options seem great: when entering the screen, tapping the card, immediately opening the browser with -is:suspended, so a user can visually see that we're excluding suspended cards, and makes the yellow -> suspended association by removing the filter The yellow background of a suspended card doesn't obviously imply that the card is suspended, and doesn't map well to the current icon we're using [as we don't include the yellow color here]

Can you just have a popup when importing that says "some or all of the cards you've imported are suspended. To start studying them, visit the browser and unsuspend these cards"

AnKingMed avatar Aug 20 '24 22:08 AnKingMed

I don't feel the comment is prescriptive in a sense of "we should do this instead", and would like a path to resolution

(In conclusion, I don't disagree with the current way.)

I still don't have a thought of "this way should be done" about this. I still don't even have a thought of "I prefer this way more", either.

I just feel that deselecting the other items might be uncommon as a behavior in such bottom sheets of Android.

If I suggest an alternative, it may be the common (in my assumption. It may be wrong) behavior as in Gmail, that is, not-deselecting the other items:

https://github.com/user-attachments/assets/da296b75-47c6-4582-b5e0-2721c86e0675

(Gmail)

However, on second thought, one click to select one item exclusively is consistent with the behavior in the desktop browser. It seems that there is enough post-action visual feedback. Also, user can select multiple items by clicking check boxes. So, I agree these distinguished selection ways are useful.

snowtimeglass avatar Aug 21 '24 09:08 snowtimeglass

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. 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 Sep 14 '24 16:09 github-actions[bot]

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. 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 Sep 29 '24 04:09 github-actions[bot]

I'll close this for now. I'm not going to have the time to get to it for a while, and when I do, I'll split it further into smaller PRs. I truly appreciate the time which was spent on it, and I do plan to get back to it as my next large piece of work

david-allison avatar Sep 29 '24 12:09 david-allison