[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: Review Request: [Dolphin] Fake click for short drag and drop on same item
From: Thomas_Lübking <thomas.luebking () web ! de>
Date: 2012-12-18 14:42:41
Message-ID: 20121218144241.2537.95589 () vidsolbach ! de
[Download RAW message or body]
> On Dec. 14, 2012, 3:56 p.m., Frank Reininghaus wrote:
> > First of all, thanks for the patch! I see now that dragging items accidentally \
> > when trying to click happens a lot for some users, and I agree that fixing this \
> > would be nice.
> > I'm just wondering if there is an easier way to do it, without making \
> > DragAndDropHelper depend on KItemListController and adding the hand-made \
> > press/release events. I have two ideas at the moment:
> > 1. Make KItemListController finally respect \
> > http://qt-project.org/doc/qt-4.8/qapplication.html#startDragTime-prop, and only \
> > start a drag in KItemListController::mouseMoveEvent() if the time since the mouse \
> > press event is larger than that, or
> > 2. Check in KItemListController::mouseMoveEvent() if the mouse cursor is still \
> > above the item where the press happened, and do not start a drag if that is the \
> > case.
> > Both approaches would prevent that the drag is started, whereas your idea was to \
> > sort of cancel the drag after it happened. Not starting the drag in the first \
> > place might be a bit cleaner and require less code from my point of view. But \
> > maybe there is a good reason why you chose to do it in a different way?
>
> Thomas Lübking wrote:
> Supporting the dragtimeout is even simpler, but the default of 500ms makes it \
> appear quite laggy on inteded DnD ops (you press, hold, drag and for the first half \
> second nothing happens) - esp. since apparently no Q/KItemView seems to care much \
> about it.
> If you'd advice ppl. to shorten the timeout to sth. "reasonable" (below 150ms), \
> they'll again run into the accidental drags.
> I'll update the patch later on to simply respect the timeout so that you can try \
> for yourself.
> Christoph Feck wrote:
> > you press, hold, drag and for the first half second nothing happens
>
> Unless you drag it far enough (= startDragDistance), this is the expected (and \
> correct) behaviour. Try window dragging by click-move on the title bar.
> Thomas Lübking wrote:
> Ok, thanks (never waited so log ;-)
> Just that the expected "autodrag w/o move" won't help on the particular issue \
> (since the drag distance is respected and effectively the problem)
> @Frank: we could simply keep the dragTime in the DragAndDropHelper instead to avoid \
> deps. on the controller.
> Kai Uwe Broulik wrote:
> If we'd just turn off that "A folder cannot be dropped onto itself" message, a huge \
> annoyance would be already gone ;) *scnr*
> Frank Reininghaus wrote:
> Thanks Thomas for the new patch and everyone else for the comments, in particular \
> Christoph - I somehow thought the idea was to start a drag when "distance > \
> startDragDistance" && "time since press > startDragTime", but with && replaced by \
> ||, it makes much more sense indeed :-)
> @Kai: "If we'd just turn off that "A folder cannot be dropped onto itself" message, \
> a huge annoyance would be already gone":
> No, in that case, we would get bug reports like "Clicking items fails randomly".
>
> @Thomas: I see that it's getting better :-) But I still think that this is a lot \
> more complex than it needs to be. Why not just put the timer into \
> KItemListController, start it when the mouse button is pressed, and in \
> mouseMoveEvent, if the drag distance is too small, check if the time since the \
> press is larger than startDragTime and do not start a drag if that is not the case. \
> This should mostly yield the same result and require a lot less code and no \
> dependencies between KItemListController and DragAndDropHelper, or am I overlooking \
> anything?
> Thomas Lübking wrote:
> This will effectively increase the QApplication::startDragDistance(), because for \
> the first 500ms -assuming default value in place- the drag distance is increased to \
> not "too small" and 500ms is much time in this context.
> You can try the outcome by increasing it legally in "kcmshell4 mouse" - it's capped \
> @ 20px, though, while a quick judder (the typical movement is down and up) can be \
> much more (esp. depending on screen resolution and mouse acceleration)
> I'm willing add an update, but notice that the approach omits the best variable for \
> the heuristic (button release, ie. drag time) and trap us between false positives \
> (dialog/notification) and laggy DnD behavior (even with 20px it's rather "strange" \
> when the DnD suddenly starts)
> If you just don't like the interdependency, it would be possible to (from the \
> controller) store the msecsSinceReference (kinda "current time", but monotic) as \
> dynamic property "DolphinDragStartTime" on the QApplication object and fetch and \
> compare it from there DnDHelper.
> Frank Reininghaus wrote:
> Well, I'm not a usability expert, but IMHO, not starting the drag if we think that \
> the user does not want to drag might be better and less distracting than starting \
> the drag, changing the mouse cursor for that, and then reverting the drag after the \
> drop.
> Code-wise, I also think that not starting the drag is better. It requires a lot \
> less code. Moreover, I'm worried that the hand-made press and release events might \
> lead to strange bugs in the future. They are delivered inside the nested event loop \
> of the drag, and I've really had a lot of unpleasant experiences with nested event \
> loops in the past.
> I see that replacing the check "distance > startDragDistance" by "distance > \
> startDragDistance || time > startDragTime" would not resolve the "accidental drag" \
> problem (it might make our behaviour more 'correct' though, but I just checked the \
> source of QAbstractItemView and found that it does not care about the startDragTime \
> either).
> What about the following idea: I guess that users never drag an item to drop it on \
> itself. We could (in KItemListController::mouseMoveEvent) determine the 'index' \
> which belongs to the position of the event and just not start the drag if this \
> index is equal to m_pressedIndex. I've suggested that earlier, but nobody commented \
> on that idea. Am I overlooking anything which is obviously wrong with that idea?
> not starting the drag if we think that the user does not want to drag might be \
> better
Yes, of course. The tricky part is to figure what the user wants.
The pre-action information is the drag distance, which you intend to increase (by \
tests in the former and last comments) what's (given the misclick isn't a dolphin \
only phenomenon) equal to telling the user to do so in the settings. This is oc. the \
best solution to prefer clicks, but comes with a pubishment to DnD.
If you simply increase it internally, be better prepared for "dolpin doen not respect \
dnd startdistance" bugs.
The startdistance actually exists for this purpose (smoothing away dirty clicks) and \
if usually sufficient, it's not reasonable to increase the setting, nor hardcode a \
higher value (just think of 128px icons...)
What is special about dolphin and source of the bug reports is its treatment of user \
failure. Usually, if a DnD cannot take place, the action silently quits (previously \
hinted by the "forbidden" cursor) but dolphin currently "annoys" the user with a (ui \
re-layouting) message (in this case false positive hint) to "punish" his failure \
(thus Kais scnr)
So my focus would not be on blocking the drag in the first place (there's a setting \
for this and dolphin does not differ from any other client in that regard) but on \
failure treatment.
This could also be a timeout on just showing the dialog - i just thought that since \
we pretty much (now!) know what the user intended, we could just do that for him.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107708/#review23469
-----------------------------------------------------------
On Dec. 14, 2012, 9:25 p.m., Thomas Lübking wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107708/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2012, 9:25 p.m.)
>
>
> Review request for Dolphin and Frank Reininghaus.
>
>
> Description
> -------
>
> When there's a relatively short click (i picked 500ms) and the item is moved to DnD \
> and released on itself, this is now assumed a "dirty click" (ie. the user actually \
> wanted to click, but juddered) instead of presenting a warning that an item cannot \
> be dragged on itself.
> Notes:
> - 500ms are quite some time. I can drag the icon out, back in and drop it in place.
> - due to the high abstraction level in the DnD processing and the application \
> window being the drag source, it is technically possible to split the view and DnD \
> an icon onto its other self within 500ms
> I'm however willing to state that if you manage to do either of that, you should \
> not have major issues on performing a regular click either. I picked the 500ms on \
> personal test (started with 1500, what seems far too much)
> - the reason for having the timeout in the first place is the assumption, that \
> users may actually intentionally try to drag an item on itself. Either because they \
> intend to link it there (link recursion can be dangerous, but is a legal action) or \
> for "ummm... i didn't want to copy that folder. errr... how do i stop this ... ok, \
> let's just put it back from where it came and hope for the best". Because of the \
> latter i think this should be hinted after the message freeze.
> - one might want to add a "don't ask again" checkbox to the hint and account that \
> by dropping the timeout
>
> This addresses bugs 283646, 297414, 307747, and 311483.
> http://bugs.kde.org/show_bug.cgi?id=283646
> http://bugs.kde.org/show_bug.cgi?id=297414
> http://bugs.kde.org/show_bug.cgi?id=307747
> http://bugs.kde.org/show_bug.cgi?id=311483
>
>
> Diffs
> -----
>
> dolphin/src/kitemviews/kitemlistcontroller.cpp 697e04f
> dolphin/src/views/draganddrophelper.h ac16f7c
> dolphin/src/views/draganddrophelper.cpp f81d4d0
>
> Diff: http://git.reviewboard.kde.org/r/107708/diff/
>
>
> Testing
> -------
>
> clickdragged a folder in both view (w/ and w/o scroll offset)
>
>
> Thanks,
>
> Thomas Lübking
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107708/">http://git.reviewboard.kde.org/r/107708/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On December 14th, 2012, 3:56 p.m., <b>Frank \
Reininghaus</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">First of all, thanks for the patch! I see now that dragging items \
accidentally when trying to click happens a lot for some users, and I agree that \
fixing this would be nice.
I'm just wondering if there is an easier way to do it, without making \
DragAndDropHelper depend on KItemListController and adding the hand-made \
press/release events. I have two ideas at the moment:
1. Make KItemListController finally respect \
http://qt-project.org/doc/qt-4.8/qapplication.html#startDragTime-prop, and only start \
a drag in KItemListController::mouseMoveEvent() if the time since the mouse press \
event is larger than that, or
2. Check in KItemListController::mouseMoveEvent() if the mouse cursor is still above \
the item where the press happened, and do not start a drag if that is the case.
Both approaches would prevent that the drag is started, whereas your idea was to sort \
of cancel the drag after it happened. Not starting the drag in the first place might \
be a bit cleaner and require less code from my point of view. But maybe there is a \
good reason why you chose to do it in a different way?</pre> </blockquote>
<p>On December 14th, 2012, 5:22 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Supporting the \
dragtimeout is even simpler, but the default of 500ms makes it appear quite laggy on \
inteded DnD ops (you press, hold, drag and for the first half second nothing happens) \
- esp. since apparently no Q/KItemView seems to care much about it.
If you'd advice ppl. to shorten the timeout to sth. "reasonable" (below \
150ms), they'll again run into the accidental drags.
I'll update the patch later on to simply respect the timeout so that you can try \
for yourself.</pre> </blockquote>
<p>On December 14th, 2012, 5:44 p.m., <b>Christoph Feck</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> you press, hold, \
drag and for the first half second nothing happens
Unless you drag it far enough (= startDragDistance), this is the expected (and \
correct) behaviour. Try window dragging by click-move on the title bar.</pre> \
</blockquote>
<p>On December 14th, 2012, 5:58 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ok, thanks (never waited \
so log ;-) Just that the expected "autodrag w/o move" won't help on the \
particular issue (since the drag distance is respected and effectively the problem)
@Frank: we could simply keep the dragTime in the DragAndDropHelper instead to avoid \
deps. on the controller.</pre> </blockquote>
<p>On December 14th, 2012, 6:52 p.m., <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If we'd just turn \
off that "A folder cannot be dropped onto itself" message, a huge annoyance \
would be already gone ;) *scnr*</pre> </blockquote>
<p>On December 16th, 2012, 7:12 p.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks Thomas for the \
new patch and everyone else for the comments, in particular Christoph - I somehow \
thought the idea was to start a drag when "distance > startDragDistance" \
&& "time since press > startDragTime", but with && \
replaced by ||, it makes much more sense indeed :-)
@Kai: "If we'd just turn off that "A folder cannot be dropped onto \
itself" message, a huge annoyance would be already gone":
No, in that case, we would get bug reports like "Clicking items fails \
randomly".
@Thomas: I see that it's getting better :-) But I still think that this is a lot \
more complex than it needs to be. Why not just put the timer into \
KItemListController, start it when the mouse button is pressed, and in \
mouseMoveEvent, if the drag distance is too small, check if the time since the press \
is larger than startDragTime and do not start a drag if that is not the case.
This should mostly yield the same result and require a lot less code and no \
dependencies between KItemListController and DragAndDropHelper, or am I overlooking \
anything?</pre> </blockquote>
<p>On December 16th, 2012, 8:19 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This will effectively \
increase the QApplication::startDragDistance(), because for the first 500ms -assuming \
default value in place- the drag distance is increased to not "too small" \
and 500ms is much time in this context.
You can try the outcome by increasing it legally in "kcmshell4 mouse" - \
it's capped @ 20px, though, while a quick judder (the typical movement is down \
and up) can be much more (esp. depending on screen resolution and mouse \
acceleration)
I'm willing add an update, but notice that the approach omits the best variable \
for the heuristic (button release, ie. drag time) and trap us between false positives \
(dialog/notification) and laggy DnD behavior (even with 20px it's rather \
"strange" when the DnD suddenly starts)
If you just don't like the interdependency, it would be possible to (from the \
controller) store the msecsSinceReference (kinda "current time", but \
monotic) as dynamic property "DolphinDragStartTime" on the QApplication \
object and fetch and compare it from there DnDHelper.</pre> </blockquote>
<p>On December 18th, 2012, 7:29 a.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, I'm not a \
usability expert, but IMHO, not starting the drag if we think that the user does not \
want to drag might be better and less distracting than starting the drag, changing \
the mouse cursor for that, and then reverting the drag after the drop.
Code-wise, I also think that not starting the drag is better. It requires a lot less \
code. Moreover, I'm worried that the hand-made press and release events might \
lead to strange bugs in the future. They are delivered inside the nested event loop \
of the drag, and I've really had a lot of unpleasant experiences with nested \
event loops in the past.
I see that replacing the check "distance > startDragDistance" by \
"distance > startDragDistance || time > startDragTime" would not \
resolve the "accidental drag" problem (it might make our behaviour more \
'correct' though, but I just checked the source of QAbstractItemView and \
found that it does not care about the startDragTime either).
What about the following idea: I guess that users never drag an item to drop it on \
itself. We could (in KItemListController::mouseMoveEvent) determine the \
'index' which belongs to the position of the event and just not start the \
drag if this index is equal to m_pressedIndex. I've suggested that earlier, but \
nobody commented on that idea. Am I overlooking anything which is obviously wrong \
with that idea? </pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> not starting the \
drag if we think that the user does not want to drag might be better Yes, of course. \
The tricky part is to figure what the user wants. The pre-action information is the \
drag distance, which you intend to increase (by tests in the former and last \
comments) what's (given the misclick isn't a dolphin only phenomenon) equal \
to telling the user to do so in the settings. This is oc. the best solution to prefer \
clicks, but comes with a pubishment to DnD.
If you simply increase it internally, be better prepared for "dolpin doen not \
respect dnd startdistance" bugs.
The startdistance actually exists for this purpose (smoothing away dirty clicks) and \
if usually sufficient, it's not reasonable to increase the setting, nor hardcode \
a higher value (just think of 128px icons...)
What is special about dolphin and source of the bug reports is its treatment of user \
failure. Usually, if a DnD cannot take place, the action silently quits (previously \
hinted by the "forbidden" cursor) but dolphin currently "annoys" \
the user with a (ui re-layouting) message (in this case false positive hint) to \
"punish" his failure (thus Kais scnr)
So my focus would not be on blocking the drag in the first place (there's a \
setting for this and dolphin does not differ from any other client in that regard) \
but on failure treatment.
This could also be a timeout on just showing the dialog - i just thought that since \
we pretty much (now!) know what the user intended, we could just do that for \
him.</pre> <br />
<p>- Thomas</p>
<br />
<p>On December 14th, 2012, 9:25 p.m., Thomas Lübking wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Dolphin and Frank Reininghaus.</div>
<div>By Thomas Lübking.</div>
<p style="color: grey;"><i>Updated Dec. 14, 2012, 9:25 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">When there's a relatively short click (i picked 500ms) and the item \
is moved to DnD and released on itself, this is now assumed a "dirty click" \
(ie. the user actually wanted to click, but juddered) instead of presenting a warning \
that an item cannot be dragged on itself.
Notes:
- 500ms are quite some time. I can drag the icon out, back in and drop it in place.
- due to the high abstraction level in the DnD processing and the application window \
being the drag source, it is technically possible to split the view and DnD an icon \
onto its other self within 500ms
I'm however willing to state that if you manage to do either of that, you should \
not have major issues on performing a regular click either. I picked the 500ms on \
personal test (started with 1500, what seems far too much)
- the reason for having the timeout in the first place is the assumption, that users \
may actually intentionally try to drag an item on itself. Either because they intend \
to link it there (link recursion can be dangerous, but is a legal action) or for \
"ummm... i didn't want to copy that folder. errr... how do i stop this ... \
ok, let's just put it back from where it came and hope for the best". \
Because of the latter i think this should be hinted after the message freeze.
- one might want to add a "don't ask again" checkbox to the hint and \
account that by dropping the timeout</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">clickdragged a folder in both view (w/ and w/o scroll offset)</pre> \
</td> </tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=283646">283646</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=297414">297414</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=307747">307747</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=311483">311483</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>dolphin/src/kitemviews/kitemlistcontroller.cpp <span style="color: \
grey">(697e04f)</span></li>
<li>dolphin/src/views/draganddrophelper.h <span style="color: \
grey">(ac16f7c)</span></li>
<li>dolphin/src/views/draganddrophelper.cpp <span style="color: \
grey">(f81d4d0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107708/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic