Ability to ignore defined files and directories from search results
ClosedPublic

Authored by martinkostolny on Nov 28 2017, 10:02 PM.

Details

Summary

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.

Test Plan

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
martinletacek requested review of this revision.Nov 28 2017, 10:02 PM
martinletacek created this revision.
martinletacek added a reviewer: Krusader.
martinletacek edited the summary of this revision. (Show Details)Nov 28 2017, 10:06 PM
martinkostolny added a subscriber: martinkostolny.

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);
This revision is now accepted and ready to land.Nov 28 2017, 11:25 PM
martinkostolny requested changes to this revision.Nov 29 2017, 1:09 PM

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.

This revision now requires changes to proceed.Nov 29 2017, 1:09 PM
martinletacek marked 2 inline comments as done.Nov 29 2017, 7:45 PM

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?

abika added a subscriber: abika.Nov 29 2017, 8:35 PM

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

"colon-separated", semicolon is ";"

abika requested changes to this revision.Nov 29 2017, 8:51 PM

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...

martinletacek marked 4 inline comments as done.

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 :).

martinletacek updated this revision to Diff 24441.EditedDec 28 2017, 8:55 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 23 2018, 7:27 PM
Closed by commit R167:4f6949242455: Ability to ignore defined files and directories from search results (authored by Alexander Bikadorov <alex.bikadorov@kdemail.net>). · Explain Why
This revision was automatically updated to reflect the committed changes.
abika reopened this revision.Jan 23 2018, 7:32 PM

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.

martinkostolny commandeered this revision.Feb 19 2018, 12:04 AM
martinkostolny edited reviewers, added: martinletacek; removed: martinkostolny.

I'll update the diff here so the flow here can be properly finished.

martinkostolny updated this revision to Diff 27506.EditedFeb 19 2018, 12:06 AM

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 :).

martinkostolny edited the test plan for this revision. (Show Details)Feb 19 2018, 12:09 AM
martinkostolny added a project: Krusader.
asensi accepted this revision.Feb 21 2018, 5:40 PM
asensi added a subscriber: asensi.

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!

martinkostolny updated this revision to Diff 27887.EditedFeb 24 2018, 12:29 AM

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

One more small update - fix include order.

asensi accepted this revision.Feb 24 2018, 10:57 AM

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 :-)

nmel accepted this revision.Feb 24 2018, 11:10 PM
nmel added a subscriber: nmel.

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.

martinkostolny marked 3 inline comments as done.Feb 25 2018, 6:01 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 25 2018, 6:07 PM
This revision was automatically updated to reflect the committed changes.