In addition, refactored and simplified bookmark search code,
added debug output and comments.
Details
Details
Apply on top of bookmark-search branch!
Extensively test bookmark search.
Diff Detail
Diff Detail
- Repository
- R167 Krusader
- Branch
- bookmark-search-bold-highlight_arc
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Search bar visibility tweaks are upcoming, just don't want to mix with the refactoring.
Comment Actions
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 | Should use curly braces too match the rest of the code. | |
| 594 | Is there a reason this is not just if (nMatches == 1)? | |
| 595 | 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 | |
| 601 | 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. | |
| 603 | If this is changed to "if (nMatches > 1)" then this comment isn't needed. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
Just for the record, it is on man page ;)
https://cgit.kde.org/krusader.git/tree/doc/man-krusader.1.docbook#n97
Comment Actions
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 panelComment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
In that case, accepting :-). I totally forgot subject of this patch, sorry about that.