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.
Details
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
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. |
Nice! I have to note that this only works without issue if it is applied along with D11523.
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... | |
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. |
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.
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'. |
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 :). |
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. ;) |
Changed return code so that we return true, even if we couldn't find a matching bookmark or folder
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. |
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.
Thanks Rade for the patch!
krusader/BookMan/krbookmarkhandler.cpp | ||
---|---|---|
591 | Just FYI, I fixed this message in the final patch. |