Skip to content

Allow search to show subdecks if they match the searched term#18298

Merged
david-allison merged 1 commit intoankidroid:mainfrom
ShridharGoel:18287
Jun 3, 2025
Merged

Allow search to show subdecks if they match the searched term#18298
david-allison merged 1 commit intoankidroid:mainfrom
ShridharGoel:18287

Conversation

@ShridharGoel
Copy link
Member

@ShridharGoel ShridharGoel commented May 10, 2025

Purpose / Description

This addresses a bug where searching for a sub-deck in the deck picker would fail to return the expected deck if one of its ancestor decks was collapsed. For example, if a user has the deck structure Science::Math::Algebra::Group and the Math deck is collapsed, searching for "Group" would not yield the Science::Math::Algebra::Group deck. This is not ideal, as users should not need to remember the collapsed state of parent decks when searching for a specific sub-deck.

Fixes

Approach

When searching, we'll ignore collapsed state and always search sub-decks.

How Has This Been Tested?

Manual testing + unit tests.

Demo

Screen_recording_20250511_010042.webm

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

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

I'd appreciate one extra test, but it clearly solve the issue so fine by me

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

Actually, I know see another problem with your code.

If you search for "algebra", then the little icon on the left of the deck shows that "science" is not collapse, "math" is collapsed, and "algebra" is not collapsed.

The button on the left of "algebra" will allow to indeed collapse on uncollapse subdecks. The button on the left of "science" and "math" will be ignored.

I believe we should remove the buttons whose action are ignored. That is, "science" and "math" should not show any button to collapse/uncollapse.

In an ideal world, I'd keep them, if I were certain the user would understand that the impact of those button would be seen one the search is removed. However, this behavior would be very confusing, so I believe we should just remove it.

This can be left as a TODO, given that your PR is already an improvement over the current state of the codebase, I'm fine merging it once the extra tests are added

@Arthur-Milchior Arthur-Milchior added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels May 18, 2025
@Arthur-Milchior Arthur-Milchior self-requested a review May 18, 2025 02:52
@ShridharGoel ShridharGoel added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels May 18, 2025
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label May 18, 2025
Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

Once the issue is created and the TODO contains an explanation instead of a link I'm fine merging it

@ShridharGoel ShridharGoel removed the Needs Author Reply Waiting for a reply from the original author label May 27, 2025
@ShridharGoel ShridharGoel added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels May 30, 2025
@ShridharGoel ShridharGoel requested review from david-allison and removed request for david-allison June 1, 2025 09:08
@david-allison david-allison added this pull request to the merge queue Jun 3, 2025
Merged via the queue into ankidroid:main with commit c139bd9 Jun 3, 2025
9 checks passed
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Jun 3, 2025
@github-actions github-actions bot added this to the 2.21 release milestone Jun 3, 2025
@ShridharGoel ShridharGoel deleted the 18287 branch June 3, 2025 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search should show hidden subdeck if it match

3 participants