D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a comment. In D8958#212246 , @ngraham wrote: > @aleksejshilin doesn't have commit access Actually, I do now. :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin,

D8958: Fix unintentional breadcrumb menu item activation

This revision was automatically updated to reflect the committed changes. Closed by commit R241:4a6a3b81bedc: Fix unintentional breadcrumb menu item activation (authored by aleksejshilin). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8958?vs=26872=27866

D8958: Fix unintentional breadcrumb menu item activation

ngraham added a comment. @aleksejshilin doesn't have commit access, so would you like to land this, @dfaure? REPOSITORY R241 KIO BRANCH fix_accidental_breadcrumb_menu_item_activation REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks, dfaure Cc:

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin edited the summary of this revision. REPOSITORY R241 KIO BRANCH fix_accidental_breadcrumb_menu_item_activation REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks, dfaure Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Ah, OK. REPOSITORY R241 KIO BRANCH fix_accidental_breadcrumb_menu_item_activation REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks, dfaure Cc:

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a comment. In D8958#209050 , @dfaure wrote: > Instead of the bool member, isn't it enough to test the distance again in mouseReleaseEvent? AFAIK that's how most widget do it. It also leads to one difference of behaviour in

D8958: Fix unintentional breadcrumb menu item activation

dfaure added a comment. Instead of the bool member, isn't it enough to test the distance again in mouseReleaseEvent? AFAIK that's how most widget do it. It also leads to one difference of behaviour in case someone moves the mouse a bit and then back to the original position, in that case

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks, dfaure Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a comment. In https://phabricator.kde.org/D8958#203931, @ngraham wrote: > OK cool. The patch works for me! +1. Great, thanks a lot! > Get a code review from someone else before committing. I don't have push access, so no worries. :) REPOSITORY R241

D8958: Fix unintentional breadcrumb menu item activation

ngraham added a comment. OK cool. The patch works for me! +1. Get a code review from someone else before committing. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added inline comments. INLINE COMMENTS > ngraham wrote in kurlnavigatormenu.cpp:85 > Will this still work if the user is using the mouse in left-handed mode? Quoting Qt documentation [1]: > | Qt::LeftButton | The left button is pressed, or an event refers to the left > button.

D8958: Fix unintentional breadcrumb menu item activation

ngraham added inline comments. INLINE COMMENTS > kurlnavigatormenu.cpp:85 > +// it unless mouse was moved. > +if (m_mouseMoved || (btn != Qt::LeftButton)) { > +QAction *action = actionAt(event->pos()); Will this still work if the user is using the mouse in left-handed mode?

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin updated this revision to Diff 26872. aleksejshilin added a comment. - Don't consider mouse moves which are smaller than drag distance - Don't pass the ignored mouse release event to the base class REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D8958: Fix unintentional breadcrumb menu item activation

ngraham added a comment. Would you mind re-basing this on master? I can reproduce the issue and would be happy to test the patch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a comment. Hi again. :) Any news on this? Is there anything I should improve? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin added a comment. So, any comments on the updated revision? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8958 To: aleksejshilin, #frameworks Cc: broulik, ngraham

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin updated this revision to Diff 22866. aleksejshilin added a comment. - Don't consider mouse moves which are smaller than drag distance - Don't pass the ignored mouse release event to the base class REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D8958: Fix unintentional breadcrumb menu item activation

broulik added a comment. I think you should store the mouse position and then check whether the `manhattanLength()` between new and old is `>= QApplication::startDragDisance()` to be consistent with regular click and drag drop handling – not everyone is capable of holding a mouse perfectly

D8958: Fix unintentional breadcrumb menu item activation

aleksejshilin created this revision. aleksejshilin added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION SUMMARY Since breadcrumb menu is opened on mouse press, it may receive the corresponding mouse release event which may unintentionally activate the