Touch support for Gwenview
Needs ReviewPublic

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

Details

Reviewers
ngraham
muhlenpfordt
Group Reviewers
Gwenview
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
arcpatch-D13901
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3322
Build 3340: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
steffenh edited the test plan for this revision. (Show Details)Jul 9 2018, 4:33 PM
rkflx added a comment.Jul 9 2018, 7:47 PM

Thanks for the updates, glad to see your fast progress.

  • need we multiple selection with the touchscreen in Browse mode?

For now I'd say we should defer that particular feature to a future patch. This probably needs design work/ideas/research on how that interaction should work, not only in Gwenview, but in all of KDE's apps.

For example on some platforms there is a special selection mode which can be entered via a button or through tap-and-hold, after which a single tap allows to toggle the selection for the rest of the images. In other words, this is more than a simple change because it requires changes to the UI.

  • many variable to modify the sensitivity, are a result of testing on my device, I don't know if on other devices the sensitivity equal or not. Have we system variable for the touchscreen behavior?

Not sure how much difference there is between your and Nate's touchscreen behaviour, but generally speaking Gwenview would not be the right place for such configuration switches (if they are needed at all).

do you want I work through my TODO list before I post the next diff, or is step per step ok.

It's probably too much to ask for testing everything after every update, but if you want to get feedback on a particular change or addition, I'd say just update the Diff. In any case detailed testing of the final result will be needed.

Excellent progress. You managed to resolve nearly all the problems I encountered with the first version of this patch! Behavior-wise, here's all I found with this latest version:

  • Two-finger pinch zooming in View Mode is better, but still too sensitive IMHO. We don't yet have a global KCM to control touchscreen behavior, but it's inevitable that we'll get one. In the meantime, let's shoot for a sane default.
  • Two-finger pinch zooming in View Mode still doesn't center the zoom on the midpoint between your fingers, but I think this is a known issue you already mentioned?
  • It's still possible to one-finger swipe to the next and previous image when zoomed in, which results in a lot of unintended swipes when trying to pan around a zoomed-in image with one finger. I would advocate that you should only be able to (for example) one-finger swipe to the left if the zoomed-in viewport is already touching the left-hand side of the image.

Hi @rkflx,

For now I'd say we should defer that particular feature to a future patch. This probably needs design work/ideas/research on how that interaction should work, not only in Gwenview, but in all of KDE's apps.

Ok, I will ignore this at the moment.

It's probably too much to ask for testing everything after every update, but if you want to get feedback on a particular change or addition, I'd say just update the Diff. In any case detailed testing of the final result will be needed.

Ok, I will work trough my TODO list.

Hi @ngraham,
thank you for testing

  • Two-finger pinch zooming in View Mode is better, but still too sensitive IMHO. We don't yet have a global KCM to control touchscreen behavior, but it's inevitable that we'll get one. In the meantime, let's shoot for a sane default.

I will reduce the sensitive, no problem.

  • Two-finger pinch zooming in View Mode still doesn't center the zoom on the midpoint between your fingers, but I think this is a known issue you already mentioned?

Yes, this is a problem I am working at the moment. I think I have found a solution, but I will test it first on my device.

  • It's still possible to one-finger swipe to the next and previous image when zoomed in, which results in a lot of unintended swipes when trying to pan around a zoomed-in image with one finger. I would advocate that you should only be able to (for example) one-finger swipe to the left if the zoomed-in viewport is already touching the left-hand side of the image.

Ok, this is a good idea, I will implement this.

I hope for the weekend, I will have a new version of the patch.

steffenh updated this revision to Diff 37730.Jul 14 2018, 9:18 AM
steffenh edited the test plan for this revision. (Show Details)

Fix some issure, implented some things

new features:

  • pinch gesture for zooming in browse mode
  • double touch for toggle fullscreen in view mode
  • reimplented two fingers scrolling / panning in browse and view mode
  • double touch for the open image / directory in browse mode, if the system mouse behavior set to "double click to open

file.."

  • add one finger scrolling to various sidebars

fixed issure:

  • reduce sensivity for zooming (insert a delay for same millisecond, to suppress same unwanted zooming)
  • swipe gesture to change image, only works now if viewport of the image at the left or right border
