Differential D10954

Add back search as you type too the bookmark menu
AbandonedPublic
Actions

Authored by nmel on Mar 1 2018, 9:56 PM.

Details

Reviewers
rade
Group Reviewers
Krusader
Summary

Originally submitted as email here. And tested by Martin Kotelnik.
Original text of the first email copy pasted below

Before the upgrade to Qt5 you could go to any bookmark in the bookmark
menu by simply typing the shortest unique first part of its name.
This functionality disappeared in the upgrade because kde frameworks 5
removed the functions setKeyboardShortcutsEnabled and
setKeyboardShortcutsExecute (see the commented out code in
krbookmarkbutton.cpp). As I could find no equivalent functions in kde
framework 5 I wrote some code to emulate their functionality instead.
My code seems to be functionally identical with the old one with two
exceptions
1)In krusader 2.4beta3 the text that matches what you have already typed
is underlined for each of the bookmarks
2)In my code '&' shortcuts (naming a bookmark '&movies' means 'm' is a
shortcut for that bookmark) have absolute priority. This means that if
you have three bookmarks named 'abc', 'abd', and, 'k&bc' and then type
'ab' it will open the 'kbc' bookmark, because of the 'b' shortcut. In
krusader 2.4beta3 once you have typed more than one character all the
shortcuts are disabled assuming none of them matched the first letter.

The below is the license for the patched that's attached too this email.
It's the same as the original file but i figured it might be worth
explicitly specifying just in case it is needed.
/*****************

  • This program is free software; you can redistribute it and/or modify *
  • it under the terms of the GNU General Public License as published by *
  • the Free Software Foundation; either version 2 of the License, or *
  • (at your option) any later version. * * *
  • This package is distributed in the hope that it will be useful, *
  • but WITHOUT ANY WARRANTY; without even the implied warranty of *
  • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
  • GNU General Public License for more details. * * *
  • You should have received a copy of the GNU General Public License *
  • along with this package; if not, write to the Free Software *
  • Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301

USA *
*****************/

Test Plan

Suggested test plan

Verify behaviour by comparing to krusader 2.4beta3

Confirm that both the lack of underlining and the bookmark behaviour are acceptable

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
rade requested review of this revision.Mar 1 2018, 9:56 PM
rade created this revision.
rade edited the summary of this revision. (Show Details)Mar 1 2018, 10:32 PM
nmel requested changes to this revision.Mar 2 2018, 8:19 AM
nmel added a subscriber: nmel.

First of all, thanks a lot for the patch!

Three concerns beside the code style:

  1. Ampersand bookmarks are activated in the middle of typing. I have "&working dirs" and "downloads", typing d - o - w selects the first one. Kind of unnatural. I suggest only activate a bookmark by a shortcut if it's the first typed symbol only.
  2. Since you handle backspace, probably we should use some UI element to show what's typed already. This is not a blocker.
  3. When I type just 's', the menu just closes. Can you repro or it's just my setup? BTW, debugging is tricky since menu captures keypresses - I just locked the whole DE and had to kill debugger from the console - what's the ideal debugging strategy?
krusader/BookMan/krbookmarkhandler.cpp
481

space: // Ha...

484

Please use the following style for all ifs:

if (cond) {
    body;
}

i.e. fix spaces.

485

style: QKeyEvent *kev

499

style: QList<QAction *>

502

auto act

since act is a pointer

503

remove double space

krusader/BookMan/krbookmarkhandler.h
84

Why not just QString?

This revision now requires changes to proceed.Mar 2 2018, 8:19 AM
martinkostolny added a subscriber: martinkostolny.Mar 3 2018, 3:05 PM

Thanks, Rade, for submitting your patch here! I'm really looking forward to this feature.

Although I know there are code style inconsistencies throughout Krusader code, I also agree with Nikita's suggestions. We should try to be more consistent. I'd like to add this one:

if (cond) {
    body;
} else {
    body;
}

And one more suggestion - I'd prefer using nullptr instead of NULL.

To the points Nikita wrote:
ad 1) I agree with this.
ad 2) Also agree, but this can be done later, not necessarily in this patch.
ad 3) I can reproduce it. I think the menu should stay open if no bookmark is found. As for debugging, I'd simply suggest using wayland session -> no application is allowed to eat all keypresses there.

From my testing I've found this issue: I have bookmark-submenu and when this submenu gets activated the whole menu closes. Which is logical for regular actions but not submenus. I'd expect the menu to stay up and submenu to be populated. This is probably also not that important for now.

rade updated this revision to Diff 28530.EditedMar 4 2018, 1:17 AM
rade marked 6 inline comments as done.

