Fix accidentally triggering menus
ClosedPublic

Authored by rade on Mar 20 2018, 6:54 PM.

Details

Summary

As is there is a bug in qt where setActiveAction triggers menus. This code ensures that we only highlight non menu items to avoid triggering the menus until the menu is the only match.

Test Plan

Test matching code with a menu and a bookmark with the same beginning, it should pick the bookmark

Test the matching code with only menus that match the typed string

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
rade requested review of this revision.Mar 20 2018, 6:54 PM
rade created this revision.
rade added inline comments.Mar 20 2018, 11:23 PM
krusader/BookMan/krbookmarkhandler.cpp
615

No return here because we have not handled the event. So we are letting someone else see if they can handle it.

martinkostolny accepted this revision.Mar 21 2018, 12:56 AM

Nice! I have to note that this only works without issue if it is applied along with D11523.

This revision is now accepted and ready to land.Mar 21 2018, 12:56 AM
nmel requested changes to this revision.Mar 21 2018, 5:32 AM

Thanks! It works fine, however please address inline comments.

krusader/BookMan/krbookmarkhandler.cpp
580

firstMatch no longer corresponds to its name. It's the last match of a folder or the first match of a bookmark. Either rename (what's a good name?) or use two vars firstBookmarkMatch and firstFolderMatch.

606

space: // because...
in other comments too

615

Why do you think we haven't handled it in this case? We got the character, updated the search bar text, so it means we acknowledge the character that's been sent to search and not to do something else. IMO, we need return true here.

This revision now requires changes to proceed.Mar 21 2018, 5:32 AM
rade updated this revision to Diff 30126.Mar 21 2018, 2:07 PM

Added return statement from D11521 and renamed firstMatch

rade updated this revision to Diff 30128.Mar 21 2018, 2:16 PM

I have changed to using git format-patch, and phabricator doesn't like one file with several diffs in it apparently so I made them into one full diff.

rade marked 2 inline comments as done.Mar 21 2018, 2:27 PM
rade added inline comments.
krusader/BookMan/krbookmarkhandler.cpp
615

I imagine a scenario like so.

QMenu has added code so that when ever someone presses 'k' in a menu everybody gets a kitten.

If we have a bookmark called 'nokittens' and the user types 'nok' we want return true too tell QMenu that nobody gets a kitten because that was our 'k'.
However if we only have a bookmark called 'nopuppies' and the user types 'nok' then it's reasonable to say "We don't know why the user typed 'k', does anyone else know what to do?" and then QMenu goes, "Oh oh oh! I know what to do! You get a kitten, and you get a kitten. EVERYBODY GETS A KITTEN!!!"

krusader/BookMan/krbookmarkhandler.cpp
615

Now, that I understand better this part of code :) I think I as a user wouldn't like to 'get a kitten' when I typed "nok" because I definitely wanted to find a bookmark starting at least with "no". Only in case I'd press 'k' as a first letter I'd like to 'get a kitten'. Just my 2 cents :).

nmel added a comment.Mar 22 2018, 6:33 AM

For git format-patch, please only do it once your code is accepted — just attach it here like in this example:


Phabricator doesn't seem to understand this way — when I download raw diff from here, it's just has diff --git ... without From or other fields.

krusader/BookMan/krbookmarkhandler.cpp
615

Exactly. Since we already consumed 'k' for our purpose (and it's even reflected in the UI), it would be unnatural to let other widgets consume it too. By typing "no" we already entered the search mode and for this purpose we try to suppress other features like accelerators or any kitten-like hooks. Even though kittens are so warm and furry. BTW, thanks for the nice scenario, it made me smile today. ;)

rade updated this revision to Diff 30341.Mar 23 2018, 6:47 PM

Changed return code so that we return true, even if we couldn't find a matching bookmark or folder

rade marked 5 inline comments as done.Mar 23 2018, 6:49 PM
rade added inline comments.
krusader/BookMan/krbookmarkhandler.cpp
615

After thinking about it I agree. The rule of least surprise would seem to dictate that the same action will happen weather the bookmark was found or not.

rade marked 2 inline comments as done.Mar 23 2018, 6:49 PM
rade added a comment.EditedMar 23 2018, 6:53 PM
In D11525#231295, @nmel wrote:

For git format-patch, please only do it once your code is accepted — just attach it here like in this example:


Phabricator doesn't seem to understand this way — when I download raw diff from here, it's just has diff --git ... without From or other fields.

Right now, in order to be able to send you a single monolithic patch I'm doing a git reset, and then I commit all my changes again as a single commit. This is less than ideal as I loose all my commits which I might want later for personal reference. Option two is too create a new branch which I git reset, but that feels like a lot of commands for something that should be easy. Do you know of a good way to generate a single commit with git format-patch while still having several personal commits hidden behind the scenes?

nmel accepted this revision.Mar 24 2018, 3:31 AM
In D11525#232436, @rade wrote:

Right now, in order to be able to send you a single monolithic patch I'm doing a git reset, and then I commit all my changes again as a single commit. This is less than ideal as I loose all my commits which I might want later for personal reference. Option two is too create a new branch which I git reset, but that feels like a lot of commands for something that should be easy. Do you know of a good way to generate a single commit with git format-patch while still having several personal commits hidden behind the scenes?

New temporary branch is the way. For me, it doesn't feel hard as I do this visually with gitk and git gui. Just 3 easy steps: create branch, mixed reset, commit. This way you leave your personal branch intact. The hardest part is writing a proper message, IMO.

This revision is now accepted and ready to land.Mar 24 2018, 3:31 AM
This revision was automatically updated to reflect the committed changes.
nmel added a comment.Mar 24 2018, 6:00 AM

Thanks Rade for the patch!

krusader/BookMan/krbookmarkhandler.cpp
591

Just FYI, I fixed this message in the final patch.