Differential D28017

Update dragCursor while dragging
ClosedPublic
Actions

Authored by trmdi on Mar 13 2020, 8:07 AM.

Details

Reviewers
meven
ngraham
davidedmundson
elvisangelaccio
Group Reviewers
Dolphin
Commits
R318:b96c48b44e6e: Update dragCursor while dragging
Summary

While dragging, the user could want to change the modifier, so we should call event->acceptProposedAction() to do update the dragCursor.

FIXED-IN: 20.04.0

Test Plan

While dragging files, change modifiers between Ctrl/Shift/Alt... and move the mouse at least 1px to see the cursor changes.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trmdi created this revision.Mar 13 2020, 8:07 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 13 2020, 8:07 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
trmdi requested review of this revision.Mar 13 2020, 8:07 AM
meven added a comment.Mar 13 2020, 8:19 AM

This works only once the mouse starts moving after the key modifier has been pressed.

trmdi added a comment.EditedMar 13 2020, 8:30 AM
In D28017#626772, @meven wrote:

This works only once the mouse starts moving after the key modifier has been pressed.

I try like this: start dragging > move the mouse (1) > press and hold modifier keys > move the mouse (at least 1px) (2)
and it works.

You mean it doesn't work without the step (2) ?

meven added a comment.Mar 13 2020, 8:43 AM
In D28017#626775, @trmdi wrote:
In D28017#626772, @meven wrote:

This works only once the mouse starts moving after the key modifier has been pressed.

I try like this: start dragging > move the mouse (1) > press and hold modifier keys > move the mouse (at least 1px) (2)
and it works.

You mean it doesn't work without the step (2) ?

Yes, the cursor does not change as the modifier is pressed.
At least despite the cursor does not change in the situation the drop event will run the expected action.
This makes the UI and behavior incoherent though.

So I guess we should add a keypress connection during a drag and disconnect it at the end caring only for modifiers and calling event->acceptProposedAction()

ngraham added a comment.Mar 13 2020, 8:38 PM
In D28017#626772, @meven wrote:

This works only once the mouse starts moving after the key modifier has been pressed.

That's a pre-existing issue unrelated to this patch: https://bugs.kde.org/show_bug.cgi?id=388259

It should definitely be fixed somehow, but it's probably not a blocker for this patch. However since it's related, if you can and want to fix it in this patch, that would be lovely!

I've been very impressed with your work lately, @trmdi. Great job!

trmdi updated this revision to Diff 77639.EditedMar 15 2020, 12:47 AM

Address @meven and @ngraham's comments.
Now you can change the modifier key without having to move the mouse.

ngraham edited the summary of this revision. (Show Details)Mar 15 2020, 3:03 AM
ngraham added a subscriber: elvisangelaccio.

Nice, it works! Not sure about the code, I'll let @meven or @elvisangelaccio review it.

trmdi added a reviewer: davidedmundson.Mar 15 2020, 12:28 PM

Is this a Qt bug or an intention? Because QDrag is used in many places.

davidedmundson added a comment.Mar 15 2020, 1:50 PM

Is this a Qt bug or an intention

which bit specifically?

davidedmundson added inline comments.Mar 15 2020, 2:36 PM
src/kitemviews/kitemlistcontroller.cpp
72

Is this a timer constantly timing out during a drag?

trmdi added a comment.Mar 15 2020, 4:06 PM
In D28017#627729, @davidedmundson wrote:

Is this a Qt bug or an intention

which bit specifically?

These ones:

1, When dragging, if you change the modifier key and not move the mouse afterwards -> the cursor does not change, no way to update the cursor except sending a MouseMoveEvent.
2, When you drag stuff from one process into another one the possible actions depends on the modifier key (e.g. ShiftModifier -> possibleActions = MoveAction)
If you change the modifier key without moving the mouse, the possibleActions doesn't change, while you expect the new action -> no way to update this except sending a MouseMoveEvent.

Are these bugs or not? Could this be fixed in QDrag or we have to fix our code?

trmdi added inline comments.Mar 15 2020, 4:24 PM
src/kitemviews/kitemlistcontroller.cpp
72

Yes, it's a Qt zero timer. During the drag, it eats all KeyPressEvent.

trmdi updated this revision to Diff 77672.Mar 15 2020, 5:58 PM

Set accepted state and dropAction of event more correctly.

davidedmundson added inline comments.Mar 15 2020, 6:08 PM
src/kitemviews/kitemlistcontroller.cpp
933

