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 crash during deck filtering #11880

Merged

Conversation

shaiguelman
Copy link
Contributor

@shaiguelman shaiguelman commented Jul 18, 2022

Pull Request template

Purpose / Description

Fixes issue where search bar was not filtering properly and crashing the app.

Fixes

Fixes #11877

Approach

Creates a copy of the list of decks before filtering. The error was caused by using the same list in filtering as in the UI. I also made a small optimization of the containsFilterString method as well as added error catching in TypedFilter in case error happens again.

How Has This Been Tested?

Reproduced error using process described in the issue before and after code changes. Screenshot of results after code changes:

Screenshot_20220717-200601

Learning (optional, can help others)

Stack overflow post that helped me narrow the issue

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

@welcome
Copy link

welcome bot commented Jul 18, 2022

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@shaiguelman
Copy link
Contributor Author

Added basic unit tests to ensure filter does not break again. Could probably use a few more to cover more edge cases but figured this was good enough for now

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I love this, thank you!

Revert the publishResults change (or discuss if I'm wrong) and it's good to go

@david-allison david-allison added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Jul 18, 2022
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

looks great, thank you so much

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jul 18, 2022
AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt Outdated Show resolved Hide resolved
@@ -51,7 +52,13 @@ abstract class TypedFilter<T>(private val getCurrentItems: (() -> List<T>)) : Fi
override fun publishResults(constraint: CharSequence?, results: FilterResults?) {
// this is only ever called from performFiltering so we can guarantee the value is non-null
// and can be cast to List<T>
val list = results!!.values as List<T>
val list = try {
Copy link
Member

Choose a reason for hiding this comment

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

I think an explicit call to if results == null would be better. Try are more costly than conditional in general, and anyway clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've since reverted this code since David said it was better to have it crash

Copy link
Contributor Author

@shaiguelman shaiguelman Jul 19, 2022

Choose a reason for hiding this comment

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

I changed it to use a nullable to avoid an if check

Copy link
Member

@david-allison david-allison Jul 19, 2022

Choose a reason for hiding this comment

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

I think I'd prefer a crash here (AKA no change), as it's an assertion which doesn't hold, but I'll leave the final decision to you. Implementer's choice, I'm happy either way =)

I think the answer to the question of 'what do we do if we get a null' is hard to pin down: do we want all results, or no results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that's a good question. I did no results since that was simpler to implement but it's not necessarily the best UX choice. It would require a sizeable refactor though to return all results.

I think since we're getting a little outside the scope of the PR I'll revert it to original for now and leave it for a non-urgent issue.

@lukstbit 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 Jul 21, 2022
@david-allison david-allison merged commit 54c8426 into ankidroid:main Jul 21, 2022
@github-actions github-actions bot removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge" labels Jul 21, 2022
@github-actions github-actions bot added this to the 2.16 release milestone Jul 21, 2022
@david-allison
Copy link
Member

Thank you!

@shaiguelman shaiguelman deleted the fix/crash_during_deck_filtering branch July 21, 2022 18:14
@github-actions
Copy link
Contributor

Hi there @shaiguelman! This is the OpenCollective Notice for PRs merged from 2022-07-01 through 2022-07-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself.

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

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 during deck filtering
4 participants