Add support for touch scrolling in Dolphin
Needs ReviewPublic

Authored by abalaji on Apr 12 2018, 8:21 PM.

Details

Reviewers
ngraham
elvisangelaccio
Group Reviewers
Dolphin
Summary

This patch adds support for touch scrolling in Dolphin using QScroller, with touch events sharing some functions with mouse events to behave like clicks. Rubberbands do not trigger during touch scrolling. To prevent accidental item activation during touch scrolling, I check if there was a touch update event with a touch move emitted in between a touch begin and a touch end event, in which case I prevent item activation from happening. Also added the QScroller gesture to the information panel (which is kinda wip right now due to issues).

FEATURE: 385066

Diff Detail

Repository
R318 Dolphin
Branch
touchscroll
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6013
Build 6031: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham requested changes to this revision.Apr 12 2018, 10:27 PM

Wow, this is really great with the default single-click setting (it's mostly useless with double-click, but that's expected). Overall, quite fantastic, and I'm really looking forward to using Dolphin with this feature!

I found some papercuts:

  • The Information panel doesn't scroll via touch
  • Scrollviews in the Configure window don't scroll via touch
  • I can't seem to get touch-and-drag working. I touch, hold my finger down, and then move it after 1 second; nothing happens. Regardless, 1 sec is probably too long. I would recommend 500ms or even shorter.
  • I can trivially break single-touch-to-open such that subsequent touches only select:
    • Change the view mode (e.g. Icons to Details, and back again)
    • Try touch-and-drag
    • Try a two-finger pinch-zoom gesture
This revision now requires changes to proceed.Apr 12 2018, 10:27 PM

Hi @ngraham thanks for reviewing this!

  • I'll implement touch in the information and settings panels next.
  • 1 second is probably too long, but regardless it might be failing because a mouseMove event is accidentally getting triggered while you're holding (probably finger fatigue during that one long second), which instantly cancels the QTimer that enables dragging. One way to fix that would be to store the coordinates of the mouse press event, and on a mouse move event, rather than cancelling the timer, have a threshold of distance your finger can wiggle around.
  • The last issue is probably because I'm currently assuming that the events fire like touch start -> (maybe) touch move -> touch release, which would obviously break with multiple fingers. I can just ignore every additional finger touch, easy fix.

So I'm facing issues with this thing which after a bit of digging turns out to be this feature where kwin lets you "Drag windows from all empty areas" (default System Settings -> Application style -> Widget Style -> Applications -> Configure... -> Windows' drag mode), which often gets triggered (incorrectly) by touching and dragging on an empty area like while touch scrolling on the information panel. If you click and drag with a mouse, the dragging works as expected, but when you touch and drag, the window gets put into dragging mode (turns translucent due to desktop effects), but stays where it is and freezes. I have to lift my finger and touch it back down at a spot to make the window move there. This probably has nothing to do with dolphin itself but it's really weird when this effect kicks in, but regardless of whether it happens correctly or not, I was wondering how kwin figures out which widget is an "empty area" and if there's a way for a widget to tell the window manager it wants to disallow that?

abalaji updated this revision to Diff 32092.Apr 14 2018, 2:26 AM
abalaji edited the summary of this revision. (Show Details)

Due to the limitations of detecting multiple touches with "fake" mouse events triggered by automatically by Qt on touch, I've had to implement real touch event handlers and which means goodbye drag and drop because after some digging it seems that touch events and native drag n drop don't work well with each other. I've also added the touch gesture to the information panel which is partly broken right because of the "Drag windows by empty space" feature discussed before

anthonyfieroni added inline comments.
src/kitemviews/kitemlistcontroller.cpp
1392–1407

emitItemActivated increases complexity, so just remove it

if (m_view->isAboveExpansionToggle(index, pos)) {
    const bool expanded = m_model->isExpanded(index);
    m_model->setExpanded(index, !expanded);
    emit itemExpansionToggleClicked(index);
} else if (shiftOrControlPressed || m_singleClickActivationEnforced
|| !(m_view->style()->styleHint(QStyle::SH_ItemView_ActivateItemOnSingleClick))) {
    // The mouse click should only update the selection, not trigger the item
} else {
    emit itemActivated(index);
}
abalaji updated this revision to Diff 32099.Apr 14 2018, 7:40 AM

Simplify some if-else statements

Uh-oh, it looks like most of the diff got wiped out by that update...

abalaji updated this revision to Diff 32133.Apr 14 2018, 5:58 PM

Squash diffs together

Okay, so that seems to have fixed that. Is there a cleaner way to append a diff rather than squashing commits together? Can I just push to a branch or something?

