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

Add setting to prevent or highlight message overflow #3418

Merged
merged 20 commits into from
Nov 13, 2022

Conversation

acdvs
Copy link
Contributor

@acdvs acdvs commented Dec 23, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Native Twitch chat prevents the user from typing past 500 characters entirely. Right now, Chatterino only enforces this rule by preventing 501+ characters from being sent. This adds a setting to prevent or highlight input overflow to match the native Twitch functionality.

My motivation to add this comes from some frustration in wanting to send a long message but having to manually delete characters until only 500 were left.

Prevent overflow

overflow_prevent

Highlight overflow

overflow_highlight

@leon-richardt
Copy link
Collaborator

I would certainly like some sort of indication why text input is disabled. (Note that this criticism applies to the current mechanism of failing silentyl too!)
Perhaps using a red background for the input box as well as a message above the input?

Just spitballing here, there might be better solutions.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Dec 23, 2021

Perhaps using a red background for the input box as well as a message above the input?

This already happens with the character counter

2021-12-23_15-02

@acdvs
Copy link
Contributor Author

acdvs commented Dec 23, 2021

@Mm2PL I think the concern there may be that visual feedback needs to be on the input text itself. Maybe something similar to what Twitter does:
image

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.

This PR does not respect our character counter logic, please make change it to take that into account.

It's still possible to paste messages from the emote popup and paste text via Ctrl+V and right-click menu. I personally don't like the way you achieved this feature with whitelisting keys that could be pressed. IMO It's way better to use a character inserted or text changed event.

This should let me insert more spaces because spaces at the end shouldn't matter:
2021-12-23_17-25

@acdvs
Copy link
Contributor Author

acdvs commented Dec 23, 2021

I've updated it to use a textChanged slot instead of a keyPress event which handles both typing and pasting all in one. This still won't allow extra spaces at the end as you mentioned, but I strongly suggest that shouldn't be possible because it makes the UX side of character limitation unclear.

I've also gone ahead and added a red highlight option to this setting (similar to what I mentioned above) because it's another good way of solving the core issue all in one setting.

@acdvs acdvs changed the title Add setting to limit message input length Add setting to prevent or highlight message overflow Dec 24, 2021
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.

Pull request looks nice, I like the idea of letting user know what part of the message won't go through anyway. Got a couple things to point out:

  1. Name of the setting in Settings dialog (Message overflow) isn't very descriptive to an average end user imho. I can't really come up with a better naming, but would like to hear others' opinions about it. cc @Felanbird
  2. I think that Highlight option makes sense to be set to as a default setting value instead of Allow - bringing in new functionality that isn't enabled by default makes it difficult to discover by users; and I think this isn't anything harmful to set by default.

src/widgets/splits/SplitInput.cpp Outdated Show resolved Hide resolved
src/singletons/Settings.hpp Outdated Show resolved Hide resolved
src/widgets/helper/ResizingTextEdit.cpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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.

I've got an issue with the feature as is now. If you type out a very loooong message, change the setting to Prevent. And then type a character into the filled input will lag Chatterino for a good while. From my testing it turns out that it's because the text changed event triggers itself recursively. With enough characters Chatterino will segfault (~12k is enough), again probably because of a stack overflow. While this is not really a very bad issue per se, I'm thinking that it's a sign of a mistake somewhere. A possible solution would be to take the input text, clip it to 500 chars and then set it back to the input, leaving only one recursive text changed event which we can live with. Another solution would be to have some kind of a locking mechanism to make sure that the recursively generated events are safely ignored.

Sorry for the pedantic review.

Fun fact: bt in my gdb is still going as I'm correcting/expanding this message.

src/widgets/splits/SplitInput.cpp Outdated Show resolved Hide resolved
src/widgets/splits/SplitInput.cpp Outdated Show resolved Hide resolved
src/widgets/splits/SplitInput.cpp Outdated Show resolved Hide resolved
src/widgets/splits/SplitInput.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/helper/ResizingTextEdit.cpp Outdated Show resolved Hide resolved
src/widgets/helper/ResizingTextEdit.cpp Outdated Show resolved Hide resolved
@acdvs
Copy link
Contributor Author

acdvs commented Dec 25, 2021

I appreciate the feedback! I'll make changes soon.

@zneix I spent longer than I'd like trying to find better wording for this. Originally I was using "Input overtype behavior" but that seemed too wordy.

@acdvs
Copy link
Contributor Author

acdvs commented Dec 28, 2021

@zneix I've changed the default to Highlight based on your comment. It can only be beneficial to the UX as a purely visual feature.

@Mm2PL The Prevent behavior now trims the whole input via QString::left instead of per-character with a cursor. This also means no extra checks are needed to prevent excessive recursion. SplitInput::editTextChanged will retrigger just once, though this is probably a good thing because the first pass trims the text and the second pass will then process any other checks like /r replies.

@ALazyMeme
Copy link
Collaborator

So after my testing on fecba0e:

  • The PR tries to enforce the highlighting on IRC channels, which isn't correct (I believe it's 512 characters?)
  • The "prevent" option doesn't work in IRC

Not sure if you're able to add a separate limit for IRC or something? Then would also need the prevent option fixed.

Otherwise it seems to work as intended.

@acdvs
Copy link
Contributor Author

acdvs commented Oct 30, 2022

Apologies for leaving this hanging for so long. Is anyone else able to figure out a good fix for ALazyMeme's feedback? My knowledge is limited.

@pajlada
Copy link
Member

pajlada commented Oct 30, 2022

I fixed the merge conflicts and fixed the issues ALazyMeme mentioned, right now I only have two concerns that I can get to myself later:

  1. The setting should, imo, be less accessible. It should be hidden under Advanced somewhere as I think the default "Highlight" is going to be good/preferred for 99% of users. The text for the setting should probably be something different too, although naming things is hard so I can't come up with a direct improvement yet!
  2. With the Highlight setting, it's possible to get into the state where the message you're typing is completely red. This is achieved by overflowing your messagebox, selecting all text, then starting to type
2022-10-30.20-38-37.mp4

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/widgets/splits/SplitInput.hpp Show resolved Hide resolved
@pajlada pajlada enabled auto-merge (squash) November 13, 2022 11:26
@pajlada pajlada merged commit a9d3c00 into Chatterino:master Nov 13, 2022
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.

6 participants