Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude directories containing file. #91

Closed
wants to merge 2 commits into from
Closed

Exclude directories containing file. #91

wants to merge 2 commits into from

Conversation

chrahunt
Copy link

This is not ready, but I wanted to check that this is an OK approach.

The use case mentioned in #90 requires only that a single static filename be configurable for excluding directories. Giving users the ability to set this single filename should be sufficient to provide the most benefit for the least increase in complexity. Users that want to check for multiple files can use a tool like find to locate files by pattern or using multiple filenames and create the necessary file checked by QDirStat. Further, we do not handle edge cases or try to emulate a particular backup program with respect to validating the file has any particular attributes or contents. A user may already check for edge cases like that using find/grep/diff.

Missing from this PR is the changed filename taking effect immediately when the settings are saved.

@shundhammer
Copy link
Owner

shundhammer commented Jan 26, 2019

Eh - no.

This is wrong on so many levels that I don't even know where to begin.

This has no place in the DirTreeModel. The DirTreeModel only maps the DirTree to the TreeView widget. The underlying data is stored in the DirTree. It gets there with DirReadJobs. Those DirReadJobs use ExcludeRules to exclude data to even be put into the DirTree.

Limiting that entire idea to only one static file name is another very bad idea. It does not integrate at all with anything in the entire program, and it imposes completely unnecessary limits.

This might happen to satisfy your specific requirements; if it solves a problem for you, of course you can hack up your own private QDirStat to do it like that. But something like that will never be merged into the main code line: It's either a clean, general approach, or it is not going to happen. And this here is anything but a clean, general approach.

@chrahunt
Copy link
Author

chrahunt commented Jan 26, 2019

Thanks for the feedback. If you have time can you help correct my understanding.

This has no place in the DirTreeModel. The DirTreeModel only maps the DirTree to the TreeView widget. The underlying data is stored in the DirTree.

My impression was that the DirTreeModel was responsible for creating and initially configuring a DirTree. I was emulating the approach that seemed to already be taken by the "CrossFileSystems" setting:

  1. The relevant configuration is propagated by DirTreeModel to DirTree
  2. DirTree keeps the current configuration for access by DirReadJobs
  3. DirReadJobs takes the configuration from DirTree and uses it for filtering during directory entry processing

Given a standalone setting like this (taking my assumption that it would not be integrated into ExcludeRule into account), what would be a better approach? Does this critique not also apply to CrossFileSystems?

Limiting that entire idea to only one static file name is another very bad idea.

To me the use case requiring matching multiple filenames in a directory lacks motivation. As mentioned earlier, any user that may want to exclude based on multiple files or use more generic patterns could use find before/during running QDirStat. I would hate to complicate QDirStat so much (extending ExcludeRule and complicating the UI and directory entry processing logic), when there is such a straightforward way to accomplish the same thing available to users.

Am I over-estimating the effort/performance cost to take the more general approach (extending ExcludeRule and holding directory entries until all are processed), or over-valuing having a workaround (find) available to make up for a more restricted solution? What makes a general solution desirable in this case if there does not seem to be a use case for it?

It does not integrate at all with anything in the entire program, and it imposes completely unnecessary limits.

As stated above, the implementation seemed in-line with the CrossFileSystems - satisfying a specific use case and following essentially the same pattern. What am I missing here?

This might happen to satisfy your specific requirements; if it solves a problem for you, of course you can hack up your own private QDirStat to do it like that.

Thanks. The project was very easy to build and get into, I appreciate the efforts you've taken to keep it that way.

@shundhammer
Copy link
Owner

shundhammer commented Jan 27, 2019

Looks like you will have to dig into the sources to get some better understanding which classes do what. Reading some header files will already give you some insights.

Check out:

  • DirTree (where the important data are stored)
  • FileInfo / DirInfo (the item classes)
  • DirReadJob (where directories are actually read)
  • ExcludeRule

That's the core classes. DirTreeModel is already GUI related; it is an adaptor class between the QAbstractItemModel class that the Qt item view classes need and QDirStat's core classes. The core classes can live just fine without those GUI classes.

The configuration classes "feed" other classes with config parameters, mostly via QSettings.

The CrossFileSystems parameter is something completely different because that's only one boolean config parameter, and it's really just one bit large. Exclude rules like what you have in mind might come in different shapes and sizes, and users/admins might choose to introduce several of them:

  • exclude all directories that have a file .qdirstatexclude
  • exclude all directories that have a file matching a shell glob .*exclude*
  • exclude all directories that have a file matching a regexp [._]?noba(k|ckup)

You get the idea.

Why would that be restricted to only one static string? Why not give users the choice between a static string, a shell glob, and a regexp - just like with the existing exclude rules?

@chrahunt chrahunt closed this Jan 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants