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

Color mentions to match the mentioned users color #2284

Merged
merged 8 commits into from Dec 19, 2020
Merged

Color mentions to match the mentioned users color #2284

merged 8 commits into from Dec 19, 2020

Conversation

kiwec
Copy link
Contributor

@kiwec kiwec commented Dec 15, 2020

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

2020-12-15-223048_656x354_scrot

Fixes #1963
Fixes #2121

@TranRed
Copy link
Contributor

TranRed commented Dec 17, 2020

should there be a setting for it or do we force this? I'd personally prefer not to use it

@leon-richardt
Copy link
Collaborator

should there be a setting for it or do we force this? I'd personally prefer not to use it

I agree that this is a very "opinionated" change, it adds too much visual "clutter" for my liking. I believe there should definitely be a setting for this.

@kiwec
Copy link
Contributor Author

kiwec commented Dec 17, 2020

I should have explained that this change is not for visual fluff. I added it because it is necessary to be able to follow conversations in any reasonably large communities.

@ALazyMeme
Copy link
Collaborator

I should have explained that this change is not for visual fluff. I added it because it is necessary to be able to follow conversations in any reasonably large communities.

What yunglpr and tranred said is still applicable. Features that certain people want, others may not want. It should be an option, not a forced feature.

Copy link
Collaborator

@leon-richardt leon-richardt left a comment

Choose a reason for hiding this comment

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

I have some comments regardless of whether or not we eventually want a setting for this 😄

CHANGELOG.md Outdated Show resolved Hide resolved
src/common/ChannelChatters.cpp Show resolved Hide resolved
src/common/ChannelChatters.cpp Outdated Show resolved Hide resolved
src/common/ChannelChatters.cpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchMessageBuilder.cpp Outdated Show resolved Hide resolved
src/common/ChannelChatters.cpp Outdated Show resolved Hide resolved
src/common/ChannelChatters.cpp Outdated Show resolved Hide resolved
src/providers/twitch/TwitchMessageBuilder.cpp Outdated Show resolved Hide resolved
@kiwec
Copy link
Contributor Author

kiwec commented Dec 18, 2020

Thanks for the feedback. I added a setting and fixed the issues pointed out by leon.

Copy link
Collaborator

@zneix zneix left a comment

Choose a reason for hiding this comment

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

I like this feature and I think it's a nice addition overall 👍 Sorry for being a little pedantic over the code though 😛

Two more things:

  1. You can include Fixes #1963 \n Fixes #2121 in PRs body to link the issues and close them upon merging.
  2. What will happen when I send something like @asdasdasdasdasdasdasdxd? Chatterino will resolve this to a valid mention, but obviously such user doesn't exist - how would this feature behave in such cases?

CHANGELOG.md Outdated Show resolved Hide resolved
src/common/ChannelChatters.hpp Outdated Show resolved Hide resolved
src/common/ChannelChatters.cpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@pajlada
Copy link
Member

pajlada commented Dec 19, 2020

Looks good - the unresolved comments are still changes I'd like to see - after that I'll give it a whirl and we can get this merged in! Thanks a lot for contributing!

@kiwec
Copy link
Contributor Author

kiwec commented Dec 19, 2020

Pushed the requested changes. 🙂

  1. What will happen when I send something like @asdasdasdasdasdasdasdxd? Chatterino will resolve this to a valid mention, but obviously such user doesn't exist - how would this feature behave in such cases?

The default text color will be used (MessageColor::Text).

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Copy link
Collaborator

@zneix zneix left a comment

Choose a reason for hiding this comment

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

👍

@pajlada pajlada changed the title Color mentions (#1963) Color mentions to match the mentioned users color Dec 19, 2020
@pajlada pajlada merged commit fea52fa into Chatterino:master Dec 19, 2020
@fanway
Copy link
Contributor

fanway commented Dec 19, 2020

Does it continuously populate color cache without ever clearing it?

@fanway
Copy link
Contributor

fanway commented Dec 19, 2020

Sorry btw to write after it have been merged xd

@kiwec kiwec mentioned this pull request Dec 19, 2020
1 task
pajlada pushed a commit that referenced this pull request Dec 20, 2020
PR #2284 introduced this bug: whispers aren't linked to a twitch channel
but we're storing user colors in a twitch channel. So, dereferencing
a nullptr. Not good.
@fanway fanway mentioned this pull request Jan 9, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants