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

Formalize zero-width emote implementation #4314

Merged
merged 38 commits into from
Mar 18, 2023

Conversation

dnsge
Copy link
Contributor

@dnsge dnsge commented Jan 18, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR attempts to fix the notion of a zero-width emote by introducing layered emotes: when one (or more) zero-width emotes follow a regular emote, the emotes are all gathered into a single LayeredEmoteElement which handles layered rendering through the LayeredImageLayoutElement and provides useful tooltips.

The TooltipWidget now has two "styles": a vertical layout, and a grid layout. If there are more than 2 emotes being shown at once in the tooltip, it will use the grid layout. Otherwise, the vertical layout will be used.

Images of changes (margins and spacing are slightly out of date as of 62edb0c)

Single zero-width emote:
Screen Shot 2023-01-18 at 8 57 04 PM

Multiple zero-width emotes:
Screen Shot 2023-01-18 at 8 57 18 PM

Excessive number of zero-width emotes:
Screen Shot 2023-01-18 at 8 57 33 PM

Context menu with zero-width emotes:
Screen Shot 2023-01-18 at 9 00 48 PM

Also adds a "Enable zero-width emotes" setting (default: true). If off, zero-width emotes are shown as regular emotes.

Fixes #3615
Fixes #2994

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@dnsge dnsge marked this pull request as ready for review January 19, 2023 02:27
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@Felanbird Felanbird linked an issue Jan 19, 2023 that may be closed by this pull request
@Felanbird
Copy link
Collaborator

Functionality looks good from brief testing, will use this build for the week and see if I run into any issues 👍

Also as noted by my linked-issue, this PR resolves the issues with animated BTTV emotes rendering on-top of zero-width emotes

image

@Felanbird
Copy link
Collaborator

Felanbird commented Jan 19, 2023

First thing I've noticed is the general tool-tip size is larger, much more noticeable by the space in-between the emote info and the bottom

Before:
image
image

Now:
image
image

note edit: Just so it's known this large tool-tip thing applies to stream titles as well
image

@dnsge
Copy link
Contributor Author

dnsge commented Jan 19, 2023

general tool-tip size is larger

Should be fixed now in 62edb0c

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@kornes kornes left a comment

Choose a reason for hiding this comment

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

very nice feature, gonna see how it runs for few days 👍

src/widgets/TooltipEntryWidget.hpp Outdated Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

general tool-tip size is larger

Should be fixed now in 62edb0c

Issue is resolved, there's a slight miscentering when there's exactly 2 zero-width emotes, but I'd take that over the huge tool-tips any day 👍

Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

I believe this works as expected 👍 - I actually feel like I've seen less zero-width emotes since I started using this PR, but it's much more likely less are being used than something being broken.

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.

I've taken a bit of a glance at the code, overall looks fine - mostly nitpicks in my review.
Thank you for looking into this, this feels like an overall better & more consistent approach!

src/messages/MessageBuilder.hpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Outdated Show resolved Hide resolved
src/messages/MessageElement.cpp Outdated Show resolved Hide resolved
src/widgets/helper/ChannelView.cpp Outdated Show resolved Hide resolved
src/widgets/helper/ChannelView.cpp Outdated Show resolved Hide resolved
src/messages/MessageElement.hpp Show resolved Hide resolved
src/messages/MessageElement.hpp Show resolved Hide resolved
src/messages/MessageElement.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a 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

There were too many comments to post at once. Showing the first 25 out of 53. Check the log or trigger a new build to see more.

src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/widgets/TooltipEntryWidget.cpp Show resolved Hide resolved
src/widgets/TooltipEntryWidget.cpp Show resolved Hide resolved
@dnsge dnsge requested a review from pajlada March 2, 2023 05:45
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.

@dnsge If you could take a last look at your own code & the changes I've made before I merge this in that would be great - seems to work as expected for me with tooltips & everything on both X11 & Sway

Copy link
Contributor

@github-actions github-actions bot left a 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

There were too many comments to post at once. Showing the first 25 out of 35. Check the log or trigger a new build to see more.

src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/MessageElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
@dnsge
Copy link
Contributor Author

dnsge commented Mar 18, 2023

@pajlada I just went through your changes and over the full PR again, everything looks good to me!

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.

Tooltips for zero-width emotes are inaccessable Animated emotes renders on top of emote modifiers.
4 participants