In addition, refactored and simplified bookmark search code,
added debug output and comments.
Details
Apply on top of bookmark-search branch!
Extensively test bookmark search.
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.
Search bar visibility tweaks are upcoming, just don't want to mix with the refactoring.
This looks good. I would just recommend some minor changes.
Could you tell me how to enable the debug output? Running 'cmake -DCMAKE_BUILD_TYPE=Debug' seems not to be correct.
krusader/BookMan/krbookmarkhandler.cpp | ||
---|---|---|
551–557 | Should use curly braces too match the rest of the code. | |
595 | Is there a reason this is not just if (nMatches == 1)? | |
596 | I feel this comment should be removed. It's apparent from the code that this is what is happening. And sparse comments seem to fit how the rest of the file is commented | |
602–603 | This should be moved to ahead of line 596. Otherwise if a match opens a submenu, i.e "popular urls" or a folder, that submenu will be offset from it's parent. This occurs because it's opened then the quicksearchbar hides moving everything up one step. As opposed to hiding the quicksearchbar and _then_ opening the submenu. | |
604 | If this is changed to "if (nMatches > 1)" then this comment isn't needed. |
Thanks Rade for your review. I agree with all the points.
Could you tell me how to enable the debug output?
Use -d parameter when you run krusader.
Looks great!
I don't know if the -d flag is a kde standard flag (it doesn't exist in dolphin at least), but if it isn't then it should probably be added to the man page.
But that's a problem for another patch.
Just for the record, it is on man page ;)
https://cgit.kde.org/krusader.git/tree/doc/man-krusader.1.docbook#n97
Good to hear. However it's definitely not present on the man page that shipped with my distro. Dunno if it's a recent addition to the man page, or if my distro removed it for some reason.
--left <path> start left panel at <path> --right <path> start right panel at <path> --profile <panel-profile> Load <panel-profile> on startup --author show the authors --license show the license url if there is already a tab open with that url, it is activated, otherwise a new tab is opened in the active panel
Nice, thanks!
I've found one problem corner case with bookmark folders:
- say we have these bookmarks:
- work (bookmark folder)
- downloads (bookmark)
- documents (bookmark)
- open bookmarks (ctrl+d)
- type "w" to activate a bookmark folder "work"
- press esc to close the folder submenu
- press "d"
The "d" gets lost. Next "d" press will behave OK.
I can replicate this, but it's a bug unrelated to this patch. I have a patch ready to go that fixes that bug and several other bugs related to the handling of folders. It's however written on top of this patch. I'll try to submit it and see if there is any way to have phabricator understand that it's a derivative of this patch.
Thanks for reporting. Yes, this issue is known to me and it's independent of the change. There are several issues with the current bookmark search implementation, let's try to solve them one by one (BTW, feel free to participate if you have interest and time).
Note: The feature is in progress. All accepted reviews will be applied only to the bookmark-search 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.
In that case, accepting :-). I totally forgot subject of this patch, sorry about that.