Add new touch-friendly drag handlers
ClosedPublic

Authored by Leon0402 on Aug 21 2019, 4:09 PM.

Details

Summary

Added new drag Handlers for the rectangular region option, which become free-floating) if there is not enough space (results in a bigger touch area for moving the rectangle) and are more touch-friendly.

BUG: 371843
FIXED-IN: 19.12.0

Test Plan

Before:

  • drag handles very small, not touch-friendly
  • If rectangle to small: drag handles too close together (or just removed in the newest version? The screenshot are from Plasma 5.16) and moving rectangle is not possible anymore (as touch areas for resizing fill out the rectangle)


After:

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Leon0402 created this revision.Aug 21 2019, 4:09 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 21 2019, 4:09 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
Leon0402 requested review of this revision.Aug 21 2019, 4:09 PM
ngraham retitled this revision from Fix Bug 371843 (https://bugs.kde.org/show_bug.cgi?id=371843): Added new touch-friendly drag Handlers to Add new touch-friendly drag handlers.Aug 21 2019, 4:15 PM
ngraham edited the summary of this revision. (Show Details)
ngraham added reviewers: Spectacle, VDG.

Thanks for the patch! There are some formatting and whitespace issues here; could you fix those up first?

src/QuickEditor/QuickEditor.cpp
53

Unrelated change

258

unnecessary whitespace change

450

unrelated change (should be fixed, but not in this patch)

828

unrelated change

src/QuickEditor/QuickEditor.h
72

unnecessary whitespace change

In testing it out, I really like it! I might even make the handles a bit larger for more touch-friendliness. Not a lot larger, just a bit.

filipf added a subscriber: filipf.Aug 21 2019, 4:29 PM

Can you add some before & after screenshots (or vids) for VDG ?

Leon0402 updated this revision to Diff 64231.Aug 21 2019, 4:31 PM

Removed unessecary changes

Leon0402 updated this revision to Diff 64232.Aug 21 2019, 4:33 PM
Leon0402 marked 3 inline comments as done.

Removed unnecessary changes

Leon0402 edited the test plan for this revision. (Show Details)Aug 21 2019, 4:44 PM
Leon0402 edited the test plan for this revision. (Show Details)Aug 21 2019, 5:10 PM
ngraham added inline comments.Aug 21 2019, 5:16 PM
src/QuickEditor/QuickEditor.cpp
28–33

best to never use decimal values when describing the size of UI elements, since it can and often does lead to blurriness and scaling anomalies at various scale factors.

Leon0402 marked an inline comment as done.Aug 21 2019, 5:23 PM

There is actually one "problem" I noticed. If you resize the rectangle from big to small (where the handles become free-floating), the cursor is at the end still there, where the handler should be in the normal mode. So the handler moved away from the cursor and you have to move the cursor back to the drag handler, if you want to resize the rectangle again. I'm actually not quite sure if that's a problem, but it's definitely different to how it behaved before the patch. One possible solution would be to move the cursor automatically to the drag handler it grabbed before. But it might look strange if the cursor suddenly jumps somewhere else?
Some opinions on this would be great!
@ngraham Is there a possibility to detect a touch device? If not what radius would you prefer? I also included an option to make the area, where resizing events are noticed bigger than the actual handles. At the moment the area is twice as big see "increaseDragAreaFactor". What value would you prefer for that?

There is actually one "problem" I noticed. If you resize the rectangle from big to small (where the handles become free-floating), the cursor is at the end still there, where the handler should be in the normal mode. So the handler moved away from the cursor and you have to move the cursor back to the drag handler, if you want to resize the rectangle again. I'm actually not quite sure if that's a problem, but it's definitely different to how it behaved before the patch. One possible solution would be to move the cursor automatically to the drag handler it grabbed before. But it might look strange if the cursor suddenly jumps somewhere else?
Some opinions on this would be great!

Moving the cursor is an option (but might it not work on Wayland?), though I'm not sure this is actually a serious problem in the first place.

@ngraham Is there a possibility to detect a touch device? If not what radius would you prefer? I also included an option to make the area, where resizing events are noticed bigger than the actual handles. At the moment the area is twice as big see "increaseDragAreaFactor". What value would you prefer for that?

Since 2-in-1 laptops can be interacted with using both touch and a traditional mouse or touchpad, I would instead check to see if each individual click was a touch event instead. For this, can use https://doc.qt.io/qt-5/qmouseevent.html#source

Once a touch event is detected, maybe enlarge the handles until the next click event.

Leon0402 updated this revision to Diff 64252.Aug 21 2019, 9:03 PM

Added "touch-mode": Adjusts the size of the handles if touch is used

@ngraham You have a 2-1 device, right? Could you please test if the the handles` radius increases as soon as you use touch and goes back to normal as soon as you use your mouse again? (change should happen on single click/touch)

Also, I would appreciate some feedback from anyone regarding the size of the handles in mouseMode, in touchMode and the factor to make the mouse area of the handles bigger (currently set to 2.0).

ngraham accepted this revision as: VDG, ngraham.Aug 21 2019, 9:44 PM
ngraham added a subscriber: davidre.

It works perfectly. Nicely done. The circle sizes seem just right, too.

@davidre, what do you think?

This revision is now accepted and ready to land.Aug 21 2019, 9:44 PM
Leon0402 updated this revision to Diff 64258.Aug 21 2019, 9:50 PM
Leon0402 marked 2 inline comments as done.

Removed unused property: Instead of a fixed size for the mouse area, it's a multiple of the handle`s size instead as it varies (touch or mouse mode)

davidre requested changes to this revision.EditedAug 22 2019, 8:37 AM

Doesn't work with multiple screens:

This revision now requires changes to proceed.Aug 22 2019, 8:37 AM
Leon0402 updated this revision to Diff 64304.Aug 22 2019, 1:12 PM

Temporary fix for multiple screen issue: Other parts (like the tooltip) don't handle multiple screens either. Will fix all these issue in a seperate patch

Much better. Would it be possible to only move the markers on the edge which is at the border or would that look weird?

This also removes the ability to resize the rectangle so that it can be moved when it's really small. I think we should keep that feature and only disable it at problematic sizes.

Leon0402 added a comment.EditedAug 22 2019, 1:52 PM

This is how it would look like


or like this

Personal opinion:

  • Not really better, but also not worse than the prior design
  • Would require major code changes. Currently there is only one offset for each handle (we would need an offset property for every handle then), currently spectacle just looks if there is at least one screen edge in it's way ... for the change we would need to find out, we would need to check very screen edge separately and handle "corner situations" (where it's too close to two edges) as the offset for the corner handle should then only be applied once and not twice -> Possibly even more (design?) issues when the rectangle is small and the handles become free-floating

