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

Added support for Twitch's Chat Replies #3722

Merged
merged 92 commits into from
Jul 31, 2022
Merged

Conversation

dnsge
Copy link
Contributor

@dnsge dnsge commented May 10, 2022

Description

Adds message reply thread support (https://help.twitch.tv/s/article/chat-basics?language=en_US#replies)

  • Only supported for TwitchChannels
  • Upon clicking the reply button, the user is presented with a reply dialog that shows the message thread and allows for typing responses with the same features as the standard input box (emote autocomplete, for example)
  • New replies are automatically added to the reply dialog similar to the user info dialog
  • /reply <username> <message> command sends a reply to the given user's most recent message

Design choices:

  • The message being replied to is rendered, but only the first line like in Twitch
  • The message being replied to does NOT render emotes and shows the raw text instead

The reply button

I initially tried only rendering the reply button when hovering a message, but the message rendering pipeline makes this pretty much impossible. The tiny reply button was the best compromise to having an accessible but not too intrusive option.

The reply buttons are not visible by default but can be enabled in settings if a user wants a single-click way to reply to a message.

Screenshots

Expand

Reply to message option:
Screen Shot 2022-05-29 at 10 41 27 PM

Inline message replying
Screen Shot 2022-05-29 at 10 41 39 PM

Message with reply:
Screen Shot 2022-05-29 at 10 41 59 PM

Message thread dialog:
Screen Shot 2022-05-29 at 10 42 05 PM
Screen Shot 2022-05-29 at 10 42 12 PM

Reply buttons enabled:
Screen Shot 2022-05-29 at 10 44 29 PM

Closes #1912

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

@dnsge dnsge marked this pull request as ready for review May 11, 2022 05:58
zneix
zneix previously requested changes May 11, 2022
@kornes
Copy link
Contributor

kornes commented May 11, 2022

From 30mins testing:

  • icons visible at all times are no go, too intrusive
  • icons don't scale well with zoom changes screen
  • window titles should include channel of origin
  • allowing single instance of reply window per target username would be cool to minimize mess
  • we shouldn't be allowed to reply own messages
  • full thread view (with replies from both sides) shouldn't include "Replying to" rows img2
  • i would make whole SingleLineTextElement clickable to open thread view for easier access

Overall looks good, but I think it would be better to remove icons/settings altogether from initial implementation in this PR. Context menu link to initiate reply and whole "Replying to @user" row clickable to answer, would be good in my book.

@Mm2PL
Copy link
Collaborator

Mm2PL commented May 11, 2022

  • allowing single instance of reply window per target username (or even per channel) would be cool to minimize mess

There is no limit to usercards, search windows. I don't see why this should have a limit.

  • we shouldn't be allowed to reply own messages

Why? IRC doesn't care about this. Webchat does, but webchat isn't the one-and-only chat experience.

@dnsge
Copy link
Contributor Author

dnsge commented Jul 23, 2022

random note: I noticed because of this PR that the Sending message as felanbird... text was not centered, this PR happens to center it which will be slightly annoying at first for people who do notice it, but is likely correct.

This was actually a bug that I did not notice... The HBox that contains the "Replying to" label was still taking up space while its children were not visible. I've now fixed that problem so the message input box looks the same as master when not replying.

@pajlada
Copy link
Member

pajlada commented Jul 24, 2022

This is the patch required to fix the tab/autocomplete focus lose issue:

diff --git a/src/widgets/helper/ChannelView.cpp b/src/widgets/helper/ChannelView.cpp
index 0b3459b3..09696c5b 100644
--- a/src/widgets/helper/ChannelView.cpp
+++ b/src/widgets/helper/ChannelView.cpp
@@ -163,7 +163,7 @@ ChannelView::ChannelView(BaseWidget *parent, Split *split, Context context)
     // and tabbing to it from another widget. I don't currently know
     // of any place where you can, or where it would make sense,
     // to tab to a ChannelVieChannelView
-    this->setFocusPolicy(Qt::FocusPolicy::StrongFocus);
+    this->setFocusPolicy(Qt::FocusPolicy::ClickFocus);
 }
 
 void ChannelView::initializeLayout()

I touched this place very recently and felt like something was wrong in the Focus Policy that was set for ChannelView. Would need to be tested a bit extra most likely but worked as expected for me on Arch Linux btw

@Felanbird
Copy link
Collaborator

Tab focus issue is resolved with the ClickFocus patch, focus setting should still be disabled by default as it does not make sense for an input box to be removed on loss of focus, as well as it's impossible to use the emote menu with that setting enabled.

@pajlada pajlada requested a review from Mm2PL July 30, 2022 09:58
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.

Awesome work!

Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

Found only a tiny visual bug, I'm fine with merging as is and fixing this at a later date.
With closing reply threads on lost focus:
2022-07-30_14-21

Without:
2022-07-30_14-21_1

@dnsge
Copy link
Contributor Author

dnsge commented Jul 30, 2022

Found only a tiny visual bug

This isn't actually a visual bug but an intentional change. When close-on focus loss is true, we use a frameless window. On non-linux platforms, we won't have any area to drag the popup around because the ChannelView and SplitInput take up the entire popup window. So instead, we introduce a margin to allow the user to drag the popup around.

https://github.com/dnsge/chatterino2/blob/2ea0d8c7c92836c84d363bc75275b647f4e8f7ab/src/widgets/dialogs/ReplyThreadPopup.cpp#L92-L93

We could guard this behavior in a macro because we don't do frameless windows on linux:

https://github.com/dnsge/chatterino2/blob/2ea0d8c7c92836c84d363bc75275b647f4e8f7ab/src/widgets/DraggablePopup.cpp#L11-L21

But like you said, we can do this at a later time. With the amount of code that was touched in this change, I'm sure we'll find more issues to fix once this change is on nightly.

Comment on lines +71 to +91
if (hasLocalizedName)
{
usernameText = localizedName;
}
else
{
usernameText = username;
}
}
break;

default:
case UsernameDisplayMode::UsernameAndLocalizedName: {
if (hasLocalizedName)
{
usernameText = username + "(" + localizedName + ")";
}
else
{
usernameText = username;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified with a ternary operator.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The code is just copy-pasted from another function since it needed to be reused, so not super in scope of this PR to fix it
  2. I would argue that using ternary operators here wouldn't necessarily make the code more readable

@@ -109,6 +137,7 @@ private slots:
void editTextChanged();

friend class Split;
friend class ReplyThreadPopup;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it needed here?

Copy link
Member

Choose a reason for hiding this comment

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

This is necessary for ReplyThreadPopup to be able to call giveFocus on the SplitInput

@pajlada pajlada changed the title Add Message Reply Threads Added support for Twitch's Chat Replies Jul 31, 2022
@pajlada pajlada enabled auto-merge (squash) July 31, 2022 10:29
@pajlada pajlada merged commit 20c974f into Chatterino:master Jul 31, 2022
@zneix
Copy link
Collaborator

zneix commented Jul 31, 2022

thanks for caring about me at all

@dnsge dnsge deleted the reply-threads branch July 31, 2022 21:51
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.

Add support for reply threads
9 participants