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 3118
Build 3136: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
rkflx added a comment.EditedJul 7 2018, 6:59 AM

libinput debug-gui shows that my touchpad and touchscreen can both can see two, three, and four finger swipes, two-finger pinches, two-finger rotation, and two finger scrolling.

(Can you really see two-finger swipes, in contrast to two-finger scrolling?)

Results of testing:

Thanks for testing! I think such detailed feedback is really helpful. +1 for most of it, some additional commentary below:

View Mode

  • One-finger swipe left and right on a touchscreen: works, but there's no horizontal transition animation, so it looks really weird. Would be nice to have an animated effect of the image following your finger and moving to the left or right.

Agreed, but I would suggest to work on this in a separate patch (or at least file a bug).

@ngraham Is this working in Okular for you? If not, I would suggest to focus on touchscreen gestures here, and change the line in the summary to a CCBUG only.

  • Two-finger pinch in and out on a touchscreen: More or less works, it's much too sensitive; zoom should be slower and more gradual. Also, performance is pretty bad, and the zoom is quite choppy.

Perhaps this first needs work (in another patch) to make it work smoothly for zooming by scrolling and pressing Ctrl?

It's getting late here and my brain is turning off, but I can test mode use cases as people come up with them,

More ideas for what to touch on:

  • Start page
  • various tabs of the sidebar
  • thumbnail bar in fullscreen View mode
  • swiping in View mode when one of the items is a video

Hi @ngraham,

many thanks for your time to test this.

I will work on the points you have found, but I will concentrate on the touchscreen at the moment. If I get the touchscreen working, perhaps I can try the touchpad.

One-finger touch-and-drag with a touchscreen: draws a selection marquee and also scrolls. If you touch-and-dragged over a picture, it scrolls and also drags the image, resulting in a drag-and-drop menu when you remove your finger. Produces in big ugly white areas if you scroll beyond the content area.

Yes, this is a problem, I am working at the moment. The reason for this is, Qt synthesized a mouse click from the touch, I need to catch this and let only the right mouse clicks through.

  • Pinch in and out on a touchscreen: interpreted as a touch-and-drag instead of a pinch-zoom as it should

The pinch on this, is not implemented at this moment, I was not aware of this feature for Gwenview. I will implement it later.

  • One-finger touch-and-drag on the thumbnail strip with a touchscreen: scrolls, but the thumbnail that was under your finger gets dragged and again you see the drag-and-drop menu when you release your finger. Also produces big ugly white areas if you scroll beyond the content area.

Is the same as above. A synthesized mouse click from Qt start an unwanted drag. The big white area is a "feature" from the Qt QScroller.

  • Three or four-finger swipe over the image on a touchpad: doesn't work; not implemented?

I am using the Qt TouchEvent and GestureEvent for this, I have the feeling the touch on the touchpad will not land in the Event. Bug or feature from Qt??

  • One-finger swipe left and right on a touchscreen: works, but there's no horizontal transition animation, so it looks really weird. Would be nice to have an animated effect of the image following your finger and moving to the left or right.

For the image change I use the "build in" function from Gwenview, also the same effect if you press the next button in the toolbar.

  • Two-finger pinch in and out on a touchscreen: More or less works, it's much too sensitive; zoom should be slower and more gradual. Also, performance is pretty bad, and the zoom is quite choppy.

I can reduce the sensitive for this. At the moment I have a problem to set the center of the zoom to the center of the pinch gesture, I have not found the right mapping, so this result in a jumpy zoom effect.

  • One-finger touch-and-drag to pan a zoomed-in image: doesn't work, and also horizontal drags are interpreted as swipe gestures and inappropriately move to another image

Yes, sometimes this works, try to touch the toolbar button "next image", or touch on the image counter in the bottom of the window, after this is working for me. I don't know the reason for this, but I am looking for a solution. Perhaps I need to write a own event handler for this.

  • Two-finger rotate with a touchscreen: works great, but is inappropriately triggered with you put two fingers down and move both of them in an arc (it should only trigger when one finger is held still and the other one moves in an arc)

I will change the trigger for the rotation, so it only triggers if the center is stationary.

steffenh updated this revision to Diff 37467.Jul 9 2018, 4:32 PM

Fix some issure found in the last testing from @ngraham

  • no unwanted drag action in browse mode
  • to start draganddrop you need a TabAndHold Gesture before you move the touchpoint
  • set QScroller OvershootPolicy to always off, no big ugly white areas anymore
  • one, two or more finger swipe in View mode to change image
  • reduce sensitify for zoom
  • one finger paning for image in view mode
  • draganddrop in view mode, after TabAndHold gesture
  • make pinch gesture for rotation more stationary to suppress unwanted rotations

add Test Plan to summary

TODO

  • QScroller for all scrollbar sidebars
  • new Drag Action (TabAndHold) for drag actions in all sidebars
  • Zooming in Browse mode
  • make zooming behavior in View mode nicer
  • need we multiple selection with the touchscreen in Browse mode?
  • 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?

  • if the system mouse behavior is set to double-click to open a file and directory, tap on touchscreen don't open Image

(double tap don't work)

  • two finger panning in View mode

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

steffenh edited the summary of this revision. (Show Details)Jul 9 2018, 4:33 PM
steffenh edited the test plan for this revision. (Show Details)
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.
1157

Space before { missing.

1164

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 TranscriptMon, Aug 27, 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.Sat, Sep 22, 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