Wow, does this really fix 396011!? If so, congratulations! I wish I could test, but alas (or thankfully?) I don't experience the problem.

This fixes the issue for me for both desktop icons and panel widgets.

Using KF 5.51, Plasma 5.14.2, and Qt 5.11.2.

In D16643#353517, @ngraham wrote:

Wow, does this really fix 396011!? If so, congratulations! I wish I could test, but alas (or thankfully?) I don't experience the problem.

Yes, it fixes the problem on my system.
But I am not sure, because I've just started learning C++ recently and this is my first patch. It could be wrong, but I hope someone else could improve it.

So if I understand it correctly, the sequence of events is:

• an object is dragged to the target area
• m_enabled is initially false
• m_enabled becomes true because the object is acceptable
• the mouse is moved, but the integer position probably is unchanged, so the event is ignored/rejected

In any case, I think the setAccepted(m_enabled) should go below the first if statement, and as m_enabled == true holds afterwards, you can use event->accept().

@bruns
If setAccepted goes below the first if:

• if there is a change that make m_enabled change from true -> false while moving, the cursor still display the draggable icon

So I let it be on the top to make the cursor be able to change the icon to forbidden one right away.

In D16643#353669, @trmdi wrote:

@bruns
If setAccepted goes below the first if:

• if there is a change that make m_enabled change from true -> false while moving, the cursor still display the draggable icon
  
  So I let it be on the top to make the cursor be able to change the icon to forbidden one right away.

This is not covered by your summary - you have only listed cases where the drag is erroneously not accepted. Please update the summary.

In D16643#353670, @bruns wrote:

This is not covered by your summary - you have only listed cases where the drag is erroneously not accepted. Please update the summary.

I wrote it in the first line:

m_enabled could change while moving.

@ngraham
How to see the full file content from this page? "Show raw file" doesn't work. Sorry, I'm new to Phabricator.

In D16643#353679, @trmdi wrote:

@ngraham
How to see the full file content from this page? "Show raw file" doesn't work. Sorry, I'm new to Phabricator.

You need to either use arc to create the diff (see https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist) or else add context manually by using git diff -U9999 to generate your diff.

In D16643#353672, @trmdi wrote:

In D16643#353670, @bruns wrote:

This is not covered by your summary - you have only listed cases where the drag is erroneously not accepted. Please update the summary.

I wrote it in the first line:

m_enabled could change while moving.

You made the summary worse ...

m_enabled may change between consecutive DragMove events, but not during an event.

In this case, though, it does not, and is constantly true. You can verify this by attaching a debugger or by adding debug statements.

The only wrong code part is the setAccepted(false), which should be setAccepted(true) (or just accept(), both are equivalent).

In D16643#354120, @bruns wrote:

m_enabled may change between consecutive DragMove events, but not during an event.

In this case, though, it does not, and is constantly true. You can verify this by attaching a debugger or by adding debug statements.

The only wrong code part is the setAccepted(false), which should be setAccepted(true) (or just accept(), both are equivalent).

Please see these videos, note the cursor icon:

• If I place setAccepted(m_enabled) below the first if like you said: https://streamable.com/oj1j4 -> the cursor does not change
• If I place setAccepted(m_enabled) above the first if like I've done in my patch: https://streamable.com/2g58g -> the cursor changes

In D16643#354262, @trmdi wrote:

In D16643#354120, @bruns wrote:

m_enabled may change between consecutive DragMove events, but not during an event.

In this case, though, it does not, and is constantly true. You can verify this by attaching a debugger or by adding debug statements.

The only wrong code part is the setAccepted(false), which should be setAccepted(true) (or just accept(), both are equivalent).

Please see these videos, note the cursor icon:

• If I place setAccepted(m_enabled) below the first if like you said: https://streamable.com/oj1j4 -> the cursor does not change
• If I place setAccepted(m_enabled) above the first if like I've done in my patch: https://streamable.com/2g58g -> the cursor changes

You have a conceptual misunderstanding of the enabled property, see QQuickItem

enabled : bool
This property holds whether the item receives mouse and keyboard events. By default this is true.

It would also be good if you stick to one issue at a time.

!Ping Reviewers

Please review this. It's just a very simple patch.

In D16643#355177, @bruns wrote:

You have a conceptual misunderstanding of the enabled property, see QQuickItem

enabled : bool
This property holds whether the item receives mouse and keyboard events. By default this is true.

Yes, but what do you think about this:
http://doc.qt.io/qt-5/qevent.html#accepted-prop

accepted : bool
Setting the accept parameter indicates that the event receiver wants the event.

Isn't DropArea the event receiver? What's wrong when I use m_enabled for setAccepted(bool accepted) ?

It would also be good if you stick to one issue at a time.

Why for a very simple one-line patch ?

In D16643#355964, @trmdi wrote:

In D16643#355177, @bruns wrote:

You have a conceptual misunderstanding of the enabled property, see QQuickItem

enabled : bool
This property holds whether the item receives mouse and keyboard events. By default this is true.

Yes, but what do you think about this:
http://doc.qt.io/qt-5/qevent.html#accepted-prop

accepted : bool
Setting the accept parameter indicates that the event receiver wants the event.

Isn't DragArea the event receiver?

If it does not receive any events, it can and must not accept. All events go to the parent.

In D16643#355965, @bruns wrote:

If it does not receive any events, it can and must not accept. All events go to the parent.

DeclarativeDropArea::dragMoveEvent(QDragMoveEvent *event) is called only when it receives the event, isn't it?
When it does not receive any events like you said, it does nothing. So what's wrong with my patch ? It only corrects the accept flag when it is the event receiver, nothing else.

@mart ping

I don't know what to do next, can you help me @ngraham ?

This should not be accepted as is, it breaks the temporaryInhibition case when a DropArea is child of another

