Added quick search bar to the bookmark menu + various fixes
ClosedPublic

Authored by nmel on Mar 11 2018, 8:55 AM.

Details

Summary

4 changes included:

Fixed populating bookmark menu multiple times on startup
On startup there were (number of tabs + 2) calls to the populate function.
Replaced with a single call in the bookmark handler singleton constructor.

Replaced ugly static variable with a proper design in bookmark menu builder

Added quick search bar to the bookmark menu
This allows users to see current text and also fixes quick search issue
in case no match is found that the search text was reset.

Removed outdated comments

Test Plan

Apply to the bookmark-quicksearch branch!

Test various patterns of bookmark search and accelerators.
Known issue: folders are behaving counter-intuitive some times, however the issue was there before and the bar just reveals it.

Diff Detail

Repository
R167 Krusader
Branch
bookmark-quicksearch-bar_arc
Lint
No Linters Available
Unit
No Unit Test Coverage
nmel requested review of this revision.Mar 11 2018, 8:55 AM
nmel created this revision.
nmel added reviewers: rade, martinkostolny.

I also spent a few hours trying to switch KrBookmark to QWidgetAction. I was able to make a progress, however the new menu items are styled and behave differently than regular. I found some hacks on the web and deep dived into painter code for menus. Finally, I have come to a conclusion we couldn't make a universal solution for all styles due to the way menu item are painted. Detailed explanation would be very long but in short, QStyle gets an action text and sends it to a style to paint while it doesn't provide a region for a custom widget if an icon and/or a shortcut is set.

That's why I decided to implement the search bar which also fixes not-found behavior. In case you like the approach, I can switch to just making matched menu items bold (the whole menu item can be set bold with QAction) and remove the hack with invisible underscores. We won't need to highlight matched chars as they are visible in the search bar. Please let me know.

nmel added a comment.Mar 11 2018, 9:14 AM

Just for the reference, the previous discussion regarding the branch is here.

martinkostolny requested changes to this revision.Mar 11 2018, 2:46 PM

Nicely done, Nikita!

I like where this is going. My personal preference would be using bold font for matched items instead of &-underline. My other personal preference would be to leave the quickSearchBar always visible so there is no visual jumping. Ideally with a placeholderText "Type to search...".

Please initialize PopularUrls before KrBookmarkHandler in krusader.cpp near line 158. Otherwise Krusader crashes on start because KrBookmarkHandler is using popularUrls. I realize this issue was here before but this patch uncovered it for me.

Please see my code comments. Thanks for your work!

krusader/BookMan/krbookmarkhandler.cpp
76

I'd prefer:

_quickSearchAction = new QWidgetAction(this);
_quickSearchBar = new QLineEdit(mainWindow->widget());

...this way the objects will be automatically destroyed when parent is destroyed. But I may be wrong.

This revision now requires changes to proceed.Mar 11 2018, 2:46 PM
rade requested changes to this revision.EditedMar 12 2018, 12:03 AM

Thanks for looking into QWidgetAction, and the code!

I agree with martin that having the field always visible would be better. It would help with discoverability. Currently it's not obvious that you can type in the bookmark menu.
An issue with not having it always visible is that if "popular urls" matches then_quickSearchBar disappears causing the sub-menu to be misaligned because everything else just moved up one space.

Using bold to mark the full text of the matches would be better as there is no telling when the current underlining hack might stop working.

A problem is that as _quickSearchAction is set too disabled the text becomes grey on a light grey background, at least with my theme.

.
Also looking at the documentation for QLineEdit it should be able to handle all manner of keyboard shortcuts for text editing (e.g Ctrl+backspace) but they don't work currently because it doesn't get focus.
Maybe the code could be changed so that _quickSearchBar gets focus once a letter is typed and the search code is triggered by one of its signals e.g textChanged?

nmel updated this revision to Diff 29377.Mar 13 2018, 4:50 AM
  • Fixed ownership of the quick search action
  • Fixed popular URLs container initialization order
nmel marked an inline comment as done.Mar 13 2018, 5:21 AM

Thanks Martin and Rade for reviewing and testing!

I didn't catch the Popular URLs crash as I had the submenu disabled. Thanks for letting me know.

For the bar visibility, I agree it should be visible by default, however I'd prefer to have an option to disable the visibility when nothing is typed. It's because I already know it's there and I only need this when I search bookmarks - in general scenario I'd like to save space. I can add a Konfiguratior option later. The branch is not ready for merging with master anyways.

It's good that we agree on bold highlighting.

The bar is disabled for a reason. It's just used for displaying the typed text and not for editing (I thought about using a QLabel but a disabled text field seemed to be a better fit; Rade, sorry that it's displayed poorly to your perception in your theme). The main idea is that it should not catch the focus. Consider my previous example with Downloads bookmark. I type d - o - Enter, because Downloads happens to be the first bookmark and it's focused. One can try to implement a more sophisticated logic like with the quick search for files, however this change is not about it.

For popular urls we hit the problem with folders that I stated in the Test Plan. Beside the menu height mismatch, if there is a plain bookmark that starts with X and folder that starts with X, when X is typed, the folder is selected and the following search happens in the folder. It's a bug, IMO. I'll try to find time to look into it.

Please accept the revision in case you think I should submit these changes to the branch, so everyone of us could make further improvements on top of it.

krusader/BookMan/krbookmarkhandler.cpp
76

Thanks! I was moving around the code and forgot to update the ownership. QLineEdit doesn't need the widget because the next call transfers ownership to the action. https://doc.qt.io/qt-5/qwidgetaction.html#setDefaultWidget

rade accepted this revision.EditedMar 13 2018, 10:40 PM

I don't agree that having the QLineEdit pop in just so that one extra bookmark can be displayed before typing is worth it. But either way works fine. Having it be an option as you suggested would be nice, but not really all that important. Especially as I think almost nobody will ever discover that there is an option for it.

martinkostolny accepted this revision.Mar 14 2018, 10:31 PM

Thanks for working in the code comments and the docs about setDefaultWidget - I wasn't aware of that. Accepting :). Sorry for my late response.

This revision is now accepted and ready to land.Mar 14 2018, 10:31 PM
This revision was automatically updated to reflect the committed changes.
nmel marked an inline comment as done.
nmel added a comment.Mar 15 2018, 7:25 AM

Thanks Martin and Rade for reviewing.