D16643: Correct the accept flag of the event object on DragMove

bcooksley added a comment. Your name has now been updated. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc: bcooksley, fvogt, aacid, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. In D16643#406076 , @bcooksley wrote: > Please update your name on Identity, then i'll sync it from there. Done. Thank you! REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643

D16643: Correct the accept flag of the event object on DragMove

bcooksley added a comment. Please update your name on Identity, then i'll sync it from there. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc: bcooksley, fvogt, aacid, bruns, dkorth, ngraham,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. 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 REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma,

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. Never mind, missed that this was for a Frameworks repo. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc: fvogt, aacid, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

fvogt closed this revision. fvogt added a comment. Landed (phab didn't notice): https://cgit.kde.org/kdeclarative.git/commit/?id=856672f370fb32c7a3c3d1a13d873fb8b767d0e8 REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma,

D16643: Correct the accept flag of the event object on DragMove

bcooksley added a comment. Phabricator can take up to 30 minutes to become aware of commits depending on how active a repository normally is. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc:

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. Hmm, should this go to the 5.15 branch too? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc: fvogt, aacid, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

fvogt accepted this revision. fvogt added a comment. @trmdi: Do you have push access? If not, which name should be used for the commit? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns, fvogt Cc: fvogt,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. In D16643#406051 , @fvogt wrote: > @trmdi: Do you have push access? If not, which name should be used for the commit? No, I don't. tr...@yandex.com / Tranter Madi REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

trmdi updated this revision to Diff 50979. REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16643?vs=47815=50979 REVISION DETAIL https://phabricator.kde.org/D16643 AFFECTED FILES src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp To: trmdi, mart,

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. In D16643#405814 , @fvogt wrote: > @trmdi: Can you do the small change @bruns suggested? ^ Then it can be landed and everyone's happy :-)  REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

fvogt added a comment. @trmdi: Can you do the small change @bruns suggested? ^ Then it can be landed and everyone's happy :-) REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: fvogt, aacid, bruns, dkorth,

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

fvogt added a comment. > 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

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. 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 ||

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. @bruns? Last call before we land this. :) REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: fvogt, aacid, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

fvogt added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

aacid added a comment. @bruns waiting for your comment on whether your issues were addressed or not REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: aacid, bruns, dkorth, ngraham, kde-frameworks-devel,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. But wait, can we completely remove `m_oldDragMovePos` ? If the event is accepted in the case `event->pos() == m_oldDragMovePos`, then why don't we generate the move event? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To:

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. @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 REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

trmdi edited the summary of this revision. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. A lot of folks are probably out on vacation, BTW. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. What will happen if @bruns doesn't want to reply here anymore? I've updated the patch, can you review it ? @broulik REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: anthonyfieroni,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. !ping REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, bruns Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi updated this revision to Diff 47815. trmdi edited reviewers, added: bruns; removed: davidedmundson. REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16643?vs=44860=47815 REVISION DETAIL https://phabricator.kde.org/D16643 AFFECTED FILES

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. Is it better if I change the line 91 to `event->setAccepted(m_enabled && !m_temporaryInhibition)` @bruns ? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni,

D16643: Correct the accept flag of the event object on DragMove

broulik added a comment. 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. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. In D16643#378823 , @broulik wrote: > Only if that potential issue with inhibition has been addressed You mean @bruns is right and I'm wrong? REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

broulik added a comment. Only if that potential issue with inhibition has been addressed REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham,

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. !ping @hein Could you land this patch? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. 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? REPOSITORY

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. No competence here, sorry. Perhaps @bruns or a #plasma person can help. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc:

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. This should not be accepted as is, it breaks the temporaryInhibition case when a DropArea is child of another REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc:

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. I don't know what to do next, can you help me @ngraham ? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel,

D16643: Correct the accept flag of the event object on DragMove

mart accepted this revision. This revision is now accepted and ready to land. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

mart requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel,

D16643: Correct the accept flag of the event object on DragMove

mart accepted this revision. This revision is now accepted and ready to land. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

hein added a comment. @mart ping REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. In D16643#355964 , @trmdi wrote: > In D16643#355177 , @bruns wrote: > > > You have a conceptual misunderstanding of the enabled property, see QQuickItem

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. In D16643#355177 , @bruns wrote: > You have a conceptual misunderstanding of the enabled property, see QQuickItem > > > enabled : bool > > This property

D16643: Correct the accept flag of the event object on DragMove

anthonyfieroni added inline comments. INLINE COMMENTS > DeclarativeDropArea.cpp:97 > if (event->pos() == m_oldDragMovePos) { > -event->setAccepted(false); > return; Change it to accept() and try it. REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. !Ping Reviewers Please review this. It's just a very simple patch. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: bruns, dkorth, ngraham, kde-frameworks-devel,

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. In D16643#354262 , @trmdi wrote: > In D16643#354120 , @bruns wrote: > > > `m_enabled` may change between //consecutive// DragMove events, but not during an event. > > > >

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

trmdi updated this revision to Diff 44860. trmdi added a comment. git diff -U -- DeclarativeDropArea.cpp > patch.diff REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16643?vs=44784=44860 REVISION DETAIL https://phabricator.kde.org/D16643

D16643: Correct the accept flag of the event object on DragMove

ngraham added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

trmdi edited the summary of this revision. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi edited the summary of this revision. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. @ngraham How to see the full file content from this page? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. 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: >

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. In D16643#353670 , @bruns wrote: > No. The event loop is running in a single thread. m_enabled is constant during the function. Can it be changed from outside? REPOSITORY R296 KDeclarative REVISION DETAIL

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

trmdi added a comment. @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 dragable icon So I let it be on the top to make the cursor be able to change the icon to

D16643: Correct the accept flag of the event object on DragMove

bruns added a comment. 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

D16643: Correct the accept flag of the event object on DragMove

trmdi retitled this revision from "Correct m_enabled on DragMove " to "Correct the accept flag of the event object on DragMove ". REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D16643 To: trmdi, mart, broulik, #plasma, hein, davidedmundson Cc: dkorth, ngraham,