It's probably not worth it and brings more disadvantages than advantages.

This also removes the ability to resize the rectangle so that it can be moved when it's really small. I think we should keep that feature and only disable it at problematic sizes.

In theory this should be automatically resolved as the handles become free-floating if the size is too small. So they already have an offset of say 40px, which would be decreased by radius/2 ... so it's still about 35px outside of the rectangle and moving the rectangle should be possible without problems (Disabling the feature then might be still a good idea, as the space between the handles should be consistent). Or did I misunderstood you?
This means also that you cannot resize the rectangle on the edges atttached to the screen edges, if the rectangle is very small. But I cannot imagine anyone having a small rectangle in the screen corner/ at the screen edge and then wanting to make the rectangle even smaller from that side.

This is how it would look like

Personal opinion:

  • Not really better, but also not worse than the prior design
  • Would require major code changes. Currently there is only one offset for each handle (we would need an offset property for every handle then), currently spectacle just looks if there is at least one screen edge in it's way ... for the change we would need to find out, we would need to check very screen edge separately and handle "corner situations" (where it's too close to two edges) as the offset for the corner handle should then only be applied once and not twice -> Possibly even more (design?) issues when the rectangle is small and the handles become free-floating

    It's probably not worth it and brings more disadvantages than advantages.

This also removes the ability to resize the rectangle so that it can be moved when it's really small. I think we should keep that feature and only disable it at problematic sizes.

In theory this should be automatically resolved as the handles become free-floating if the size is too small. So they already have an offset of say 40px, which would be decreased by radius/2 ... so it's still about 35px outside of the rectangle and moving the rectangle should be possible without problems (Disabling the feature then might be still a good idea, as the space between the handles should be consistent). Or did I misunderstood you?
This means also that you cannot resize the rectangle on the edges atttached to the screen edges, if the rectangle is very small. But I cannot imagine anyone having a small rectangle in the screen corner/ at the screen edge and then wanting to make the rectangle even smaller from that side.

Whoops the sentence is missing the important part. I was referring to the ability to resize the rectangle anywhere on it's edges not only on the dots.

This is how it would look like


or like this

Personal opinion:

  • Not really better, but also not worse than the prior design
  • Would require major code changes. Currently there is only one offset for each handle (we would need an offset property for every handle then), currently spectacle just looks if there is at least one screen edge in it's way ... for the change we would need to find out, we would need to check very screen edge separately and handle "corner situations" (where it's too close to two edges) as the offset for the corner handle should then only be applied once and not twice -> Possibly even more (design?) issues when the rectangle is small and the handles become free-floating

    It's probably not worth it and brings more disadvantages than advantages.

This also removes the ability to resize the rectangle so that it can be moved when it's really small. I think we should keep that feature and only disable it at problematic sizes.

In theory this should be automatically resolved as the handles become free-floating if the size is too small. So they already have an offset of say 40px, which would be decreased by radius/2 ... so it's still about 35px outside of the rectangle and moving the rectangle should be possible without problems (Disabling the feature then might be still a good idea, as the space between the handles should be consistent). Or did I misunderstood you?
This means also that you cannot resize the rectangle on the edges atttached to the screen edges, if the rectangle is very small. But I cannot imagine anyone having a small rectangle in the screen corner/ at the screen edge and then wanting to make the rectangle even smaller from that side.

Another idea would be to only move them away from the edge but not change the height:

Okay going to implement the feature that only some of the handles move inside (and we can see then if it's worth it) and going to implement the feature that resizing work on the rectangle`s edges works as well if the rectangle is not too small.

Leon0402 updated this revision to Diff 64334.Aug 22 2019, 5:23 PM

Implemented new behaviour of handlers when they touch a screen edge
Rectangle can no be resized on edges, if the rectangle is big enough (>= 100x100)

Leon0402 edited the test plan for this revision. (Show Details)Aug 22 2019, 5:28 PM
davidre accepted this revision.Aug 23 2019, 9:01 AM

Works really well now. I tested multipel edges corner, too (including whole screen selection). Code is also good from my side since I never really looked into it before.
What does VDG/@ngraham think about the new look when near a corner?

This revision is now accepted and ready to land.Aug 23 2019, 9:01 AM
ngraham accepted this revision.Aug 23 2019, 1:29 PM

I think the corner look/behavior is just fine! Very nice work.

This revision was automatically updated to reflect the committed changes.