Hi,
This modification allows the user to setup a custom list of filenames/directory names that are to be ignored during searching files.
The path settings is in Settings > Setup Krusader > Advanced > Fine-Tuning > Ignored Search Paths.
The search dialog also allows to enable/disable this feature if not required - 'Use Ignored Paths' (enabled by default).
The standard 'do not search in' doesn't seem to remember the configuration and also doesn't work for subdirectory names, i want to ignore some directories, eg. .svn, logs and so on.
I'm new here, but i really love krusader and want to improve it so please feel free to suggest any modifications to the diff.
Details
- Reviewers
abika martinletacek asensi nmel - Group Reviewers
Krusader - Commits
- R167:29adfb1ddf30: Merge branch 'ignore-folders-search'
R167:4f6949242455: Ability to ignore defined files and directories from search results
R167:71df62e55af8: Changne excludeFolderNames splitter from colon to space.
R167:dd6a8b1e7b1c: Minor code cleanups
R167:e31f1db94a30: fix include order
R167:fce60f65485e: Ability to ignore defined files and directories from search results
Open search dialog and search with and without 'Exclude Folder Names' option. Ideally set multiple folder names as excluded.
Diff Detail
- Repository
- R167 Krusader
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Hi! Thanks for your interest and code! I like it. Sometimes I also don't want to search in .git directories and this change will solve this, so this is great!
I'm no C++/Qt expert but I think there can be minor simplifications in your code - please see my code comments.
krusader/FileSystem/krquery.cpp | ||
---|---|---|
750–757 | Just a suggestion for simplification: const QString basename = url.fileName(); for (const QString &ignorePath : whereIgnorePath) if (basename == ignorePath) return true; if (!matchDirName(basename)) | |
785–788 | Another suggestion: whereIgnorePath.clear(); whereIgnorePath.append(paths); |
Sorry, I've found a bug. During today Krusader use with your patch, there is reproducible crashing when using Synchronizer. When clicking "Compare" in Synchronizer dialog Krusader crashes. I'll have time to more closely look into that tomorrow.
Hi,
Yep these seems good for me, i'm more a PHP developer :).
I have just checked the Synchronizer and it seem to be working for me, let me know if i can help...
Also not sure if the search dialog wouldn't get too much crowded, should i hide the checkbox when there's nothing set in the settings?
Thanks for your contribution! Excluding by folder names is a good idea and very useful.
However, having the string definitions in the configuration settings doesn't feel "right" for me. There is now a checkbox in the search dialog, but the actual folders can only be set in another dialog far away.
Why not combining both, e.g. with a KHistoryComboBox (the main "Search for: " edit line is an example) in the search dialog? This way you have only one GUI element, you can quickly edit it, or clear it, and you have even a history.
This is my suggestion besides the minor code improvements. What do you think?
krusader/FileSystem/krquery.cpp | ||
---|---|---|
750–757 | +1 | |
785–788 | +1 (or << or +=) | |
krusader/FileSystem/krquery.h | ||
135 | QStringList here and everywhere else | |
137 | better name it ignoredPaths() here and everywhere else | |
193 | ignoredPaths | |
krusader/Konfigurator/kgadvanced.cpp | ||
126 ↗ | (On Diff #23104) | "colon-separated", semicolon is ";" |
And I can confirm the crash in Synchronizer. The checkbox useIgnoredPaths is not initialized here. Should be set to nullptr and checked for it before access. (Crash is in generalFilter.cpp line 547).
Another naming suggestion: "Exclude folder names" globally for the feature.
Hi abilka.
RE: Naming suggestions: the Exclude folder names looks like a better idea, will change the code in this way in the evening.
RE: Search dialog input box: Currently from my tests the search dialog doesn't remember much (over app restart), it might be good idea to have a history for it, but i would much more like that i don't have to remember to change that field at all.
Also as i was saying i don't want to make the search dialog bigger and even thinking about hiding this checkbox when it doesn't have any settings. on the other hand people won't know about this feature if the checkbox would be hidden. Not sure what would be better here...
Hi,
Sorry it took so long, but hopefully i added all the changes to the patch now.
The feature is now called Exclude Folder Names, changed throughout the code,
The crash in Synchronizer should be fixed (i have moved the inspecting of the checkbox into the correct condition in the getSettings function).
Still the same behavior: checkbox is always in the search dialog and the directories are in the configuration.
Let me know what you think
Thanks for the update, Martin! And sorry it took me so long to dive in. I like it but I'd actually prefer the idea of Alex - keep all the settings in search dialog. I have modified your diff and made 2 screenshots.
I have moved the new checkbox right under "Do not search in" list-box and added KHistoryComboBox next to it. It becomes disabled when checkbox is unchecked. This is of course just an idea. I still agree both checkbox and the new KHistoryComboBox should be persistent. I'm attaching the changed diff, but it is not a final one, it just contains the visual changes. I'm willing to finish it if You wouldn't want to :).
Hi,
So i have updated the diff using your suggestion.
It is now in the search dialog only but as it doesn't remember the text itself when app is restarted (it is only in the history) the checkbox is disabled by default.
If you can think of any standard way to remember this over restart i would like to have it there, but this may be fine anyway (will test if i will remember to turn this on)...
Let me know :)
Thanks for the update! I've managed to make the input persistent. Please see attached diff (modification of yours) if it works :).
Hello! Have you, Martin, or anybody else tried the last diff? Is this the right approach to the problem? I've been using it since and technically there were no crashes with my use-cases.
I pushed a new branch arcpatch-D9041 with Diff 24441 and the patch file. Merging was not that trivial.
And I already made some fixes but its not done yet (and uncommited).
@martinletacek If you give me your email address, I can change the author information of the commit.
I will merge into master when everything is done.
Hi, Alex. Thanks for giving it a go!
The attached diff of mine (exclude-folder-names-02.diff) was meant as a diff against master. It was a modification of what @martinletacek did and it was meant as final. I simplified / removed some changes of Martin which were IMO unnecessary; that is why my diff was a bit lighter. Therefore I don't understand why You merged the diffs. But I must be missing something...
Hi,
Sorry i was not able to do much with the change here lately :-(.
I have checked out the @martinkostolny change, it works as i would expect, thanks.
I was originaly a little suprised with the amount of changes i have done (i was mostly copying some parts as they were done for another feature on the search screen) so it's good it's simplified like this.
If I understand correctly, the last diff (exclude-folder-names-02.diff) can be committed and pushed.
@abika Alex, is it also OK for you? Or have you found any issues with it?
@martinletacek Martin, do you have write access? I think you should push it since it was your idea and you wrote most of the code.
Updating diff. This is the one attached previously as exclude-folder-names-02.diff.
If this get accepted, I'm going to push the change with @martinletacek as the original author. Martin, if you have write access, it would be best for you to do that. Anyway, thanks a lot for your idea and realization! I use this feature daily :).
Thank you! For my part, I accept this because I performed some tests and the new patch worked correctly (items inside the specified folders are excluded, files and folders that are shown are the expected ones, etc.), although when there was a folder with a ":" inside its name then the folder couldn't be excluded.
This last minor problem may be less important than leaving people nowadays without the advantages of the new patch.
For more consistency, I'm thinking that, if in the "Search for:" text field, Krusader users space as a delimiter... maybe it would also use it in the same window, in the proposed «Exclude Folder Names» text field.
Users also see and manage more clearly
.git .svn logs .Trash trash
than
.git:.svn:logs:.Trash:trash
If, in the future, a "RegExp" button is implanted next to the «Exclude Folder Names» text field: then escaping a space with "\ " will be easier to see for Bash users than "\:" (because Bash users have normally seen a lot of "\ " and no "\:").
To use space as a delimiter, in the new "generalfilter.cpp" file it is only needed to change
split(':');
into
split(' ');
Thanks for all!
Thanks, Toni, for testing the patch and the pretty good idea to change colon to space!
I've changed delimiter to space and used KShell::splitArgs() method to allow using
- quoted folder names
- folder names with escaped spaces
To be more clear - we can now set this:
.git "folder with spaces" more\ spaces colon:folder
...to exclude folder names:
.git folder with spaces more spaces colon:folder
I've changed delimiter to space and used KShell::splitArgs() method to allow using
- quoted folder names
- folder names with escaped spaces
Excellent!
I did more tryings, and they were successful :-)
I also tested and it seems to work fine. Thanks for the work to both Martins!
Please consider including the inline suggestions that I have.
Personally, if I were designing the feature from the start, I'd consider expanding "Do not search in" meaning by allowing plain names in this list. Currently only full paths seem to be supported. On the other hand, the new "Exclude Folder Names" has a history now but "Do not search in" doesn't have it, and it's a nice bonus. Just my 2 cents.
krusader/FileSystem/krquery.h | ||
---|---|---|
137 | Current name is better because it's really about folder names, not whole paths. | |
krusader/Filter/generalfilter.cpp | ||
435 | minor: two ifs could be combined in if ((properties & FilterTabs::HasDontSearchIn) && (properties & FilterTabs::HasRecurseOptions)) | |
463 | minor: two ifs could be combined in if ((properties & FilterTabs::HasDontSearchIn) && (properties & FilterTabs::HasRecurseOptions)) | |
605 | space: if ( |
Thanks, Nikita, for your remarks! I have worked them in and I will push the changes right away.
This GUI configuration was not a first iteration but you are right. Best would be somehow integrate it in "Do not search in". Ideally along with persistent option, allow enable / disable and provide a
history dropdown. This was a compromise, but I'm of course open to any further proposals to draft the GUI better.