Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add functionality to custom study by tags or card state #17948

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add functionality to custom study by tags or card state #17948

wants to merge 3 commits into from
+675 −244

Conversation

lukstbit
Copy link
Member

@lukstbit lukstbit commented Feb 9, 2025

Purpose / Description

Enables for the app the full desktop functionality related to custom study by tags. Some notes related to the implementation:

  • follows the desktop flow with "Choose tags" button showing a new dialog and on ok on that dialog returning to the previous dialog to do the actual setup of the custom study
  • uses different layouts for portrait/landscape but the transition between them doesn't work because DeckPicker is set(although it shouldn't) to handle the configuration manually
  • I added a border for both lists because the middle label was kind of blending with the lists IMO
How it looks on mobile

Screenshot from 2025-02-09 20-11-15
Screenshot from 2025-02-09 20-11-45Screenshot from 2025-02-09 20-14-58

Screenshot from 2025-02-09 20-15-52

Screenshot from 2025-02-09 20-12-57

Screenshot from 2025-02-09 20-17-13
Screenshot from 2025-02-09 20-16-33

How it looks on desktop

Screenshot from 2025-02-09 20-51-31
Screenshot from 2025-02-09 20-51-41

Fixes

How Has This Been Tested?

Ran the tests, manually verified the new dialogs/code paths.

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 9, 2025

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@lukstbit lukstbit force-pushed the feat_customStudyByTagsOrCardsState branch from 896f7f1 to 00505b8 Compare February 10, 2025 14:09
@lukstbit lukstbit changed the title Add functionaity to custom study by tags or card state Add functionality to custom study by tags or card state Feb 11, 2025
david-allison
david-allison requested changes Feb 13, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Don't let any of these nitpicks get in the way of getting it in. It's more than 'good enough'

Mostly UI nitpicks:

Margins are off:

Screenshot 2025-02-13 at 09 10 08
  • add a little more spacing between the 'Select tags to exclude' and the list
  • if there is a scrollbar, always show it in the selection dialog
  • make the 'Select tags to exclude' label look like a title
  • Anki replaces "_" with " " when visually displaying tags in this screen. Optional if you want to do this
  • (optional/add as TODO): it feels like we want a ripple select
  • I /think/ the highlighting should go away if a user unchecks the checkbox

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/TagLimitFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/TagLimitFragment.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/TagLimitFragment.kt Outdated Show resolved Hide resolved
@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Feb 13, 2025
@lukstbit
Refactor CustomStudyDialog to handle the STUDY_TAGS path
26005ce 
This is a no-op commit that adds the required code for CustomStudyDialog
to handle the STUDY_TAGS option like the other paths. The new code is not
used yet as the dialog will still go through TagsDialog.

For tags, the ui will show an input box to enter the amount of cards(like
the other options) but it will also show a Spinner with 4 entries to select a
card state to further limit the cards available for studying.

The enum CustomStudyCardState models the 4 cards states the user
can select from before selecting the tags.
@lukstbit lukstbit force-pushed the feat_customStudyByTagsOrCardsState branch 2 times, most recently from a2540a4 to 3dd0757 Compare February 14, 2025 10:16
@lukstbit
Replicate desktop custom study by tags functionality
d3291bc 
Folow ui/functionality from desktop.

A new dialog(instead of using/absuing TagsDialog), TagLimit, was
created to show the same ui as desktop.
@lukstbit
Remove unused code in TagsDialog related to custom studying
e2fcdc7
@lukstbit lukstbit force-pushed the feat_customStudyByTagsOrCardsState branch from 3dd0757 to e2fcdc7 Compare February 14, 2025 10:19
@lukstbit
Copy link
Member Author

Implemented most requests, except for:

I /think/ the highlighting should go away if a user unchecks the checkbox

because that's how desktop behaves.

How it looks now

Screenshot from 2025-02-14 12-11-08
Screenshot from 2025-02-14 12-11-32
Screenshot from 2025-02-14 12-11-49
Screenshot from 2025-02-14 12-21-28
image

@lukstbit lukstbit removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Feb 14, 2025
@david-allison

This comment was marked as resolved.

Sign in to view
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Feb 14, 2025
david-allison
david-allison approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like a minor re-think of the design of "select tags to exclude", as it still doesn't fully feel like a title

But this is great

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/IncludedExcludedTagsAdapter.kt
position: Int,
) {
val model = tags[position]
holder.tagView.text = model.name.replace("_", " ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to name this label, or userFacingLabel on the class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison approved these changes

Assignees
No one assigned
Labels
Needs Author Reply Waiting for a reply from the original author Needs Review Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom Study: "Study by card state or tag"
3 participants
@lukstbit @david-allison @mikehardy