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

Fix selection clearing not working in Reply window #4218

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

kornes
Copy link
Contributor

@kornes kornes commented Dec 6, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Currently Reply window is missing behaviour of main chatterino window, where selection in splitInput and channelView cancels each other.

PR changes:

  • SplitInput will now expose SplitInput::selectionChanged signal, instead of doing any clearing locally
  • moved ChannelView::selectionChanged signal to Qt for consistency
  • use both signals in Reply window where it was missing

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

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

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.

Please undo the changes related to moved ChannelView::selectionChanged signal to Qt for consistency unless you have some other reasoning for it

@kornes
Copy link
Contributor Author

kornes commented Dec 7, 2022

Main reasoning for this change was to have handling of both signals use same solution since they are next to each other, personally i prefer Qt because its connect() better show both sides of connection and improve readability.
With Qt overall progress and Qt6 bringing new nice things like one-shot connections, imo we should encourage using native solution in new code and fallback to alternative only when there is benefit.
And finally, while not really in scope, this change doesn't have side effects as changed signal is used just in this 1 place.

If my excuses didn't convince you, im fine with reverting it, $notify me or let me know here :D

@pajlada
Copy link
Member

pajlada commented Dec 7, 2022

Yeah please undo the change for now, can review at some other point

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

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

@pajlada pajlada enabled auto-merge (squash) December 7, 2022 17:55
@pajlada pajlada merged commit a16d148 into Chatterino:master Dec 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

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

@kornes kornes deleted the fix_reply-window-select-clear branch December 8, 2022 19:04
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