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.

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

Fix Apply button crashing on no selection #11875

Merged
merged 1 commit into from Jul 17, 2022

Conversation

viciousAegis
Copy link
Member

@viciousAegis viciousAegis commented Jul 17, 2022

Pull Request template

Purpose / Description

col.buildSearchString was getting an empty node, thus raising the error. This takes care of it by returning early if the search is empty.
Side Note: Should buildSearchString handle empty nodes?

Fixes

Fixes #11872

Approach

This takes care of it by returning an empty string early if the search is empty, so buildSearchString is not called

How Has This Been Tested?

Manually on my phone

Screenrecorder-2022-07-17-10-57-38-952.mp4

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

Learning (optional, can help others)

Describe the research stage

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • 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

@BrayanDSO
Copy link
Member

I think that disabling the Apply button if there isn't anything selected would be a nicer ergonomic fix.

If it would be best to implement it now or later when the other filter options are added, I don't know. As you're the implementer, you should have a better idea of when.

@viciousAegis
Copy link
Member Author

Yeah, that makes sense, I'll remove all functionality on it for now as a temp fix, and i'll disable it later when I have some more free time

@Arthur-Milchior Arthur-Milchior merged commit 9b07705 into ankidroid:main Jul 17, 2022
@github-actions github-actions bot added this to the 2.16 release milestone Jul 17, 2022
@Arthur-Milchior
Copy link
Member

It's not clear to me how you'll generalize it to a search with multiple options such as Note Type, but this is clearly a valid PR so I merge.
I may have a slight preference to return Null instead of "" to indicate a special value, especially if you do a test later, but not enough to block on it.

My main question is:

  • let's say you already have a search in the searchbar. You open the filter sheet. You select nothing and click apply. What is the expected behavior. I believe that removing the search would be more natural, as you'd apply the filter sheet's selection, and so don't filter anything anymore. However, I admit that this case would probably lead to a long discussion with no right answer as two users may have different intuition here.

Relatedly: we may consider a special case where, if the user selected all colors and also "no flag", this node is also removed. Just to simplify the query

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.

[Bug] Crash if card browser query is made with no items selected
4 participants