abalaji marked an inline comment as done.Apr 14 2018, 6:01 PM

Thanks! I'll use that from now on. I've just been using git format-patch so far.

abalaji updated this revision to Diff 32137.Apr 14 2018, 6:23 PM

Now using arc

Getting better! The click/tap issue is gone. But now touch-and-drag in an empty area always starts a rubber band selection, and touch-and-drag on an item immediately starts to drag it--though in both cases, the view is scrolled too, if it's scrollable.

Touch-and-hold-for-a-moment-and-then-drag works correctly to drag an item or start a rubber band selection in an empty area.

Hmm, I had to get rid of the touch-hold-drag thing because earlier I was just adapting the existing mouse event handlers with touch, but because multitouch caused issues with that, I've now had to setAcceptTouchEvents(true) on the KItemListView, which means Qt will no longer emit fake mouse events on touch, and that also means no drag and drop because it seems that only works with mouse events. I also disabled rubberbands on touch so I'm seeing any on my end. What kind of touchscreen do you have? Can you verify if touchBeginEvent, touchUpdateEvent, and touchEndEvent are all firing for you?

abalaji updated this revision to Diff 32154.Apr 14 2018, 9:54 PM
  • Touch double click
abalaji updated this revision to Diff 32433.Apr 17 2018, 11:51 PM
  • Minor fixes and cleanup

Sorry for the long hiatus!

Touch double-click is working well now, very nice! But continue to have touch-and-drag issues:

  • When the Breeze theme setting for Windows' drag mode is set to Drag windows from titlebar only, touch-and drag in an empty area starts a rubber band selection, and touch-and-drag on top of a file or folder drags it (this should only happen on touch-and-hold-and-drag).
  • When it's set to "nav Drag windows from all empty areas}, I can't actually drag files and folders at all; they never get picked up when I touch-and-hold-and-drag.
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptMay 15 2018, 11:09 PM

What's the status here, @abalaji? I and a lot of people would really love to see this in Dolphin! :-)

abalaji added a comment.EditedJun 12 2018, 6:39 PM

Hey @ngraham sorry I've been missing this whole time. I'm back in school and flooded with assignments atm :-(. If you're super interested in this, then I can work on this over the weekend. So at the moment, here's what's working on my config at least:

  • Touch scroll in the KItemList (which covers both the file/folder view as well as the "Places" panel on the left)
  • Opening items on single touch and double touch (respects the single/double click activation policy)

What's not working as well

  • Touch scroll in the information panel on the right with the Breeze widget style with "Drag windows from all empty spaces" enabled. What happens is a drag gets initiated, but because of some reason, the move stops right there and nothing happens when I move my finger around. Same goes for the settings window.

When I pull my finger away, the window stays in the "move" mode. What I mean by this is the cursor stays in the + shape and the window is translucent if that desktop effect is enabled, even if my finger is now off the screen. When I touch the screen again, the window jumps into that spot as if I intended to move the window to that spot. This bug already exists and occurs on the upstream dolphin.

What I had to remove

  • The touch-hold-n-drag to preserve the dragging capabilities from mouse usage.

What happened is, initially I was relying on "fake" mouse events generated by Qt if the widget does not choose to listen to touch events. The problem with this is that we are limited to one touch point, and if the user tries to touch multiple fingers at once, one cannot accurately determine when each touch is released, since a mouse release event is never fired consistently in that scenario, causing a lot of glitches when you tried multitouch gestures for instance.

Now, I've had to switch to native touch gestures, and make the mouse and touch events wrappers for the actual functionality, which is now refactored into separate functions. Qt does not have a touch equivalent for mouse drag gestures, hence dragging cannot work properly, so I had to remove it.

abalaji retitled this revision from Support for touch scrolling in Dolphin to Add support for touch scrolling in Dolphin.Jun 12 2018, 7:00 PM

Thanks for the updates! Hopefully someone else from Dolphin can give you some pointers.

Hi,
I have one problem with touch on the scrollbar, it does not work proberly.
I have change follow a line in the kitemlistcontainer.cpp :

QScroller::grabGesture(this), QScroller::TouchGesture);

to:

QScroller::grabGesture(this->viewport(), QScroller::TouchGesture);

an in the informationpanelcontent.cpp:

QScroller::grabGesture(m_metaDataArea, QScroller::TouchGesture);

to

QScroller::grabGesture(m_metaDataArea->viewport(), QScroller::TouchGesture);

and now it works perfect.

