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

Improve Appearance of Reply Curve #4077

Merged
merged 5 commits into from
Nov 2, 2022

Conversation

dnsge
Copy link
Contributor

@dnsge dnsge commented Oct 22, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Improves the overall appearance of the reply curve. Related: #4009

Curve drawing parameters are defined here and can be tuned if people feel that some other value set would look better:
https://github.com/dnsge/chatterino2/blob/cf1c994cb4c5bae40c27af172e233965e84ca016/src/messages/MessageElement.cpp#L687-L690

New Appearance:
Screen Shot 2022-11-01 at 11 00 49 PM

The vertical expanding nature of the reply curve has been removed from this PR. I will create another PR for the logic.

Description Outdated

Improves the overall appearance of the reply curve. Related: #4009

  1. Uses linear horizontal and vertical parts with a curve in the top left (much like discord)
  2. Fully extends downwards to the element below

Screen Shot 2022-10-21 at 9 13 38 PM

The vertical growth works as follows:

  1. All elements are laid out, as usual, line by line
  2. We iterate through every MessageLayoutElement and find every instance of VerticalExpandingMessageLayoutElement
  3. For each VerticalExpandingMessageLayoutElement, if there is a line below it, we find the top of the elements directly beneath it
  4. We then increase the size of the current element to touch the element(s) beneath it

This screenshot + drawing sort of show the process:
Screen Shot 2022-10-21 at 9 17 18 PM

We identify the elements that are below the reply curve element and consider their bounding boxes. In this example, we don't need to worry about the two badges because they aren't directly below the reply curve. We then grow the bottom of the reply curve to meet the top of the timestamp.

The top of our reply curve remains unmoved; when we constructed the reply curve element, we gave it a zero height and told the MessageLayoutContainer to center it vertically in the line (new to this PR).

If we manually increase the width of the reply curve, we can see how the curve chooses the tallest element beneath it:
Screen Shot 2022-10-21 at 8 37 53 PM

@dnsge dnsge marked this pull request as ready for review October 22, 2022 01:35
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

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.hpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.hpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutElement.hpp Show resolved Hide resolved
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.

The curve looks great, but the change to compact emotes needs to be undone. It's fine if the setting is removed, since it's on by default and unconfigurable but the logic for compact emotes must stay.

2022-10-22.11-17-11.mp4

@dnsge dnsge force-pushed the feat/better-reply-curve branch from c7a74b5 to 5f61cec Compare October 22, 2022 18:19
@dnsge
Copy link
Contributor Author

dnsge commented Oct 22, 2022

The change to compact emotes needs to be undone

I returned the compact emote rendering logic. Didn't realize that the setting was enabled by default 😅

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

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
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 feel like this change is a bit too complex for what it gives, let's maybe start with only changing the curve of the line and work on the layout changes at another point - Sorry for the back-and-forth

The compactEmotes setting removal can still stay

@dnsge dnsge force-pushed the feat/better-reply-curve branch from 5f61cec to cf1c994 Compare November 2, 2022 03:10
@dnsge
Copy link
Contributor Author

dnsge commented Nov 2, 2022

I've gone ahead and removed the vertically expanding element stuff so that this PR now only includes adjustments to the shape of the reply curve itself.

New Appearance:
Screen Shot 2022-11-01 at 11 00 49 PM

If anyone is interested in playing around with the curve parameters, you can modify the constants here:
https://github.com/dnsge/chatterino2/blob/feat/better-reply-curve/src/messages/MessageElement.cpp#L687-L690

@pajlada
Copy link
Member

pajlada commented Nov 2, 2022

Thank you! 🙇

@pajlada pajlada merged commit 7640677 into Chatterino:master Nov 2, 2022
@dnsge dnsge deleted the feat/better-reply-curve branch November 5, 2022 04:52
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.

2 participants