Touch support for Gwenview
ClosedPublic

Authored by steffenh on Jul 5 2018, 11:06 AM.

Details

Summary

Add touch support for Gwenview. At moment one finger scrolling, pinch gesture for zoom and rotate and one finger
swipe gesture to go to next or previous image is working with some little issue.

It is my first time I am modify the source code, so if I make same mistake please let me now.

CCBUG: 378021

Test Plan

Browse mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • on finger scrolling, test if uwanted drag events or selection events occur
  • two finger scrolling, test if uwanted drag events or selection events occur
  • test drag and drop action (need a TabAndHold gesture before moving the touch point)
  • pinch gesture for zoom, check sensitive and unwanted behavior
  • check double touch, if system setting is set to: open file or directory with a double click

View Mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • one finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • pinch gesture for zoom and rotate, check sensitive and unwanted behavior
  • drag and drop action (need a TabAndHold gesture before moving the touch point)
  • test double touch to toggle between view mode and fullscreen mode.

Thumbnail bar in view Mode:

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger scrolling, test if uwanted drag events or selection events occur
  • drag and drop action (need a TabAndHold gesture before moving the touch point)

Fullscreen Mode

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger panning, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • one finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger swipe gesture, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • pinch gesture for zoom and rotate, check sensitive and unwanted behavior
  • drag and drop action (need a TabAndHold gesture before moving the touch point)
  • check if you get access to the fullscreen thumbnail bar on the top screen border

Thumbnail bar in fullscreen Mode:

  • test, if mouse and touchpad working as previously: scrolling, selection and drag and drop

all follow tests with touchscreen:

  • one finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • two finger scrolling, check if unwanted drag events or image modifying events (zoom/rotate) occur
  • drag and drop action (need a TabAndHold gesture before moving the touch point)

Diff Detail

Repository
R260 Gwenview
Branch
mytouch
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 611
Build 623: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
rkflx added inline comments.Aug 27 2018, 6:46 AM
app/folderviewcontextmanageritem.cpp
94

Q_DECL_OVERRIDEoverride

(28754fa2d95e changed this globally)

94

QEvent* event is the preferred style for Gwenview.

(I won't add the same comment again, but please change all other places accordingly.)

101–106

I wonder where that magic 10 comes from?

In general this code block reads like you want to catch a regular tap. Would using QTapGesture work too?

103–105

Not strictly necessary, but adding const would make it clear that those variables are just a shorthand and are not intended to be changed.

104–105

at(0)first() reads a bit more fluently

(again, see below for more occurrences of this)

106

Add spaces around <

210–213

Separate these lines with a newline to group the code into logical blocks and thus make it more readable.

211

Only actual member variables should get an m prefix.

212

There should be enough visual and hard disk space for not having to abbreviate Prop, i.e. rename to scrollerProperties.

213–214

Add space after commas.

213–214

Always use enum values by name, e.g. QScrollerProperties::OvershootAlwaysOff instead of 1.

216

Since TouchGesture is the default value for the second argument, it can be left out.

217

Remove extra space before ).

app/startmainpage.cpp
165

This breaks reordering by dragging in the places panel for mouse users, so I don't think this can be removed.

166–171

