-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Handle panning touch gestures #5524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this! Since I don't have a touchscreen, it's hard to review. Some thoughts:
- How does text selection work on a touchscreen now? At least on mobile, it's common to have users hold for a short amount of time for the UI to transition into "text-selection" mode. I don't know how that's handled in web browsers on desktop, but that should be a source of inspiration.
For the future:
- It could be nice to be able to "flick" though tabs by swiping left and right (maybe restricted to swipes originating from the edges). This would also be nice to have for touchpads on laptops, since browsers allow that.
- Scrolling on a graphics tablet should also be possible, but that requires a bit more effort (especially since our scrollbar isn't a traditional one - Scrolling should behave more like browsers #3260).
void ChannelView::mouseMoveEvent(QMouseEvent *event) | ||
{ | ||
if (this->isPanning_) | ||
{ | ||
// Don't do any text selection, hovering, etc while panning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with mousePressEvent
? Does mousePressEvent
get called before? If so, it's worth checking that its state gets "reset", as it could set e.g. isLeftMouseDown_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried changing the mouse press and release event handling while panning, but it caused some selection issues when doing mouse movement after panning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks pretty good, will test this out on my touchscreen and give some more feedback.
Thank you for looking into this!
src/widgets/helper/ChannelView.cpp
Outdated
this->clearSelection(); | ||
break; | ||
default: | ||
this->isPanning_ = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is here to handle Qt::GestureFinished
or Qt::GestureCancelled
- I'd rather we handle those two states explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes these should be clearer now.
Just tested this out on my 4k touchscreen, it works fine but I feel like the speed at which it scrolls is wrong. This is most likely mostly related to #3262 that Nerixyz already mentioned above. |
Thanks for reviewing, text selection is handled with one finger movement on a touch screen, since Qt by default does a mouse click and move with it.
I tried working with some of the swipe gesture support, but it conflicts with the normal mouse event handling. Swiping would be nice, but it seems like it would look and feel better with a dedicated animation, etc. I also experimented with pinch to zoom the UI, but it feels a bit ugly. Not sure if it's specific to Qt or if there's something I'm doing wrong. Left it off this review.
Yeah, panning on the emote menus with this has the same issues as scrolling normally
I adjusted the pan scaling down a bit more, so it should feel a bit more natural now hopefully. The scroll speed setting is ignored for panning (unless I missed something). Would guess you wouldn't want the scroll speed to affect panning, since panning is expected to match finger movements as close as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed two small changes:
- ran formatter & made the switch look a bit more like the ones we normally write
- added
return true
to the handled gesture to indicate to qt that we've handled the event, and it does not need to be propagated further. I have not tested this but will assume it works
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.
This adds support for the panning gesture on
ChannelView
. This lets users who have touchscreens perform a two-finger pan of the chat (effectively a scroll).Mouse move events aren't handled while panning is occurring so that erroneous text selection when going into and out panning is avoided.