Improved Touch support for Kickoff
AcceptedPublic

Authored by steffenh on Jun 15 2019, 10:23 AM.

Details

Reviewers
hein
ngraham
Group Reviewers
Plasma
Summary
  • enabled touch scrolling in Kickoff
  • open the correct category in the application tab
  • open the context menu with touch (tapandhold gesture similar to Windows 10)
  • start drag action with touch similar to Windows 10 (Issue: drag Icon doesn't follow touch point at the moment)

BUG: 406359
BUG: 406361

Diff Detail

Repository
R119 Plasma Desktop
Branch
kickoff
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13075
Build 13093: arc lint + arc unit
steffenh created this revision.Jun 15 2019, 10:23 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 15 2019, 10:23 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
steffenh requested review of this revision.Jun 15 2019, 10:23 AM
ngraham accepted this revision.Jun 17 2019, 12:00 AM
ngraham added a subscriber: ngraham.

Very nice. You're becoming quite the expert on touch gestures in KDE software! :)

This works great on my touchscreen, and I did not find any regressions when using my touchpad or mouse. The diff is surprisingly small and seems sane to me. If you could work on the icon not following the cursor when dragged, that would be great, but from my perspective this is a ship-it as-is since it's a big improvement already. I'm giving it an "Approved" now and I'll bring it up to the Plasma folks at the Plasma spring that starts in three days.

applets/kickoff/package/contents/ui/KickoffListView.qml
205

remove extra leading space here

This revision is now accepted and ready to land.Jun 17 2019, 12:00 AM
ngraham edited the summary of this revision. (Show Details)Jun 17 2019, 12:00 AM
steffenh updated this revision to Diff 60018.Jun 18 2019, 11:51 AM
  • Drag icon now follows the touch point
  • to make the drag icon better visible, set the drag icon size for touch drag from medium to huge and move the drag icon a bit to the top and left
steffenh marked an inline comment as done.Jun 18 2019, 11:52 AM
davidedmundson added inline comments.
applets/kickoff/package/contents/ui/KickoffListView.qml
142

why do we need this?

204

we tend to useCamelCaseForVariables

steffenh marked an inline comment as done.Jun 18 2019, 6:41 PM
ngraham added inline comments.Jun 19 2019, 6:57 AM
applets/kickoff/package/contents/ui/KickoffListView.qml
142

Probably so click-and-hold doesn't trigger these behaviors when not using a touchscreen.

steffenh added inline comments.Jun 19 2019, 10:55 AM
applets/kickoff/package/contents/ui/KickoffListView.qml
142

This is the Fix for Bug: 406361
In the appView the listView.currentItem will get open, but listView.currentItem is only set in the onPositionChange handler.
If you make only a touch / tap you get no positionChange signal, with the resulted in the opening of the wrong category.
To fix this I call direkt the signal positionChanged, but only for a simple tap / touch event.

steffenh updated this revision to Diff 60216.Jun 21 2019, 11:30 AM

change variable name from tapandhold to tapAndHold

ngraham accepted this revision.Jun 21 2019, 2:54 PM

Works great for me.

Does a Plasma person approve?

hein added a subscriber: mart.EditedJun 21 2019, 4:43 PM

I'm in principle quite OK with this minus that other patch I need to be written first to move DragHelper out into the lib (see other comment). Pitch: If you do that, it'll make it much easier to make the other menus (and other things) act similarly later, with much smaller patches and cleaner code there.

However we need a collective decision that this way to handle tap-and-hold is the one we want to go forward with - this patch basically makes a call to resolve T10783 in a particular way - and needs some more buy-in e.g. from @mart for that reason.

applets/kicker/plugin/draghelper.cpp
91

I'm not happy with hardcoding these values. If we decide to do touch drag this way, we're going to see this being duplicated all around the codebase - with subtly different values, etc. If we want it that way we should centralize it somewhere.

We probably need to put DragHelper to plasma-framework - there's similar classes in FV and TM too we can consolidate. This isn't that much work and I'll gladly help mentoring the patch.

mart added a comment.Jun 23 2019, 3:15 PM

not entering in the code yet, i'm ok with it as a general UX behavior

@steffenh are you able to implement @hein's inline suggestions? I don't want this patch to get lost, it's good stuff.

Oh, sorry, I have the feeling I have misunderstood the post from @hein, I have the impression he wants to make a patch to DragHelper, but if I read this again, so sorry.

I have the last days looking at plasma-frameworks, but my knowledge of the working of plasma is close to zero, so I am afraid I'm not able to do this.

Perhaps is it better to remove the change in DragHelper, in the end the main point of this patch was to fix the two bugs?

hein added a comment.Jul 22 2019, 5:48 PM

I have the last days looking at plasma-frameworks, but my knowledge of the working of plasma is close to zero, so I am afraid I'm not able to do this.

Before we discuss alternatives: Are you willing to have me try and teach you? I'm actually going to be very busy with a move in the next few weeks, so this could end up taking quite significant time between the two of us, but if you have the time and patience I don't mind helping you through creating the patch I had in mind.

Hi @hein.

Before we discuss alternatives: Are you willing to have me try and teach you? I'm actually going to be very busy with a move in the next few weeks, so this could end up taking quite significant time between the two of us, but if you have the time and patience I don't mind helping you through creating the patch I had in mind.

If you can help me with that, that would be the great.