Details

Reviewers

martinkostolny
rade

Group Reviewers

Krusader

Commits

R167:2bbaedfebaf7: Added a setting for bookmark search: Search in special items

Summary

Since this was one of the final tasks in nmels email I figured I would submit a patch. I _do not_ feel this should be merged. I have merely submitted it so that we can discuss the merits of searching or not searching the specialbookmarks.

My arguments for why they should be searched are:

It is useful to search them. Say I visit 'trash' often or 'local network'. There is no way for me to assign them an accelerator key so being able too type their name to quickly access them without having to take my hand off the keyboard would be nice.

Because they are the last items in the menu they will never be highlighted by default, so if the user has a bookmark, trik. Then he can just type 't + enter' to access it instead of the trash bookmark.

The rule of least surprise. There are no indications too the user that they are special and will not be searched.

Test Plan

Discuss the merits of the patch

Diff Detail

Repository

R167 Krusader

Branch

arcpatch-D11624

Lint

No Linters Available

Unit

No Unit Test Coverage

rade requested review of this revision.Mar 23 2018, 8:22 PM

rade created this revision.

rade edited the summary of this revision. (Show Details)

fixed whitespace

nmel mentioned this in D11623: Add set jump back et al. to specialbookmarks to fix right click bug.Mar 24 2018, 5:18 AM

This should be applied on top of D11623

I added this issue in the email as I collected feedback from different reviews. The discussion of this one started around here:

In D10954#217886, @nmel wrote:

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.

In D10954#218233, @martinkostolny wrote:

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.

So these are arguments for this patch.

Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.

Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.

Sounds good to me. Would you mind doing the patch for it though? I'm not really sure how the config code works, and you seem to have a good grasp of it.

In D11624#233131, @rade wrote:

Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.

Sounds good to me. Would you mind doing the patch for it though? I'm not really sure how the config code works, and you seem to have a good grasp of it.

Yep, no problem.

In D11624#233198, @nmel wrote:

In D11624#233131, @rade wrote:

Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.

Sounds good to me. Would you mind doing the patch for it though? I'm not really sure how the config code works, and you seem to have a good grasp of it.

Yep, no problem.

Thanks! Should I abandon this revision then and you'll submit a new one?

In D11624#233131, @rade wrote:

Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.

Sounds good to me. Would you mind doing the patch for it though? I'm not really sure how the config code works, and you seem to have a good grasp of it.

I agree with this approach - config option "Search in special items", off by default. Thanks!

I'm taking over to improve the change as we discussed.

Added Konfigurator option for bookmark search: Search in special items

Restricted Application added a subscriber: kde-doc-english. · View Herald TranscriptMar 25 2018, 4:15 AM

Fixed diff.

Sorry, I first pushed diff for the whole branch as I forgot to specify the base.

I found that with the option turned on jump back actions could be searched but don't close the menu when triggered. It's unrelated to this patch, however we need to look into this. For now, it's unclear to me why it's happening — ideas are appreciated.

ideas are appreciated