steffenh edited the test plan for this revision. (Show Details)Jul 14 2018, 9:19 AM

The progress here is excellent! Here are my comments with the latest revision of the patch:

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.
  • While in View mode, when I touch a thumbnail in the thumbnail bar, mpw it sometimes takes two touches to activate. This seems to happen a lot when I change direction (e.g. when I touch a picture to the left of the current one four times, and then touch a picture to the right of the current one, very often that touch isn't registered properly)
  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

Hi, @ngraham,

thank you again for your time, to test this patch.

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.

I will increase the sensitivity for the zoom, maybe a value between this version and the previous version?

  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.

I think I don't understand this right, what you mean with "do a pinch zoom without moving the fingers".

  • While in View mode, when I touch a thumbnail in the thumbnail bar, mpw it sometimes takes two touches to activate. This seems to happen a lot when I change direction (e.g. when I touch a picture to the left of the current one four times, and then touch a picture to the right of the current one, very often that touch isn't registered properly)

Ok, I will give the tab gesture a little bit wiggel room, (I think the reason for this issure is, you have a little moving between touch begin and touch end).

  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

You mean we don't take the mouse behavior for touch behavior? Ok, I will change this.

Hi, @ngraham,

thank you again for your time, to test this patch.

You're very welcome! It's coming along nicely.

  • Pinch zooming in View mode is just a small amount too slow now. :)
  • Pinch zooming in View mode is rather laggy and could be smoother to provide a really optimal user experience.
  • Pinch zooming in Browse mode (nice feature!) is quite a bit too slow.

I will increase the sensitivity for the zoom, maybe a value between this version and the previous version?

Couldn't hurt to try it out, at least! :)

  • Pinch zooming in View mode seems to try to center the zoom on the midpoint between your fingers, but it's not as accurate as it needs to be. It works well if you pinch zoom while also moving both fingers--in this case it does a zoom-and-pan in the right general area. But if you just do a pinch zoom without moving the fingers too, the zoom seems centered somewhere else.

I think I don't understand this right, what you mean with "do a pinch zoom without moving the fingers".

I mean when you do a pinch zoom and a pan at the same time, basically, You zoom by pinching, while also moving the pinching fingers in the same direction to pan the view at the same time that the pinch zoom happens.

  • The Folders tab of the sidebar should activate items on a single-touch even if double-click is using used (clicks != touches)

You mean we don't take the mouse behavior for touch behavior? Ok, I will change this.

Right, the click setting only applies to clicks. With touch, double-click doesn't exist (and double-tap is a very challenging gesture); the whole UI should use the single-touch paradigm.

steffenh updated this revision to Diff 38176.Jul 21 2018, 11:51 AM
steffenh edited the test plan for this revision. (Show Details)

Fix some issue

  • adjust zooming speed
  • give tab gesture some wobble room
  • ignore the system behavior for double-click, activate items, always with a single touch
  • fix issue with drag and drop in browse mode, now drag the right image
  • fix issue in view mode with multiple images, now double touch to toggle fullscreen works
  • fix issue with toolbar in fullscreen view mode, now fade in the toolbar if you touch the up border area of the screen
  • adjust zoom center to pinch center (I have a feeling the zoom function doesn't like me)

Great work. Zoom speed in View mode now feels perfect, pinch zoom gestures now zoom into the midpoint between the fingers and zoom-and-pan works perfectly, the thumbnail bar works reliably, and we use the single-touch paradigm in touch mode. I feel like we're getting really close! Here are the few remaining things that still don't work perfectly for me:

  • Single-finger panning on a zoomed in image results in the image moving faster than the finger. The image should always pan at the exact same speed as the finger so that the part of the image you're touching feels "locked" to your fingertip.
  • Pinch zoom in View mode is still choppy.
  • A double-tap is still required to navigate to or expand/collapse the folders in the tree view on the Folders tab. We should use the single-click behavior here.
  • Double-tab to enter or exit full screen mode with an image doesn't work reliably for me. If I triple-tap, it will very quickly enter and then exit full screen mode. Feels like this needs to be made a bit more robust.

Hi @ngraham

thank for testing.

  • Single-finger panning on a zoomed in image results in the image moving faster than the finger. The image should always pan at the exact same speed as the finger so that the part of the image you're touching feels "locked" to your fingertip.

Strange effect, in KDE Neon 5.13 I can not reproduce this issue. In OpenSuse Leap 15.0 the issuer begins only after you touch an element of the Gwenview window, for example the image counter in the lower window bar or the "next button" in the toolbar.
After some test I found the reason for the fast scrolling, Qt synthesize a mouse event from the touch event, but usually only from unhandled events. It is no problem because I can catch this event in the mouse event handler, but it is strange.

Hi,

after some test, I found my touchpad is working for zoom and rotate action in the view mode. The zoom action (pinch on the touchpad) worked out of the box, for the rotate action I need to set an alternative shortcut to rotate left and right to "Ctrl+," and "Ctrl+.". I don't know is it my touchpad and driver Combi or work this for all touchpad's.

@steffenh Could you report how touchpad gestures are working for you in Qt's example app (see https://doc.qt.io/qt-5/qtwidgets-gestures-imagegestures-example.html)?

It sounds a bit strange that setting different keyboard shortcuts for the rotate actions will get it working. Is it possible that you have some kind of global gesture recognizer running in your desktop session, which detects that action and then sends the keyboard shortcut combination to Gwenview?

(Unfortunately I can only test again with the precision touchpad I mentioned above in several weeks.)


Also, please let me know when you are either satisfied with how the patch works, or it makes sense to work on further improvements in a later patch. I will then proceed to the code review part.

(Thanks for your patience so far, glad to see how you are able to implement Nate's suggestions…)

Hi, @rkflx

@steffenh Could you report how touchpad gestures are working for you in Qt's example app (see https://doc.qt.io/qt-5/qtwidgets-gestures-imagegestures-example.html)?

touchpad is not working in this example.

It sounds a bit strange that setting different keyboard shortcuts for the rotate actions will get it working. Is it possible that you have some kind of global gesture recognizer running in your desktop session, which detects that action and then sends the keyboard shortcut combination to Gwenview?

Yes, I know it is strange. I found this in OpenSuse Leap 15.0 and a fresh KDE Neon 5.13, no global gesture recognizer is running.I found this by accident, I was looking which event I get in the DocumentView::sceneEventFilter with a qDebug()<<event;, to my surprise I found this:

QKeyEvent(ShortcutOverride, Key_Control, ControlModifier)
QKeyEvent(KeyPress, Key_Control, ControlModifier)
QGraphicsSceneEvent(GraphicsSceneWheel, 0x7fff873cb830)
QGraphicsSceneEvent(GraphicsSceneWheel, 0x7fff873cb830)
QKeyEvent(KeyRelease, Key_Control)

QKeyEvent(ShortcutOverride, Key_Control, ControlModifier)
QKeyEvent(KeyPress, Key_Control, ControlModifier)
QKeyEvent(ShortcutOverride, Key_Period, ControlModifier, text=".")
QKeyEvent(KeyRelease, Key_Period, ControlModifier, text=".")
QKeyEvent(KeyRelease, Key_Control)

QKeyEvent(ShortcutOverride, Key_Control, ControlModifier)
QKeyEvent(KeyPress, Key_Control, ControlModifier)
QKeyEvent(ShortcutOverride, Key_Comma, ControlModifier, text=",")
QKeyEvent(KeyRelease, Key_Comma, ControlModifier, text=",")
QKeyEvent(KeyRelease, Key_Control)

for pinch, zoom and rotate on the touchpad. So that's why I came up with the idea with the shortcut.

rkflx added a comment.Jul 25 2018, 7:14 PM

It sounds a bit strange that setting different keyboard shortcuts for the rotate actions will get it working. Is it possible that you have some kind of global gesture recognizer running in your desktop session, which detects that action and then sends the keyboard shortcut combination to Gwenview?

Yes, I know it is strange. I found this in OpenSuse Leap 15.0 and a fresh KDE Neon 5.13, no global gesture recognizer is running.I found this by accident, I was looking which event I get in the DocumentView::sceneEventFilter with a qDebug()<<event;, to my surprise I found this:

Okay, so something is turning the touchpad gesture into key events. We'd have to find out what that is (possibly the touchpad driver, or due to ShortcutOverride more likely Qt?), and whether it works on more than on your machine.

As that does not seem to be related to the touchscreen support, I'd suggest to work on this in a separate Diff, though (or just open a new task for discussion on Gwenview's workboard if you have no code yet).

steffenh updated this revision to Diff 38654.Jul 28 2018, 12:31 PM

Fix some issue

  • panning on a zoomed image, the image move now with the same speed as the finger
  • triple tap only interpreted as one double tap
  • folders tab now used single tab to navigate, if the mouse behavior is set to double click to open file

known issue:

  • zoom is still choppy. At the moment I have no idea to change this.

We are so close! Other than the choppy zooming, I still have a problem with double-tap in View mode not entering full screen mode. But that's it! Everything else is working really really well.

Hi @ngraham

We are so close! Other than the choppy zooming, I still have a problem with double-tap in View mode not entering full screen mode. But that's it! Everything else is working really really well.

it is not working at all, or only sometimes?
I have double tab define as two successive one finger tabs in a short timeframe (the same time frame as mouse doubleclick time). Is the time frame too short?

Re-using the logic for double-click is likely to be inappropriate for the following reasons:

  • Double-clicks are usually very precise, with only a very small distance between the pixel clicked for the first click and the pixel clicked for the second click; double-taps are likely to have a far larger distance.
  • Double-clicks are usually fast; double-taps are likely to be slower.
steffenh updated this revision to Diff 38692.Jul 29 2018, 7:39 AM

Fix double tab in View mode

*give double tab in View mode some wiggel room and increase the time frame for double tab

ngraham accepted this revision as: ngraham.Jul 29 2018, 4:38 PM

Perfect. I've been continuing to use this daily and in terms of behavior, it gets a big thumbs up from me. You've fixed all the behavioral issues I found, and it's really smooth and polished. The only outstanding issue is the choppy pinch zoom performance, but I suppose we can work on that later.

I'll now hand the show off to @rkflx for code review!

rkflx added a comment.Jul 29 2018, 5:12 PM

Awesome. I'll look at it soon (but not sure how much time I can spend during the week, so please bear with me…).

ngraham requested changes to this revision.Aug 2 2018, 7:49 PM

@steffenh, I'm afraid I've found a regression in your patch as compared to git master from the point where you branched (2324a8681a444cd1b9b441e481adb15c09f92837):

  1. Go to Settings > Configure Gwenview > Image View > Zoom mode, and then click on "Keep same zoom and position"
  2. Navigate to a folder with more than one image in it
  3. Enter View mode for an image
  4. Zoom in to any zoom value greater than 100% but less than 400%, so that the zoomed-in image is smoothed
  5. Use the thumbnail strip, arrow keys, or a touch gesture to navigate to another image in the folder

The image that you navigate to is correctly viewed at the same zoom level, but it is not smoothed the way the previous image was; it is rendered in a sharp, pixellated manner. With master, the smoothing status is correctly preserved when navigating to a new image in View mode.

This is the only regression I've found so far in 2 weeks of real-world use (yay), but we do need to fix it before the patch can be merged.

This revision now requires changes to proceed.Aug 2 2018, 7:49 PM
rkflx added a comment.Aug 2 2018, 9:32 PM

@steffenh, I'm afraid I've found a regression in your patch as compared to git master from the point where you branched (2324a8681a444cd1b9b441e481adb15c09f92837)

@ngraham Just to be sure: With "git master from the point where you branched" you mean that you issued git checkout 2324a8681a44, compiled and ran Gwenview, and then found a regression compared to the Diff?

you mean that you issued git checkout 2324a8681a44, compiled and ran Gwenview,

Correct.

and then found a regression compared to the Diff?

2324a8681a44 works fine. HEAD works fine too. With this patch, it does not work fine.

Hi,

after
git checkout 2324a8681a44
and
arc patch D13901
I have found the same issue.
But if I patch manually (copy and paste from diff to documentview.h and documentview.ccp) it is working fine.

rkflx added a comment.Aug 3 2018, 10:43 AM

after
git checkout 2324a8681a44
and
arc patch D13901
I have found the same issue.

@steffenh Did you issue both commands and then test, or did you test after each command? To properly test whether your patch introduces a regression, you would have to compare Gwenview's behaviour with 2324a8681a44 against that with D13901.

(TBH, I cannot confirm Nate's claim, I'm pretty sure this is not a regression. I think it is an old bug which will be solved by rebasing to the latest master, something you should do eventually anyway…)

steffenh updated this revision to Diff 39006.Aug 3 2018, 11:11 AM

Fix the issues with the new master

Hi @rkflx

I have test after each command

after
git checkout 2324a8681a44
and
arc patch D13901
I have found the same issue.

@steffenh Did you issue both commands and then test, or did you test after each command? To properly test whether your patch introduces a regression, you would have to compare Gwenview's behaviour with 2324a8681a44 against that with D13901.

(TBH, I cannot confirm Nate's claim, I'm pretty sure this is not a regression. I think it is an old bug which will be solved by rebasing to the latest master, something you should do eventually anyway…)

To properly test whether your patch introduces a regression, you would have to compare Gwenview's behaviour with 2324a8681a44 against that with D13901.

Yep, that's exactly what I did. 2324a8681a44 does not have the bug. 2324a8681a44 with D13901 on top of it does have the bug (at least for me).

Regardless, we should also get this rebased on master, @steffenh.

rkflx added a comment.Aug 3 2018, 10:34 PM

To properly test whether your patch introduces a regression, you would have to compare Gwenview's behaviour with 2324a8681a44 against that with D13901.

Yep, that's exactly what I did. 2324a8681a44 does not have the bug.

But how can this be possible? You even reviewed D14269, which only got in after 2324a8681a44, so 2324a8681a44 must still have the bug.

Regardless, we should also get this rebased on master, @steffenh.

It was already rebased by Steffen after I suggested it.

Also, now that the bug is not present anymore in this patch, how would you explain that (in your opinion) the patch included a regression, and then that regression was fixed merely by rebasing, without changing anything in the patch at all? The problem was fixed in master, it had nothing to do with this patch. If I'm wrong (which is not impossible, but I doubt it in this case), what part of the code do you think relates to the problem? This is all about touch here, nothing related to smoothing or the "arrow keys" you are mentioning in your comment above.

I really don't get it, the only explanation I have is that both of you bodged your testing (which we should try to improve, because for me it's a huge waste of time to chase non-issues.)

I had completely forgotten about D14269. I guess we did both mess up our testing somehow. Are there some quirks we didn't account for when you git checkout a specific revision and build it?

rkflx added a comment.Aug 6 2018, 8:07 PM

Are there some quirks we didn't account for when you git checkout a specific revision and build it?

None that I know of, besides looking out for errors (e.g. due to changes you made to you working copy) and making sure you run the binaries you built and not the system-provided ones. But that should not be an issue, because you said with master the bug was gone (you performed exactly the same steps for master and for 2324a8681a44, did you, without any creative shortcuts?).

Nope, no creative shortcuts.

Now that 18.08 is released, it's finally time for the code review part of your patch. As mentioned before, I don't have any hardware to test this. Mouse-based usage still seems to be fine, with one exception though (see below).

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

It's in a pretty good state already! Please see below for some inline comments.

app/folderviewcontextmanageritem.cpp
98

Q_DECL_OVERRIDEoverride

(28754fa2d95e changed this globally)

98

QEvent* event is the preferred style for Gwenview.

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

100

Add comment, e.g.

// Even with mouse behaviour set to double-click,
// a single tap should activate a folder

(otherwise it might not be obvious why this was done later on)

105–110

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?

107–109

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

108–109

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

(again, see below for more occurrences of this)

110

Add spaces around <

234–237

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

235

Only actual member variables should get an m prefix.

236

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

237–238

Add space after commas.

237–238

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

240

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

241

Remove extra space before ).

app/startmainpage.cpp
166

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

167–172

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
120–128

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?

122

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

456

Remove extra space before (.

722

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.
1163

Space before { missing.

1170

Is this still needed given the check above?

lib/documentview/documentview.h
217

override

218

Remove extra space after (.

218

Better put this into the private section?

lib/thumbnailview/thumbnailview.cpp
707

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?

836

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

838

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
836

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.