Fixed jump-back actions in bookmark menu, improved separators
ClosedPublic

Authored by nmel on Mar 18 2018, 4:46 AM.

Details

Summary

This fixes the issue when bookmark search is started with 's' or 'j'.
Previously these letters were accelerators coming from the Go global menu,
which prevented from searching bookmarks started from the letters.
Now, the bookmark menu has a dedicated actions without the accelerators.

NOTE: The feature development is in progress. All accepted reviews will be applied only to the bookmark-quicksearch branch, so please accept if a patch solves the corresponding problem. For the branch merge to master I will send another code review where we can verify that every issue is addressed accordingly. I will also try to summarize the issues found in previous CRs.
Test Plan
NOTE: apply on top of bookmark-quicksearch branch.

Confirm that jump back actions are working from both Go menu and bookmark menu.
Check if designated shortcuts are working.

Diff Detail

Repository
R167 Krusader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nmel requested review of this revision.Mar 18 2018, 4:46 AM
nmel created this revision.
nmel edited the test plan for this revision. (Show Details)Mar 18 2018, 4:48 AM
nmel added reviewers: martinkostolny, rade.
nmel added a project: Krusader.
nmel edited the summary of this revision. (Show Details)Mar 20 2018, 6:09 AM
nmel edited the test plan for this revision. (Show Details)
rade requested changes to this revision.Mar 20 2018, 7:08 PM

Read through the code and it looks good.
One 'problem' though is that the shortcut 'Ctrl+j' and 'Ctrl+shift+j' used to be displayed next to jump back point and set jump back point the same way they still are in the go menu. That should probably be fixed.

This revision now requires changes to proceed.Mar 20 2018, 7:08 PM
rade added inline comments.Mar 20 2018, 7:09 PM
krusader/BookMan/krbookmark.cpp
127

I suspect this is where you are overwriting the 'Ctrl+j' part of the name.

Agreed with Rade about the missing displayed shortcuts. Otherwise it works nicely, thanks!

nmel marked an inline comment as done.Mar 21 2018, 3:30 AM

I'm intentionally not copying the shortcut - in case I do copy and issue the shortcut, krusader claims there are ambiguous shortcuts because there are two actions with the same shortcut. Although the shortcuts for the two actions are not shown on bookmark menu, they still work because they are assigned to source actions (from Go menu).

krusader/BookMan/krbookmark.cpp
127

Earlier on the line 122 I put a comment: shortcut is not copied as it will introduce ambiguity. removeAcceleratorMarker only removes accelerator key marked with &.

rade added a comment.Mar 21 2018, 11:42 AM

You could add back the the shortcut text only using
´´´

QString shortcutText = sourceAction->shortcut().toString();

´´´

But then the text obviously doesn't get right justified like the other shortcut text. It cold be right justified manually when we detect the show event and there for know the size of the menu. But that's hacky, and doesn't deal with if the menu gets resized etc.

The better option then too me would seem to be to not copy the sourceaction at all. Just add it to the menu like we used to do. Split the begining if in eventfilter that detects the show and close event into two ifs one for show and one for close. In the if that handles show, right after we do reset, just strip the accelerator from set jump back and jump back, and put the original strings in _quickSearchOriginalActionTitles. Once the menu closes they'll get their accelerators back again.

nmel updated this revision to Diff 30185.Mar 22 2018, 4:53 AM
nmel marked an inline comment as done.
  • Fixed shortcuts for jump-back actions in bookmark menu
  • Made order of jump-back actions the same as in Go menu
nmel added a comment.Mar 22 2018, 4:55 AM

I have found a way to assign shortcuts without introducing ambiguity. Please test and accept if it works.

martinkostolny accepted this revision.Mar 22 2018, 10:33 PM

It works:), thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Mar 23 2018, 4:36 AM
This revision was automatically updated to reflect the committed changes.
nmel added a comment.Mar 23 2018, 4:37 AM

Thanks for testing! :)