I think there has to be an explicit menu-close call like in the rest of the actions to close the menu. All the other actions call KrBookmarkHandler::slotActivated which in turn calls _mainBookmarkPopup->close(). There are only 4 actions without this call (please correct me if I'm wrong): set jump back, jump back, add bookmark, manage bookmarks. The last two open new dialog which probably in turn closes popup menu.

Therefore we should handle differently activation of the 2 jump-back actions so that after their work is done, popup is requested to close. Please see my code suggestions.

krusader/BookMan/krbookmarkhandler.cpp
469–470	inside the if: connect(action, &QAction::triggered, this, [=] { if (_mainBookmarkPopup && !_mainBookmarkPopup->isHidden()) { _mainBookmarkPopup->close(); } });
469–470	same here: connect(action, &QAction::triggered, this, [=] { if (_mainBookmarkPopup && !_mainBookmarkPopup->isHidden()) { _mainBookmarkPopup->close(); } });

krusader/BookMan/krbookmarkhandler.cpp

469–470

inside the if:

connect(action, &QAction::triggered, this, [=] {
    if (_mainBookmarkPopup && !_mainBookmarkPopup->isHidden()) {
        _mainBookmarkPopup->close();
    }
});

469–470

same here:

connect(action, &QAction::triggered, this, [=] {
    if (_mainBookmarkPopup && !_mainBookmarkPopup->isHidden()) {
        _mainBookmarkPopup->close();
    }
});

This revision is now accepted and ready to land.Mar 25 2018, 7:32 PM

Other then the issue martin pointed out it looks good!

Closed by commit R167:2bbaedfebaf7: Added a setting for bookmark search: Search in special items (authored by nmel). · Explain WhyMar 26 2018, 6:55 AM

This revision was automatically updated to reflect the committed changes.

nmel mentioned this in D11706: Fixed the issue with jump back actions not closing bookmark menu when triggered.Mar 26 2018, 6:59 AM

Thanks Martin and Rade for the review!

Thanks Martin for finding the root cause and for code suggestions. I've seen CONNECT_BM before and completely forgot about it thinking of a closing menu as a standard behavior. I created D11706 separately as I decided to go a little further and remove code dup. Please check it out. If you like it, I can refactor CONNECT_BM the same way some time later.

FYI, this change needs a docbook update. Thanks.

nmel mentioned this in D11898: Implemented bookmark quick search.Apr 3 2018, 6:10 AM

			Path	Packages
M			doc/bookmarks.docbook (6 lines)
M			doc/konfigurator.docbook (10 lines)
M			krusader/BookMan/krbookmark.h (15 lines)
M			krusader/BookMan/krbookmark.cpp (36 lines)
M			krusader/BookMan/krbookmarkbutton.cpp (4 lines)
M			krusader/BookMan/krbookmarkhandler.h (14 lines)
M			krusader/BookMan/krbookmarkhandler.cpp (215 lines)
M			krusader/Konfigurator/kgpanel.cpp (16 lines)
M			krusader/krusader.cpp (5 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	30351			Mar 23 2018, 8:10 PM	★	★
Diff 2	30355			Mar 23 2018, 9:02 PM	★	★
Diff 3	30463	cb7711f	- Added Konfigurator option for bookmark search: Search in special items	Mar 25 2018, 4:15 AM	★	★
Diff 4	30465	d17d5fb	Fixed diff.	Mar 25 2018, 4:16 AM	★	★
Diff 5	30591	d17d5fb	R167:2bbaedfebaf7a61fe2018de1d04cff7e8abcf1a2	Mar 26 2018, 6:43 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
9705b043382c	1e2da846911e	c43087ac7c71	Nikita Melnichenko	Added Konfigurator option for bookmark search: Search in special items	Mar 25 2018, 4:14 AM
c43087ac7c71	51630f0e7c18	d17d5fb4ddf1	Nikita Melnichenko	Prevent quicksearch from searching special bookmarks (Show More…)	Mar 25 2018, 3:54 AM
d17d5fb4ddf1	d91dabf59dcf	0697fa50911d	Nikita Melnichenko	Add back Jump Back actions to special bookmarks (Show More…)	Mar 25 2018, 3:43 AM
0697fa50911d	a71d5ff63348	2841d0f45dc6	Rade Deguerre	Fixed accidentally triggering menus during bookmark search (Show More…)	Mar 23 2018, 6:44 PM
2841d0f45dc6	89f7723aa486	8d050ac98fd5 6fd0ed74d8d3	Nikita Melnichenko	Fixed bookmark search when started with S or J, improved menu style (Show More…)	Mar 23 2018, 4:34 AM
6fd0ed74d8d3	99c527591f5a	16befc43215b	Nikita Melnichenko	Made order of jump-back actions the same as in Go menu	Mar 22 2018, 4:44 AM
16befc43215b	7e07d0ee3019	f515bfa6b40a	Nikita Melnichenko	Fixed shortcuts for jump-back actions in bookmark menu	Mar 22 2018, 4:41 AM
f515bfa6b40a	8b9d3256c356	42858f0ea2c9	Nikita Melnichenko	Fixed jump-back actions in bookmark menu, improved separators (Show More…)	Mar 18 2018, 2:45 AM
8d050ac98fd5	d8dc17865141	2364a0106dd4	Yuri Chornoivan	Add the new option to Konfigurator docs	Mar 22 2018, 5:55 PM
2364a0106dd4	1e2e74874143	c1f2a3cb0bb1	Nikita Melnichenko	Added Konfigurator option to control bookmark search bar visibility (Show More…)	Mar 21 2018, 6:42 AM
c1f2a3cb0bb1	5e33a1fd6bf4	42858f0ea2c9	Rade Deguerre	Fixed key handling bug and added a QoL improvement (Show More…)	Mar 22 2018, 6:04 AM
42858f0ea2c9	aa4a82dbfedf	82da5a6d784b 059b9ef5bcae	Nikita Melnichenko	Improved match highlighting in the bookmark quick search (Show More…)	Mar 20 2018, 5:56 AM
059b9ef5bcae	aa4a82dbfedf	2eb7ee02e6be	Nikita Melnichenko	Deselect active item in case no match is found	Mar 17 2018, 8:19 PM
2eb7ee02e6be	0abe3f6bac72	82da5a6d784b	Nikita Melnichenko	Replaced underline hack with bold highlighting in the bookmark search (Show More…)	Mar 16 2018, 8:53 AM
82da5a6d784b	da2bf23c40a3	21900f61f34a f6cc251224f6	Nikita Melnichenko	Added quick search bar to the bookmark menu + various fixes (Show More…)	Mar 15 2018, 7:16 AM
f6cc251224f6	da2bf23c40a3	a39557553c79	Nikita Melnichenko	Fixed popular URLs container initialization order (Show More…)	Mar 13 2018, 4:45 AM
a39557553c79	dc790987c99c	84dc008285c3	Nikita Melnichenko	Fixed ownership of the quick search action	Mar 13 2018, 4:32 AM
84dc008285c3	f8e9e1c7d7a7	615bdd176227	Nikita Melnichenko	Removed outdated comments	Mar 11 2018, 8:45 AM
615bdd176227	990cc931176c	6632609e5493	Nikita Melnichenko	Added quick search bar to the bookmark menu (Show More…)	Mar 11 2018, 8:24 AM
6632609e5493	19af3d19cb1d	074851d8cd44	Nikita Melnichenko	Replaced ugly static variable with a proper design in bookmark menu builder	Mar 11 2018, 6:52 AM
074851d8cd44	7405f22a3229	21900f61f34a	Nikita Melnichenko	Fixed populating bookmark menu multiple times on startup (Show More…)	Mar 11 2018, 6:47 AM
21900f61f34a	31428e43d154	cb7711f8f6f8	Rade Deguerre	Add back quick search to the bookmark menu (Show More…)	Mar 8 2018, 4:14 AM

Prevent quicksearch from searching special bookmarks
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 30463

doc/bookmarks.docbook

doc/konfigurator.docbook

krusader/BookMan/krbookmark.h

krusader/BookMan/krbookmark.cpp

krusader/BookMan/krbookmarkbutton.cpp

krusader/BookMan/krbookmarkhandler.h

krusader/BookMan/krbookmarkhandler.cpp

krusader/Konfigurator/kgpanel.cpp

krusader/krusader.cpp

Prevent quicksearch from searching special bookmarksClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 30463

doc/bookmarks.docbook

doc/konfigurator.docbook

krusader/BookMan/krbookmark.h

krusader/BookMan/krbookmark.cpp

krusader/BookMan/krbookmarkbutton.cpp

krusader/BookMan/krbookmarkhandler.h

krusader/BookMan/krbookmarkhandler.cpp

krusader/Konfigurator/kgpanel.cpp

krusader/krusader.cpp

Prevent quicksearch from searching special bookmarks
ClosedPublic
Actions

Revision Contents
Changeset List