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(mobile): fix text search #14873

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnstef99
Copy link
Contributor

Fixes #14814

Copy link
Contributor

Label error. Requires exactly 1 of: changelog:.*. Found:

if (isContextualSearch.value) {
filter.value = filter.value.copyWith(
filename: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

if you don't send null, the search will be incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

originalFileName:
filter.filename != null && filter.filename!.isNotEmpty
? filter.filename
: null,

Copy link
Contributor

@alextran1502 alextran1502 Dec 22, 2024

Choose a reason for hiding this comment

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

What do you reference there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where the filename filter is being used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the empty string is necessary because if you copyWith null you will get the previous value

Copy link
Contributor

Choose a reason for hiding this comment

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

They should be set null. I'm not sure if the server treats an empty string like null in this case, and in general a lack of a value should be represented with null instead of an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mertalev the server doesn't get an empty string, it gets null, read the referenced code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what I mean when I said

The SearchService handles it by checking if the strings context and filename are empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. It seems like a weird limitation to not be able to change an earlier value to null. Not sure if there's a better way to handle that, but I guess this is okay for now if there isn't.

@johnstef99
Copy link
Contributor Author

This commit f59b813 changed how SearchFilter.copyWith handles null value for context and filename but this created this bug #14814 where when you apply other filters the context and filename always get turned into null.

Passing empty string at the context or the filename filter is necessary to remove the filter when the user submits the search field with no text. The SearchService handles it by checking if the strings context and filename are empty.

I think this answers both of the above questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS Search bug
3 participants