This duplicates a lot of code from FolderViewContextManagerItem, so you might think about putting this into a separate function taking the viewport as a parameter (in app, but see lib/*utils* for some examples).

lib/documentview/documentview.cpp
118–126

Member variables should be prefixed with a lowercase m.

Also, some of the names are too generic, e.g. timestamp in the context of DocumentView is not descriptive enough.

That said, I wonder why all of those variables are really necessary. Is there no Qt mechanism to differentiate between different gesture and to get at timestamps or positions of gestures?

120

It's Tap, not Tab (applies everywhere).

442

Remove extra space before (.

686

I did not review event, gestureEvent and the corresponding functions in ThumbnailView in detail yet, but some general comments:

  • For a general event function, there is way too much gesture code in there. This should perhaps be split up a bit (see Qt's gestures example, 9d90a08ef5e2, or 618072f8a2ea).
  • Why is there so much code? Is there no support in Qt to detect that kind of gestures, e.g. any of QGesture's children like QTapAndHoldGesture, QPanGesture etc.? Would it make sense to subclass QGestureRecognizer if not?
  • Some parts could use more comments to make the purpose of some of the code easier to understand.
1001

Space before { missing.

1003

Is this still needed given the check above?

lib/documentview/documentview.h
212

override

213

Remove extra space after (.

213

Better put this into the private section?

lib/thumbnailview/thumbnailview.cpp
701

This also seems to duplicate code, but again, is there any mechanism in Qt for handling this instead of having to code so much manual handling of the touch events?

830

So essentially this is a bool? Also, what is it needed for?

832

Use qBound.

Restricted Application added a project: Gwenview. · View Herald TranscriptAug 27 2018, 6:46 AM

Hi @rkflx,

Thank you for your effort, to look over my code.

Sorry for the code style error, I'll correct it.

I know, I have the same or equal code on more than one position, I was thinking about moving the code in a own class. But I'm afraid I don't know enough in Qt and C++ to make it. I will try it, but please bear with me, if I make a complete mess out of this.

Unfortunately, Qt does not differentiate well between individuals gesture typs. For example, if you make a two finger pan gesture, to scroll the viewport, you will get a QGesture Event with a pan gesture, zoom gesture, rotation gesture and so on in the same time. So this is the reason for the many code in the TouchBegin, TouchUpdate and TouchEnd Event handler, I need this to differentiate between the individuals gesture.
You have linked some example, but all have the same problem, you can not make a pure two finger pan gesture without zoom or rotate effekts.
I will try to make my own QGesture Class, but as I said above, I hope I don't make a mess.

The next behavior from Qt, which sometimes causes me problems, is that Qt synthesizing a mouseclick from a touch event. This is the reason for the check on "mouseEvent->source() == Qt::MouseEventNotSynthesized)", I need to suppress this. Without the check I will start a DragandDrop action.

Yes I have some magic numbers in the code, should I insert a comment to explain the number or make a const variable with a meaningful name or both?

lib/thumbnailview/thumbnailview.cpp
830

I know it is a little bit crude, the aim was, to lower the sensitivity of the zoom. I wanted to use only every second gesture event to zoom the size of the thumbnail. Because the thumbnail size is an integer, I cannot modify the size in 0.5 steps.
Maybe I need to save the start size of the thumbnail in the zoom begin, and use this to calculate the next size, I need to test this.

Sorry, forgot about your question here.
I have no touch hardware too but try to help out with the code part.

Yes I have some magic numbers in the code, should I insert a comment to explain the number or make a const variable with a meaningful name or both?

I think a meaningful constant is enough in most places where an explanation is needed, especially if the value is used more than once. A comment is good to have a clue what's the sense of the following code. I would not comment on a single constant in the normal code (if needed maybe at the constant definition).

@muhlenpfordt, I can take point on the behavioral part of the review, but would appreciate your code expertise for the code review!

steffenh updated this revision to Diff 42128.Sep 22 2018, 1:56 PM
steffenh marked 3 inline comments as done.

The patch has completely rewritten, to implement changes based on review comments.

  • moving QScroller code to lib/scrollerutils.cpp
  • moving most of the touch handling code to extra gestures classes (lib/touch/*.*)
  • smoothing the zoom in view mode
  • add a double tap gesture for the context menu in view and browse mode
  • some code style errors corrected

Wow, pinch zooming is hugely improved with this new version. Nice work!

Here are the problems I discovered:

  • Touch swipe left and right in View mode now moves too many to the left or right, instead of just going to the next or previous image
  • Two-finger pinch zoom sometimes rotates the image by accident again. Rotation should only happen when one of the fingers remains still
  • I'm seeing a weird effect when double-tapping to enter Full screen mode from View mode:

Other than this, the behavior is good.

Hi @ngraham

Rotation should only happen when one of the fingers remains still

I am not sure, that I agree with you in this matter, because I move both fingers if I want to rotate the image.

  • I'm seeing a weird effect when double-tapping to enter Full screen mode from View mode:

I can not reproduce this at the moment, perhaps you can post your linux distribution, KDE and Qt version, so I can install it on a USB stick and look at this.

Hi @ngraham

Rotation should only happen when one of the fingers remains still

I am not sure, that I agree with you in this matter, because I move both fingers if I want to rotate the image.

Hmm, in that case, it will be important to improve rotation detection, because I repeatedly rotated images by accident while pinch-zooming. Coupled with the lack of animation, it was very jarring and unexpected.

  • I'm seeing a weird effect when double-tapping to enter Full screen mode from View mode:

I can not reproduce this at the moment, perhaps you can post your linux distribution, KDE and Qt version, so I can install it on a USB stick and look at this.

Kubuntu 18.04 with Plasma 5.12.6 and Qt 5.9.5. A bit old, I know...

steffenh updated this revision to Diff 42537.Sep 29 2018, 5:52 AM

Fix some issues

  • fix issue with swipe gesture
  • fix issues with the double tab gesture to toggle Fullscreen mode
  • change handling for rotating gesture, to ignore very low and high rotation angle changes
  • fix a problem with tabholdandmoving gesture (drag and drop), if the image was modified.
sander added a subscriber: sander.Jan 9 2019, 10:34 AM

Some of these gesture recognizers would be very helpful in other KDE programs as well (Okular is what I have in mind). Can they be moved to some central place where both Gwenview and Okular can use them?

@steffenh thanks for your patience here, and I'm sorry this hasn't landed yet. Could you rebase it on current master please? I'll redouble my efforts to test and rustle up some code reviewers for you.

ngraham accepted this revision.Feb 21 2019, 7:35 PM

Thanks @steffenh. I went back and re-tested everything and it all works *flawlessly* for me. Very impressive work. I really hope we can get it in for KDE Applications 19.04. I'll start on the code review:

From a high level perspective, I share @rkflx's concern with the amount of duplicate code. ThumbnailView::viewportEvent() is almost entirely duplicate code from DocumentView::event(), for example. Can we refactor this into re-usable functions in touch or touch_helper or something?

Also, can you help me understand why we need to implement our own finger gesture recognizers in all the files you've added under lib/touch? Can we not use Qt's built-in touch handling functionality for some reason?

lib/documentview/documentview.cpp
788

Should probably be sensitivityModifier

1001

Which older versions? If it's older than 5.9, we don't need this.

lib/scrollerutils.cpp
1 ↗(On Diff #52190)

Please remove

4 ↗(On Diff #52190)

Your copyright

lib/scrollerutils.h
1 ↗(On Diff #52190)

Can you remove this? None of the other files have it.

4 ↗(On Diff #52190)

Since you wrote this file, it's your copyright! :)

lib/touch/doubletap.cpp
1 ↗(On Diff #52190)

Please remove

4 ↗(On Diff #52190)

Your copyright

lib/touch/doubletap.h
3 ↗(On Diff #52190)

Your copyright

lib/touch/oneandtwofingerswipe.cpp
1 ↗(On Diff #52190)

Please remove

4 ↗(On Diff #52190)

Your copyright

lib/touch/oneandtwofingerswipe.h
3 ↗(On Diff #52190)

Your copyright

lib/touch/tapholdandmoving.cpp
1 ↗(On Diff #52190)

Please remove

4 ↗(On Diff #52190)

Your copyright

lib/touch/tapholdandmoving.h
3 ↗(On Diff #52190)

Your copyright

lib/touch/touch.cpp
1 ↗(On Diff #52190)

Please remove

4 ↗(On Diff #52190)

Your copyright

lib/touch/touch.h
3 ↗(On Diff #52190)

Your copyright

lib/touch/touch_helper.h
3 ↗(On Diff #52190)

Your copyright

40 ↗(On Diff #52190)

Typo: wiggel -> wiggle

lib/touch/twofingerpan.cpp
3 ↗(On Diff #52190)

Your copyright

lib/touch/twofingerpan.h
3 ↗(On Diff #52190)

Your copyright

lib/touch/twofingertap.cpp
3 ↗(On Diff #52190)

Your copyright

lib/touch/twofingertap.h
3 ↗(On Diff #52190)

Your copyright

This revision is now accepted and ready to land.Feb 21 2019, 7:35 PM

Hi @ngraham,

From a high level perspective, I share @rkflx's concern with the amount of duplicate code. ThumbnailView::viewportEvent() is almost entirely duplicate code from DocumentView::event(), for example. Can we refactor this into re-usable functions in touch or touch_helper or something?

I will have a look at this, perhaps I can move same code around.

Also, can you help me understand why we need to implement our own finger gesture recognizers in all the files you've added under lib/touch? Can we not use Qt's built-in touch handling functionality for some reason?

QT gestures do not have the functionality we need:

  • Qt::SwipeGesture is two or more fingers, but we need one or two fingers
  • Qt::PanGesture is two fingers, but we need one or two fingers
  • Qt did not have double tap gesture (for toggle full screen)
  • Qt did not have two finger tap gesture (for context menu)
  • Qt did not have tap hold and moving gesture (for drag and drop action)

Thanks, that makes sense. However, it might also make sense to add generic support for all of this into Qt itself, or at least into a KDE Framework (KWidgetsAddons maybe?). There are lots of other QWidgets-based KDE apps that could benefit from this stuff too! Dolphin and Okular immediately come to mind.

Regardless, we need to remove the extreme code duplication here somehow, even if it's just using re-usable functions in Gwenview itself. Of course once that's done, it might not be too much work to get that code into a KDE Framework... :)

steffenh added inline comments.Feb 27 2019, 1:17 PM
lib/documentview/documentview.cpp
1001

can we keep this for the moment, because I need it on my working system (OpenSUSE Leap, Qt 5.9.4, KDE 5.45) ?

ngraham added inline comments.Feb 27 2019, 2:18 PM
lib/documentview/documentview.cpp
1001

OK.

steffenh updated this revision to Diff 53070.Mar 3 2019, 5:22 PM
  • move some of the duplicate code in documentview and thumbnailview to functions in touch and touch_helper
  • fix some typos
  • fix copyright
volkov added a subscriber: volkov.EditedMar 3 2019, 8:56 PM

+1 to adding gesture recognizers to KWidgetsAddons

Thanks @steffenh!

+1 to adding gesture recognizers to KWidgetsAddons

I agree. @steffenh, any chance you'd be interested in that? Then, we could use these very nice gestures in Okular and Dolphin too. Since you've already done the hard work of de-duplicating the code and putting it into helper files, hopefully that shouldn't be too much effort, right?

https://cgit.kde.org/kwidgetsaddons.git

Hi @ngraham

+1 to adding gesture recognizers to KWidgetsAddons

I agree. @steffenh, any chance you'd be interested in that? Then, we could use these very nice gestures in Okular and Dolphin too. Since you've already done the hard work of de-duplicating the code and putting it into helper files, hopefully that shouldn't be too much effort, right?

https://cgit.kde.org/kwidgetsaddons.git

I was thinking over this in the last days, but I'm not sure my coding is up to the task. Perhaps after I have finished this patch I can try it.
But with this idea in the background, I'm beginning to complete remove the event and gestureEvent function in dokumentview.cpp and thumbnailview.cpp, and use the Signal / Slot mechanics from Qt .

Hi @ngraham

+1 to adding gesture recognizers to KWidgetsAddons

I agree. @steffenh, any chance you'd be interested in that? Then, we could use these very nice gestures in Okular and Dolphin too. Since you've already done the hard work of de-duplicating the code and putting it into helper files, hopefully that shouldn't be too much effort, right?

https://cgit.kde.org/kwidgetsaddons.git

I was thinking over this in the last days, but I'm not sure my coding is up to the task. Perhaps after I have finished this patch I can try it.
But with this idea in the background, I'm beginning to complete remove the event and gestureEvent function in dokumentview.cpp and thumbnailview.cpp, and use the Signal / Slot mechanics from Qt .

OK, so are you still working on some changes for this patch then? I ask because if so, I'll wait until you're done to land it. :)

steffenh updated this revision to Diff 53575.Mar 10 2019, 8:46 AM

Move the gestureEvent(QGestureEvent* event) and event(QEvent* event) functions from documentview.cpp and thumbnailview.cpp to the Touch class.
I think this is now ready to land.

ngraham accepted this revision.Mar 16 2019, 10:51 AM

Thanks so much for your patience on this. I think it's ready to land now!

This revision was automatically updated to reflect the committed changes.

Thanks for the patch. How is the gesture code going to get into KWidgetAddons now?

I imagine by adding what's here into the framework first, and then later raising the Gwenview frameworks dependency high enough and using them from there framework.

@steffenh, are you interested? It sure would be nice to get these into Okular and Dolphin too. :)

I started to transfer the gestures to the framework, but I cannot promise anything.

No worries, and thanks for the attempt! Once they're in, that'll really help other QWidgets-based KDE apps.

Ideally, QWidget::mouseDoubleClickEvent() could be used instead of double tap gesture...
https://codereview.qt-project.org/#/c/259787/