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

Fixes to ctrl+backspace behavior #5013

Merged
merged 11 commits into from
Dec 16, 2023
Merged

Fixes to ctrl+backspace behavior #5013

merged 11 commits into from
Dec 16, 2023

Conversation

gremble0
Copy link
Contributor

@gremble0 gremble0 commented Dec 10, 2023

Description

First contribution to chatterino, so hello :)

This PR addresses issue #5004 + fixes some issues related to that issue

Changes

Added ctrl+backspace functionality while there is an active selection to EmotePopup, SettingsDialog and SelectChannelDialog classes, as well as a small change (fix) to ctrl+backspace behavior in and SearchPopup, which is also the behavior implemented in the other classes.

Previously the behaviour for ctrl+backspace with an active selection was to clear all characters from the end of the selection to start of line (or the behavior described in the issue for the classes with it not implemented). This has been changed to ctrl+backspace deleting only the selected text, which is the behavior present in all other applications i tested it in.

Old behavior:
https://github.com/Chatterino/chatterino2/assets/45577341/160dd1e4-f46e-43aa-826e-a827dc264908

New behavior:
https://github.com/Chatterino/chatterino2/assets/45577341/8e2a4e4f-92e1-4f67-a54d-3d0544e1c7d9

It may be possible make a more generic solution to this issue than to add these eventFilter() methods wherever there is a input field, but as of yet i'm not familiar enough with QT and the chatterino codebase to do that.

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/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/SettingsDialog.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/SettingsDialog.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

This is probably a bug in Qt. I couldn't find a bugreport after a quick search.

The bug is in QWidgetLineControl::processKeyEvent. This calls cursorWordBackward, which removes the selected word from the selection. Calling del will only delete the remaining selection.

src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/SelectChannelDialog.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/SettingsDialog.cpp Outdated Show resolved Hide resolved
src/widgets/helper/SearchPopup.cpp Outdated Show resolved Hide resolved
@gremble0
Copy link
Contributor Author

Yeah, i supposed this sounds like something chatterino probably shouldn't have to deal with. Anyways I've added your suggestions which will fix the behavior until it potentially gets fixed upstream. Until then i suppose it's up to you what you want to do in chatterino.

@pajlada
Copy link
Member

pajlada commented Dec 16, 2023

@Nerixyz Does it make sense to merge this in the interim, while waiting for your fix to Qt to be released?
I'm guessing yes since this fix is small enough that it would be easy to revert once we're on a fixed Qt release

@Nerixyz
Copy link
Contributor

Nerixyz commented Dec 16, 2023

@Nerixyz Does it make sense to merge this in the interim, while waiting for your fix to Qt to be released? I'm guessing yes since this fix is small enough that it would be easy to revert once we're on a fixed Qt release

Yes, we should merge this. The fix should be in 6.6.2 (to be released on 17.01.2024). But, at least on Windows, we're still waiting on 509236.

@pajlada pajlada enabled auto-merge (squash) December 16, 2023 13:20
@pajlada
Copy link
Member

pajlada commented Dec 16, 2023

Thank you! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at the top for instructions.

@pajlada pajlada merged commit b78b57b into Chatterino:master Dec 16, 2023
19 of 20 checks passed
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.

None yet

3 participants