We have 2 concepts:

what action the event is accepted as
which widget (if you have N stacked on top of each other) accepts the event

If this widget ignores the event it shouldn't be changing the drop action

trmdi added inline comments.Mar 15 2020, 6:13 PM
src/kitemviews/kitemlistcontroller.cpp
933
you have N stacked on top of each other

Could this be possible in this case of Dolphin?

elvisangelaccio added a comment.Mar 15 2020, 6:22 PM

As far as I can see, bug #388259 has been fixed upstream (either in Qt or in the platform integration plugin).

As for this feature, the problem is it doesn't work on Wayland and I'm afraid that's a going to be a stopper.

trmdi added a comment.Mar 15 2020, 6:32 PM
In D28017#627845, @elvisangelaccio wrote:

As far as I can see, bug #388259 has been fixed upstream (either in Qt or in the platform integration plugin).

Could you try this? https://bugreports.qt.io/browse/QTBUG-75744

elvisangelaccio added a comment.Mar 15 2020, 7:06 PM
In D28017#627851, @trmdi wrote:
In D28017#627845, @elvisangelaccio wrote:

As far as I can see, bug #388259 has been fixed upstream (either in Qt or in the platform integration plugin).

Could you try this? https://bugreports.qt.io/browse/QTBUG-75744

With Qt 5.15 I get this debug output when i click that "drag" button:

Qt::CopyAction QFlags<Qt::DropAction>(CopyAction|MoveAction)

trmdi added a comment.EditedMar 17 2020, 4:28 AM
In D28017#627858, @elvisangelaccio wrote:

With Qt 5.15 I get this debug output when i click that "drag" button:

Qt::CopyAction QFlags<Qt::DropAction>(CopyAction|MoveAction)

Thanks, that means that bug is fixed in Qt 5.15.

This patch now try to change the cursor when the user change the modifier key and not move the mouse.
But I don't understand why it doesn't work in Wayland. Maybe another bug in Qt? If you open Dolphin (without this patch) drag a file, change the modifier key, then release the key, and drop it, it still behaves like you're holding that modifier key.

trmdi updated this revision to Diff 77969.Mar 19 2020, 3:47 AM

The patch now only updates the cursor while dragging.

trmdi edited the summary of this revision. (Show Details)Mar 19 2020, 3:48 AM
trmdi edited the test plan for this revision. (Show Details)
trmdi edited the test plan for this revision. (Show Details)
trmdi added inline comments.Mar 19 2020, 4:13 AM
src/kitemviews/kitemlistcontroller.cpp
933

If this widget ignores the event it shouldn't be changing the drop action

I think this only ignores the event in this widget, other one can accept it again when the drag enters it.

elvisangelaccio accepted this revision.Mar 19 2020, 10:20 PM
In D28017#628923, @trmdi wrote:
In D28017#627858, @elvisangelaccio wrote:

With Qt 5.15 I get this debug output when i click that "drag" button:

Qt::CopyAction QFlags<Qt::DropAction>(CopyAction|MoveAction)

Thanks, that means that bug is fixed in Qt 5.15.

This patch now try to change the cursor when the user change the modifier key and not move the mouse.
But I don't understand why it doesn't work in Wayland. Maybe another bug in Qt?

Yeah, I think so.

The patch is much smaller now, so I think we can merge it even if it doesn't work on Wayland.

This revision is now accepted and ready to land.Mar 19 2020, 10:20 PM
ngraham added a comment.Mar 19 2020, 10:32 PM

Stable branch?

davidedmundson added inline comments.Mar 19 2020, 10:40 PM
src/kitemviews/kitemlistcontroller.cpp
933

That's right, which is why changing the drop action from a widget which ignores the event is a weird thing to do.

(Not that it really matters)

trmdi added inline comments.Mar 20 2020, 1:42 AM
src/kitemviews/kitemlistcontroller.cpp
933

But setting the drop action is needed to change the cursor on the DragMove event.

trmdi added a comment.Mar 25 2020, 2:48 PM

@ngraham
It seems that everyone accepted this, could you help me to land this to whatever branch you want ?

Closed by commit R318:b96c48b44e6e: Update dragCursor while dragging (authored by trmdi, committed by ngraham). · Explain WhyMar 25 2020, 3:35 PM
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.Mar 25 2020, 3:35 PM

Done! Nice job.

Revision Contents
Changeset List

Diff 78464

View Options

src/kitemviews/kitemlistcontroller.cpp

Loading...