I have updated the code. Sorry for the large amount of changes, if requested I can submit the small style fixes and the large code revision separately. I have tried to follow the coding standard this time, hopefully I haven't missed anything.

  1. Ampersand in the middle of bookmarks now work as expected i.e they do not trigger unless they correspond to the first letter typed. So do&wnload will _not_ trigger on the key sequence d-o-w but only if the first letter pressed is w.
  1. I have updated the code so that all entries matching the currently typed text have their matching part underlined. So if you have entries dow1 and dow2 typing dow will display _d_o_w_1 and _d_o_w_2 . This is not how it worked in 2.4.0-beta3, there only one of the matches was underlined not all of them.

Underlining all of them is however necessary here because I use the bookmark shortcut to underline. Because of this either I underline only one match, but then I can't underline the first letter until the second one is typed (or the bookmark code will activate it). Or I have to underline all matches. The latter is preferable in my opinion.
I would even argue that this is a feature not a bug. I have sometimes been confused as to why a bookmark didn't trigger with what I have already typed and had to hunt for what other bookmarks could be matching the typed string. Having all matches underlined makes the other matches much easier to find.

  1. I would guess that what is happening is that you are triggering the "Set jump back point" shortcut as that has a default bookmark of s. So this is technically expected behaviour. As far as I can tell there is no way for the user to disable this bookmark. I suggest that the default bookmarks are removed from both "Set jump back point" and "jump back". My 'search as you type' code gives easy access to them even without the bookmarks and if one needs even faster access to them one could always use Ctrl+shift+j and Ctrl+j. Regarding debugging: I have been using QtDebug to sprinkle print statements through out the code. That has been working fine so far but I'm starting to run up against its limitations.

re:martinkostolny's notes
I have switched to using nullptr.
If by submenus you mean bookmark folders they seem to be working with this patch, you can even 'search as you type' the items inside the folder. However see known defects below.

Known defects:
If you create a folder then that folder gets triggered by the first letter in its name. similar to a bookmark. It is however not triggered in the way normal bookmarks are and I don't know what is triggering it. It could be argued that this is expected behaviour, I don't know what the documentation says on the issue.

The "Popular Urls" bookmark is not responding to my activate call. I'm certain my code is calling it but it just does nothing. All of the other special bookmarks like "Trash" and "Local network" work fine

rade updated this revision to Diff 28531.EditedMar 4 2018, 1:25 AM

fixed misplaced else.

nmel requested changes to this revision.Mar 4 2018, 5:27 AM

Hi Rade,

Thanks a lot for your rework - it is much better now. Coding style is in conformance - thanks a lot!

Let me comment issues one by one.

  1. Ampersand - I tested that it works as expected!
  2. I don't have a problem with your way of underlining and I see how this mitigates the search box absence. One problem - I don't see underlines. It's most likely unrelated to your patch - I don't see underlined letters in my bookmark list at all, even though they have '&' as seen in the bookmark editor. I'll try to figure this out. Current way of underlining is kind of hacky and may bite us back. I encourage to add a comments regarding the hack at least. Ideally, find a way to change text style / decoration in menu and consider to use bold to not conflict with one letter shortcuts.
  3. 's' and 'j' - I may think about this as minor issue if I hadn't have bookmarks in the list that start with s and j. In fact, I have many. With current patch there is no search being performed but "Set Jump Back Point" and "Jump back" are triggered correspondingly.
  4. Folder behavior - I haven't tested it yet.
  5. Not found handling - I disagree with _menuSearch = ""; The next letter after the one that triggered not-found case will start a new search. This feels unnatural especially given you support Backspace.
  6. I haven't set my mind regarding special bookmarks (Trash, Lan, Popular, etc). On one hand, they are in the menu and users might want to include them into search results. On the other hand, it may interfere with user desires to fully control search-as-you-type (he/she may setup names so that they quickly searched by one or two letters, however built-in bookmarks will spoil this setup). I'll think more about it and want to hear Martin's opinion. FYI, "Manage bookmarks" also participate in the search, it's not really nice. Grey area, kind of bug-or-feature.

Anyway, this is a good progress - thanks a lot for your effort and dedication! I really like the feature and we can help solving the issues - just let us know how.

Thanks,
Nikita.

krusader/BookMan/krbookmarkhandler.cpp
588

join 'i=', 'while', '++i' into a single 'for'

krusader/BookMan/krbookmarkhandler.h
84

Hmm, I can't mark it as done... weird...

This revision now requires changes to proceed.Mar 4 2018, 5:27 AM
martinkostolny added a comment.Mar 4 2018, 12:46 PM

Thanks Rade for the great progress and Nikita for proper review!