No competence here, sorry. Perhaps @bruns or a Plasma person can help.

In D16643#370642, @bruns wrote:

This should not be accepted as is, it breaks the temporaryInhibition case when a DropArea is child of another

Ok, could you write a simple QML file to show that case?

!ping @hein

Could you land this patch?

Only if that potential issue with inhibition has been addressed

In D16643#378823, @broulik wrote:

Only if that potential issue with inhibition has been addressed

You mean @bruns is right and I'm wrong?

I'm not blaming anyone, I'm merely stating that there is a potential issue with this patch that might not have been addressed and needs @bruns further input.

Is it better if I change the line 91 to event->setAccepted(m_enabled && !m_temporaryInhibition) @bruns ?

!ping

What will happen if @bruns doesn't want to reply here anymore?
I've updated the patch, can you review it ? @broulik

A lot of folks are probably out on vacation, BTW.

@broulik
I've updated the patch, can you review it ?

In D16643#378823, @broulik wrote:

Only if that potential issue with inhibition has been addressed

@bruns waiting for your comment on whether your issues were addressed or not

I tried to understand what this change does both by trying to reproduce the issue and reading Qt code.
Here the symptom was more drags not getting accepted at all than flipping back and forth, but this patch fixes that as well.

What might cause confusion is that the DeclarativeDropArea::enabled property is unrelated to QQuickItem::enabled.
If the latter is set to disabled, events don't get delivered at all (== rejected) and this should work directly (so I wonder why
this has a custom property at all).

From what I can tell this patch actually does two independent changes:

• Not actively ignoring an event if the move didn't change position (this is enough to fix my case, but apparently not for @trmdi, can you clarify?)
• Actively rejecting an event if the area is "disabled" or "temporarily inhibited" to prevent stealing of the event by any parent areas

IMO the first change is absolutely correct - actively reejecting a move breaks the drag.

The second change seems to be necessary because DragMove events are considered to be accepted by the current drag target item by default.
So it needs to be rejected actively if "disabled" or "inhibited". This is normally done by Qt itself for disabled items, but as this reimplements a custom
enabled property it needs to be done manually.

I suggest to wait until Monday whether @bruns has something to add, but if there's no objection this can IMO be landed.

@bruns? Last call before we land this. :)

As fvogt already said, the current (old) code is wrong, the event should not be blindly ignored (line 97).

The second change fvogt mentioned is IMHO handled quite strangely here - !m_enabled || m_temporaryInhibition is the inverse of !m_enabled || m_temporaryInhibition, so doing a event->ignore() === event->setAccepted(false) prior to the first return statement would cover the ignored case, and allows the event to bubble up.

This leaves the cases where the area is neither disabled nor inhibited. For large moves (pos() changed >= 1.0 pixels), the event is accepted, and the event is delivered to the quick item, where it can be left as is (accepted), or reset to ignored by the code.

Why is a small move handled differently? Shouldn't the if (event->pos() == oldDragMovePos) code path be removed completely?

The second change fvogt mentioned is IMHO handled quite strangely here - !m_enabled || m_temporaryInhibition is the inverse of !m_enabled || m_temporaryInhibition, so doing a event->ignore() === event->setAccepted(false) prior to the first return statement would cover the ignored case, and allows the event to bubble up.

Sure, but AFAICT that's just a matter of taste whether to ignore(); if(!something) return; do stuff; accept(); or accept(something); if(!something) return; do stuff;.

Why is a small move handled differently? Shouldn't the if (event->pos() == oldDragMovePos) code path be removed completely?

To not send a move event without position changes. DeclarativeDragDropEvent only has integer positions. Removing this condition might break existing code.

In D16643#405468, @fvogt wrote:

The second change fvogt mentioned is IMHO handled quite strangely here - !m_enabled || m_temporaryInhibition is the inverse of !m_enabled || m_temporaryInhibition, so doing a event->ignore() === event->setAccepted(false) prior to the first return statement would cover the ignored case, and allows the event to bubble up.

Sure, but AFAICT that's just a matter of taste whether to ignore(); if(!something) return; do stuff; accept(); or accept(something); if(!something) return; do stuff;.

Its about the duplicate evaluation of !m_enabled || m_temporaryInhibition. Expecially as it is not easy to spot both conditions are exactly the same.

compare

event->setAccepted(m_enabled && !m_temporaryInhibition);
if (!m_enabled || m_temporaryInhibition) {
    return;
}

with

if (!m_enabled || m_temporaryInhibition) {
    event->ignore();
    return;
}
event->accept();

@trmdi: Can you do the small change @bruns suggested? ^ Then it can be landed and everyone's happy :-)

In D16643#405814, @fvogt wrote:

@trmdi: Can you do the small change @bruns suggested? ^ Then it can be landed and everyone's happy :-)

👍

but it would also good if the summary were cleaned up - the issue is not caused by "m_enabled changed during DragMove", but by handling the event incorrectly in the "small move" code path.

Also, I have a hard time guessing what "too difficult" in the test plan means, without any prior knowledge.

In D16643#406051, @fvogt wrote:

@trmdi: Do you have push access? If not, which name <mail> should be used for the commit?

No, I don't.
trmdi@yandex.com / Tranter Madi

Hmm, should this go to the 5.15 branch too?

Never mind, missed that this was for a Frameworks repo.

Phabricator can take up to 30 minutes to become aware of commits depending on how active a repository normally is.

Nice, my first patch finally has been accepted.
Thanks everyone!

Btw, can you @bcooksley help me to change my name in my profile? Tranter Madi

Please update your name on Identity, then i'll sync it from there.

In D16643#406076, @bcooksley wrote:

Please update your name on Identity, then i'll sync it from there.

Done.
Thank you!

Your name has now been updated.