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

Add option always include broadcaster in user completions #5193

Merged
merged 9 commits into from
Feb 24, 2024

Conversation

KleberPF
Copy link
Contributor

Settings text probably needs to be improved.

Setting off:

image
image

Setting on:

image
image

I noticed we got some unit tests for ChatterSet, so let me know if I should add any.

Fixes: #5190

Copy link
Contributor

@Nerixyz Nerixyz 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! Maybe the setting name could be a bit shorter with some tooltip text, but I can't think of anything good right now 🙃

If you want, you could add a test case (maybe testing the case-sensitivity).

src/common/ChatterSet.cpp Outdated Show resolved Hide resolved
src/common/ChatterSet.cpp Outdated Show resolved Hide resolved
src/common/ChatterSet.hpp Outdated Show resolved Hide resolved
@pajlada
Copy link
Member

pajlada commented Feb 21, 2024

The "at the top" isn't important to me, just that the streamer is included. So for the variable names & the setting labels that should be reflected (e.g. "always include broadcaster in user completions" or something)

@KleberPF
Copy link
Contributor Author

Changed the setting name (might be too big now but I feel like it's reads well)

@KleberPF
Copy link
Contributor Author

KleberPF commented Feb 23, 2024

@pajlada ended up using find_if, erase and insert, everything seems to be working. It doesn't seem too hard to add InputCompletion unit tests, just mock accessChatters and create a ChatterSet using preset names, but I'm a Visual Studio guy and CMake is kicking my ass. I wasn't able to figure out how to compile the tests, so if you could give me some pointers on how to do that I'd appreciate it a lot (of course, feel free to add the tests yourself if you want)

@pajlada pajlada changed the title Add option to always have broadcaster at the top of mentions popup Add option always include broadcaster in user completions Feb 24, 2024
@pajlada pajlada enabled auto-merge (squash) February 24, 2024 12:57
@pajlada pajlada merged commit 86111d5 into Chatterino:master Feb 24, 2024
17 checks passed
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.

@ing tab autocomplete to streamer username
4 participants