ad 1) Perfect, thanks!
ad 2) I find it hacky but visually perfect as well :). I agree it should be commented in code so we can find more proper solution later. To actually see the underlines one needs to press alt or set an option in System Settings -> Application Style -> Widget Style -> Applications (tab) -> Breeze:Configure -> General (tab) -> Keyboard accelerators visibility ... to "Always Show [...]".
ad 3) and 6) My opinion is we don't search in special bookmarks. They have dedicated shortcuts so that is at least one reason for that.
ad 4) please review my code comment, it should trigger opening of bookmark folder
ad 5) I agree with Nikita on this, I wouldn't reset the search either. It can also cause accidental triggering a & letter bookmarks after some letters are already typed

Thanks a lot for your effort. It is looking really nice so far! :)

krusader/BookMan/krbookmarkhandler.cpp
497

one space behind if :)

536

I'm suggesting this instead of line 536:

if ((bool) found->menu()) {
    menu->setActiveAction(found);
} else {
    found->activate(QAction::Trigger);
}

It will open a folder menu when such menu entry is "activated".

nmel added a comment.Mar 6 2018, 5:19 AM

Since this is almost complete, I propose that I put current patch in a remote branch and author the commit as Rade - then we can have smaller iterations on each issue separately. Code reviews will have diffs based on the current code - it's more convenient. In short, git style. When we are good, I'll merge the branch into the master branch.

If I get no objection, I plan to do this in 24h.

Rade, do I have your permission to use your full name and email (as seen in krusader-devel) for the initial commit? If not, I'll just mention you in the commit message.

rade updated this revision to Diff 28878.EditedMar 7 2018, 12:09 AM
rade marked 4 inline comments as done.

Thanks for reviewing my code again.

You can use my name that's no problem.

What is the correct behaviour when the user inputs a string that isn't found? To ignore the last typed character and keep matching the last found string? Or something else?

I don't like using & too underline either, it's just a matter of time before qt changes the bookmark behaviour in a way that breaks my hack. E.g by starting to match &a&b against "a".
I think the proper way to implement it would be to switch the krbookmark class too inherit from QwidgetAction instead of QAction. That way you could use basic html tags to stylise the string. Since QWidgetAction inherits QAction it might be pretty much a drop in replacement with minimal code changes. However I need too look into it further to make sure there are no unintended consequences.

"My opinion is we don't search in special bookmarks. They have dedicated shortcuts so that is at least one reason for that."
On my install only 3/7 of the special bookmarks have dedicated shortcuts. Trash,local network,virtual filesystem, and manage bookmarks don't have a shortcut (at least not an underlined one) as far as I can tell.

"'s' and 'j' - I may think about this as minor issue if I hadn't have bookmarks in the list that start with s and j. In fact, I have many. With current patch there is no search being performed but "Set Jump Back Point" and "Jump back" are triggered correspondingly."
One way to avoid this would be to instead set these shortcuts to the first character in their string that don't match the first character of any of the bookmarks. I believe that was how krusader 2.4-beta3 did it. Personally I don't like that solution though because whenever a shortcut changes it takes me a long time change my muscle memory. Having that shortcut potentially change every time I add a bookmark would be kinda awful....

I have added code to reset on close (line 475 " ev->type() == QEvent::Close") to mitigate a bug that causes krusader to crash with a segfault. If you remove that line krusader will crash if you

  1. have two bookmarks with >1 character in common e.g ark and art and a folder with an arbitrary name
  2. Open a menu and type something that will match the bookmarks e.g "ar"
  3. Use the mouse to select manage bookmarks
  4. move one of the bookmarks (might have too be the one that was highlighted) into the folder
  5. Try to open the bookmark menu again. Krusader crashes

The crash is caused by line #593 "i.key()->setText(i.value())".

nmel added a comment.Mar 8 2018, 6:02 AM

I've tested again and created a remote branch bookmark-quicksearch with the current patch. Please don't update patch in this code review but submit new Differentials on top of the branch. Thanks!

nmel added a comment.Mar 8 2018, 6:10 AM

Rade, To answer your question about the not-found handling, IMO, it's better to store those symbols in the _menuSearch but underline only a match portion. User can erase them with the backspace or close the menu and retry again. Ideally, we can show a search bar as with the file quick search.

nmel commandeered this revision.Mar 18 2018, 6:06 AM
nmel abandoned this revision.
nmel edited reviewers, added: rade; removed: nmel.

I'm closing this as I landed this patch into the bookmark-search branch a while ago.

Revision Contents
Changeset List

Diff 28531

View Options

krusader/BookMan/krbookmarkhandler.h

Loading...
View Options

krusader/BookMan/krbookmarkhandler.cpp

Loading...