-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
feat: add shared chat badge #5661
feat: add shared chat badge #5661
Conversation
iProdigy
commented
Oct 19, 2024
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
technically we could reduce the number of helix requests made by using https://dev.twitch.tv/docs/api/reference/#get-shared-chat-session e.g., if shared chat has 6 channels, the current approach could make up to 5 helix requests while the alternate approach would just do 1 - but not a big deal in the context of the full app lifetime |
# Conflicts: # src/messages/MessageBuilder.cpp # src/messages/MessageBuilder.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nits, not tested yet - overall looks good. badge looks good
TODO for myself: test right-click actions on shared channel badges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying works as expected
I'm not fond of the badge being a mention element - I think I would prefer it to have no left-click action and instead a right-click action for some actions for the user (which I'm fine to leave out of this PR)
I got a crash with all settings set to default and then joined a channel with Shared Chat enabled. Another option could be that instead of a badge, we put the channel profile picture in front of the message. |
I'd rather that be part of a follow-up PR as a "polish pass", which would let us discuss it first too |
@8thony could you share the channel name (& the other channels that were in the shared chat session & whether you were also connected to any of those channels) also the stacktrace from the crash dump would be very helpful |
The Shared Chat Session had 2 People and I opened only one channel from those. StackStack: |
@8thony could you confirm the crash is now fixed? |
Yes, no crash anymore. |