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

Added incremental search to channel search. #3544

Merged
merged 5 commits into from
Feb 13, 2022

Conversation

qooq69
Copy link
Contributor

@qooq69 qooq69 commented Jan 26, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Added incremental search to channel search. Can be toggled in settings under Use incremental search.

@Infinitay
Copy link
Contributor

Thoughts on having this the default search method to also match emote and tagging auto-completion?

@LosFarmosCTL
Copy link
Contributor

I feel like this should be the new default behavior instead of a setting.

If the setting stays though, it should not be called "Incremental search", thats more of a technical term and most users would probably not know what it does. Something in the lines of "Show search results while typing" would be more comprehensible.

@qooq69
Copy link
Contributor Author

qooq69 commented Jan 27, 2022

My only concern if this becomes the default behavior is that it might be too taxing on low end computers and might lag users.

Regarding the name, my thought process was that people can google the term if they don't fully understand it, though I'm not opposed to changing it.

@LosFarmosCTL
Copy link
Contributor

My only concern if this becomes the default behavior is that it might be too taxing on low end computers and might lag users.

I wouldn't expect this to be especially taxing on your systems resources tbh, but yeah couldn't hurt to test it in a crappy VM or something.

Regarding the name, my thought process was that people can google the term if they don't fully understand it, though I'm not opposed to changing it.

Having to look up what the name of a setting means is definitely not a good UX.

@sando
Copy link
Contributor

sando commented Jan 27, 2022

I like it as a setting, 100% agree that "Show search results while typing" is more intuitive.

@kornes
Copy link
Contributor

kornes commented Jan 27, 2022

i would vote to make it default behavior and remove setting checkbox, it wont be more taxing than search in few hundred emotes/emojis which is currently the case and had no negative feedback

@Felanbird
Copy link
Collaborator

I'm fine with implementing this as the new default, as long as it's tested in some way on a low-end machine/vm as there are alot of Chatterino users in recent times without the best computers.

Otherwise a setting is fine, I'm aware we have alot of settings without "advanced" sorting but that doesn't mean we should always stray away from user(s) customization.

but change the setting name cuz the current one sucks.

@LosFarmosCTL
Copy link
Contributor

LosFarmosCTL commented Feb 6, 2022

I just tested this PR inside of a 1 CPU Core 2GB RAM crappy VM on my old laptop. Chatterino itself was a bit of a pain to use, but the search did not cause any problems whatsoever and worked absolutely flawless without any lags, hiccups, whatsoever.

So I would strongly advocate for this to be the default behaviour and not another new setting, since I can't really see any reason why someone would need the old behaviour.

@Mm2PL Mm2PL self-requested a review February 6, 2022 22:28
@pajlada
Copy link
Member

pajlada commented Feb 12, 2022

I agree with @LosFarmosCTL 's comment - removing setting and keeping this default is the right way to do this.
If you want to be fancy, you could add a benchmark!

@qooq69 qooq69 force-pushed the add-incremental-search branch from 02a2171 to f5c66af Compare February 12, 2022 15:34
@pajlada
Copy link
Member

pajlada commented Feb 13, 2022

@Mm2PL Merge in when you're happy!

Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

Looks good to me. Maybe we should look at completion/suggestions of parsing search predicate later.

@Mm2PL Mm2PL changed the title Added incremental search to channel search (Ctrl + F) Added incremental search to channel search. Feb 13, 2022
@Mm2PL Mm2PL merged commit 82196e3 into Chatterino:master Feb 13, 2022
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.

8 participants