abalaji updated this revision to Diff 37018.EditedJul 1 2018, 4:31 PM
  • Fix issues with scroll bar on touch

Thanks @steffenh, I've fixed the issues with the scroll bars.

It would be really nice to have eyes from some more Dolphin folks on this. :)

@broulik, any chance you could help out with the code review for this? My sense is that a lot of people would really love touch support in Dolphin.

acrouthamel added a subscriber: acrouthamel.

How cool is this, how have I not seen this yet?

Hey everyone, I can make some time for myself during this weekend to work on this, feel free to leave comments

Awesome, thank you!

abalaji updated this revision to Diff 42162.Sep 23 2018, 2:58 AM
  • Rebase

FWIW, @davidedmundson just submitted a patch that fixes one of the issues you were running into: D15942: Don't drag windows in empty areas from touch/pen events

abalaji updated this revision to Diff 42940.Oct 5 2018, 6:49 PM
  • Rebase

@abalaji, is this waiting for more UI or code review, or are you still working on some outstanding issues?

abalaji updated this revision to Diff 43096.Oct 8 2018, 2:29 AM
  • Add scrolling to item lists in Configure menu

Hey @ngraham, as you can see I just added scrolling to two list views in the configure menu. Apart from that, there's just one minor issue in the information panel that I'm working on right now: when you touch a clickable thing like a link or a ratings star, then touch scroll, then release the touch point at the same spot on top of the clickable thing, it gets activated. I'm in the process of fixing this. But otherwise, a code review would be appreciated.

Awesome. I'll be able to test again soon. I don't feel qualified to offer code review for this kind of patch though, so hopefully @broulik, @elvisangelaccio, or @anthonyfieroni will be able to do so.

abalaji updated this revision to Diff 43182.Oct 9 2018, 5:22 AM
  • Tidy up diff
abalaji updated this revision to Diff 43183.Oct 9 2018, 5:24 AM
  • Remove unnecessary newline

I don't have a touch screen for actual testing, but I'll have a look asap anyway.

I'll be happy to do all the UI testing if you can do the code review!

abalaji updated this revision to Diff 46337.Nov 27 2018, 7:24 PM
  • Rebase
elvisangelaccio requested changes to this revision.Dec 2 2018, 11:20 AM

Finally had the time to look at this diff.

The patch is surprisingly simpler than I was expecting, but still I'd recommend to split it into two parts:

  1. The code refactoring in KItemListController (i.e. onPress(), onMove(), etc.)
  2. The new support for QScroller and the touch events.
src/kitemviews/kitemlistcontroller.cpp
73

Please add this as receiver, otherwise this connection can lead to crashes.

This revision now requires changes to proceed.Dec 2 2018, 11:20 AM
abalaji added inline comments.Dec 3 2018, 6:32 PM
src/kitemviews/kitemlistcontroller.cpp
73

I'm sorry can you elaborate on this a little bit?

elvisangelaccio added inline comments.Dec 3 2018, 8:49 PM
src/kitemviews/kitemlistcontroller.cpp
73

Basically Qt could call the lambda slot after the object has been already destroyed.

More details here: https://medium.com/genymobile/how-c-lambda-expressions-can-improve-your-qt-code-8cd524f4ed9f

abalaji added inline comments.Dec 3 2018, 9:36 PM
src/kitemviews/kitemlistcontroller.cpp
73

Ahh, of course! This makes so much sense. I've experienced weird crashes with connect and lambda's before and never knew this was a thing. Thanks for pointing it out!

abalaji updated this revision to Diff 47557.Dec 14 2018, 11:16 AM
  • Fix leak and warning

@abalaji any update here? This is a really nice feature and I'd really like to see it land in 19.04. :)

Hey @ngraham Im so sorry, I didn't see your message come through. I've been pretty busy the past week because of midterms at my university. I'll split this into two asap

Thank you so much! Don't let me rush you and make your studies suffer, though. :)

FWIW, the 19.04.0 branches were just created today so this is going to have to go into 19.08.0.

Oh rip, looks like I missed the bus. I have it done halfway locally,
perhaps I can get it cleaned up by tomorrow. Any chance for 19.04?

There's always a chance. However being realistic, a big patch like this is probably better to commit right after branching anyway, because then we have four months of pre-release testing and we can ensure it's polished all nice and shiny bright for the next release. :)

Well true, probably for the best to go for 19.08. Sorry, I should have
tried to manage my time better

It's all good my friend! I have faith that it'll get in when it's ready.

Who knows, maybe eventually you'll get to use the gesture code that just landed in Gwenview once @steffenh upstreams it into a framework. :)

Friendly ping! :)