D8958: Fix unintentional breadcrumb menu item activation

2018-02-23 Thread Алексей Шилин 
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, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-23 Thread Алексей Шилин 
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&id=27866

REVISION DETAIL
  https://phabricator.kde.org/D8958

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-23 Thread Nathaniel Graham 
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: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-23 Thread Алексей Шилин 
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

2018-02-18 Thread David Faure 
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: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-18 Thread Алексей Шилин 
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 case someone moves the mouse a bit and then back to 
the original position, in that case the mouseReleaseEvent considers that no 
move happened (not sure if that's what you want here).
  
  
  If user moved the mouse considerably, then it's assumed to be intentional, 
and the subsequent release event shouldn't be ignored even if the final 
position is the same as the initial one. This way it is consistent with 
drag'n'drop behavior.
  
  Besides, only the first mouse release event should be ignored, so a flag is 
needed anyway.

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

2018-02-18 Thread David Faure 
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 the mouseReleaseEvent considers that no 
move happened (not sure if that's what you want here).

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

2018-02-16 Thread Алексей Шилин 
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

2018-02-10 Thread Алексей Шилин 
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 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8958

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-10 Thread Nathaniel Graham 
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

2018-02-10 Thread Алексей Шилин 
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. (**The left button may be the right button on left-handed mice.**) |
> |

Should work just fine.

[1] http://doc.qt.io/qt-5/qt.html#MouseButton-enum

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8958

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-10 Thread Nathaniel Graham 
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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8958

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-10 Thread Алексей Шилин 
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
  https://phabricator.kde.org/D8958?vs=22866&id=26872

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

REVISION DETAIL
  https://phabricator.kde.org/D8958

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

2018-02-10 Thread Nathaniel Graham 
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

2018-02-10 Thread Алексей Шилин 
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

2017-12-07 Thread Алексей Шилин 
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

2017-11-23 Thread Алексей Шилин 
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
  https://phabricator.kde.org/D8958?vs=22788&id=22866

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

REVISION DETAIL
  https://phabricator.kde.org/D8958

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks
Cc: broulik, ngraham

D8958: Fix unintentional breadcrumb menu item activation

2017-11-23 Thread Kai Uwe Broulik 
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 still.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8958

To: aleksejshilin, #frameworks
Cc: broulik, ngraham

D8958: Fix unintentional breadcrumb menu item activation

2017-11-22 Thread Алексей Шилин 
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 random item which happens to be under mouse pointer.
  
  This commit makes the menu ignore the first mouse release event
  unless the pointer was moved. It fixes the issue and still allows
  'Press mouse button' -> 'Move to an item' -> 'Release the button'
  usage scenario.
  
  BUG: 380287

REPOSITORY
  R241 KIO

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

REVISION DETAIL
  https://phabricator.kde.org/D8958

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks