D12162: Support for touch scrolling in Dolphin

[Ambareesh Balaji]

abalaji created this revision.
abalaji added a reviewer: Dolphin.
abalaji added a project: Dolphin.
abalaji requested review of this revision.

REVISION SUMMARY
This patch adds support for touch scrolling in Dolphin using QScroller. I'm using Qt::MouseEventNotSynthesized to figure out if a mouse event was synthesized (which means it was a touch event and not an actual mouse event). Rubberbands do not trigger during touch scrolling. To prevent accidental item activation during touch scrolling, I check if there was a mouseMoveEvent emitted in between a mousePressEvent and a mouseReleaseEvent, in which case I prevent item activation from happening. To deal with grabbing and dragging using the touchscreen, I've implemented a rudimentary "touch and hold" mechanism, with a QTimer that starts on a mousePress and waits for 1000 milliseconds to see if the mouse moved, and if that wasn't the case, it disables touch scrolling temporarily so that dragging takes over.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontainer.h
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h

To: abalaji, #dolphin
Cc: #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32010.
abalaji added a comment.


Remove unnecessary edits

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32007&id=32010

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontainer.h
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h

To: abalaji, #dolphin
Cc: #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin
Cc: #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32092.
abalaji edited the summary of this revision.
abalaji added a comment.


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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32010&id=32092

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

kitemlistcontroller.cpp:1392-1407
+ bool emitItemActivated = true;
+ if (m_view->isAboveExpansionToggle(index, pos)) {
+ const bool expanded = m_model->isExpanded(index);
+ m_model->setExpanded(index, !expanded);
+
+ emit itemExpansionToggleClicked(index);
+ emitItemActivated = false;

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);
}

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32099.
abalaji added a comment.


Simplify some if-else statements

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32092&id=32099

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32133.
abalaji added a comment.


Squash diffs together

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32099&id=32133

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontainer.h
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


To change the diff, just issue `arc diff`. You are using arc, right? ;) ) If not, see:

- https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches
- https://community.kde.org/Infrastructure/Phabricator#Step_2:_Update_your_diff_in_response_to_review_comments

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji marked an inline comment as done.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32137.
abalaji added a comment.


Now using arc

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32133&id=32137

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontainer.h
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32154.
abalaji added a comment.


- Touch double click

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32137&id=32154

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontainer.h
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 32433.
abalaji added a comment.


- Minor fixes and cleanup

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32154&id=32433

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.
Restricted Application added a subscriber: kfm-devel.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


Hey @ngraham 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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji retitled this revision from "Support for touch scrolling in Dolphin" to "Add support for touch scrolling in Dolphin".

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


Thanks for the updates! Hopefully someone else from #dolphin <https://phabricator.kde.org/tag/dolphin/> can give you some pointers.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Steffen Hartleib]

steffenh added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 37015.
abalaji added a comment.


Rebase

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=32433&id=37015

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 37018.
abalaji added a comment.


-Fix issues with scroll bar on touch

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=37015&id=37018

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


It would be really nice to have eyes from some more #dolphin <https://phabricator.kde.org/tag/dolphin/> folks on this. :)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a subscriber: broulik.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


Awesome, thank you!

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 42162.
abalaji added a comment.


- Rebase

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=37018&id=42162

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a subscriber: davidedmundson.
ngraham added a comment.


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 <https://phabricator.kde.org/D15942>

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 42940.
abalaji added a comment.


- Rebase

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=42162&id=42940

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp

To: abalaji, #dolphin, ngraham
Cc: davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 43096.
abalaji added a comment.


- Add scrolling to item lists in Configure menu

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=42940&id=43096

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp
src/settings/general/previewssettingspage.cpp
src/settings/services/servicessettingspage.cpp

To: abalaji, #dolphin, ngraham
Cc: davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a subscriber: elvisangelaccio.
ngraham added a comment.


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.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 43182.
abalaji added a comment.


- Tidy up diff

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=43096&id=43182

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp
src/settings/general/previewssettingspage.cpp
src/settings/services/servicessettingspage.cpp

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 43183.
abalaji added a comment.


- Remove unnecessary newline

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=43182&id=43183

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp
src/settings/general/previewssettingspage.cpp
src/settings/services/servicessettingspage.cpp

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Ambareesh Balaji]

abalaji updated this revision to Diff 46337.
abalaji added a comment.


- Rebase

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D12162?vs=43183&id=46337

BRANCH
touchscroll

REVISION DETAIL
https://phabricator.kde.org/D12162

AFFECTED FILES
src/kitemviews/kitemlistcontainer.cpp
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/panels/information/informationpanelcontent.cpp
src/settings/general/previewssettingspage.cpp
src/settings/services/servicessettingspage.cpp

To: abalaji, #dolphin, ngraham
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


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.

INLINE COMMENTS

kitemlistcontroller.cpp:73
+ m_touchDoubleClickTimer.setSingleShot(true);
+ connect(&m_touchDoubleClickTimer, &QTimer::timeout, [this]() {
+ m_touchDoubleClickInProgress = false;

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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham, elvisangelaccio
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in kitemlistcontroller.cpp:73
Please add `this` as receiver, otherwise this connection can lead to crashes.

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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham, elvisangelaccio
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

abalaji wrote in kitemlistcontroller.cpp:73
I'm sorry can you elaborate on this a little bit?

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

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham, elvisangelaccio
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in 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

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!

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D12162

To: abalaji, #dolphin, ngraham, elvisangelaccio
Cc: elvisangelaccio, davidedmundson, acrouthamel, broulik, steffenh, kfm-devel, anthonyfieroni, ngraham, #dolphin, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov