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

Disable selection.mimetype (and others) if too many files are selected. #95

Open
end2endzone opened this issue Sep 22, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@end2endzone
Copy link
Owner

Describe the bug
The properties selection.mimetype, selection.descrption and selection.charset should be disabled if too many files are selected. In a use case where a user right-click on his vacations picture collection with 10000+ files, File Explorer will appear Not responding.

Based on the code in Context::RegisterProperties(), ShellAnything will extract the mimetype, description and character set of all 10000+ files which will certainly take a ridiculous (and probably unnecessary) amount of time.

This is because, by design, all properties are updated even if they are not required/used in a Configuration.

This bug was introduced when implementing #92 and #94 .

To Reproduce
Steps to reproduce the behavior:

  1. Select multiple files (like 10000+) in File Explorer
  2. Right-Click all files. File Explorer hangs for along period of time.

Expected behavior
When more than X files are selected, it usually means that user is not interested in filtering by mimetype, description or character set. The appropriate value for X should be discussed below.

Environment

  • OS: N/A
  • Version 0.7.0

Additional context
Add any other context about the problem here.

@end2endzone end2endzone added the bug Something isn't working label Sep 22, 2021
@end2endzone
Copy link
Owner Author

I am think about 10, but opened to other suggestion.

@Cirn09
Copy link
Contributor

Cirn09 commented Oct 6, 2021

The user should be given options and let the user decide.

Before that, I also considered the issue of too many files are selected.

I have a few thoughts: One is to merge multiple selections into one when the attributes are the same, but this scheme is not practical, also not easy to implement.
Another option is a filter. For example, I selected several files of *.exe and other format files, and I set to filter "exe", then selection.filename should only have *.exe after filtering. In order to achieve this function, a has() macro should also be implemented. But then I thought that it would be more appropriate for the filter to be implemented by a third-party program. The has macro seems to be useful, but it is little unnecessary.

In addition, although libmagic's IO overhead is small, it is not non-existent, especially for some low-speed storage media. Perhaps it is better to let the user decide whether to enable this feature, or it is better to implement this feature as a plug-in (in my imagination, plug-ins are loaded on demand)

@end2endzone
Copy link
Owner Author

First I would like to say thank you for trying to help. It is much appreciated to get feedback and new ideas from other people that uses this application.

I have looked at your proposed option. I do not understand the merge option. More specifically because I do not understand what you mean by "attributes". Are you referring to file extension? Like once the first ".jpg" file has gone through libmagic and we get the mime type, we should use the same mime type for all other .jpg files?

I am also afraid that your second option is currently impossible to implement with the current code base. The current implementation is refreshing properties (including ${selection.mimetype}) before visibility/validity filter are verified. It must be done in this order since the validation process often involves properties. This is a problem similar to "who comes first the egg or the chicken". If I understand correctly, what you are proposing is, at property refresh time, to "peek" into all menu's visibility/validity nodes and if all of them are filtering for "exe only", then we can eliminate evaluating the property for .png and .jpg files. I do not think this is the way to go.

Perhaps it is better to let the user decide whether to enable this feature, or [not].

I agree that user should have control and be able to decide. However, my point is the application should have limitations in place "by default" to prevent performance issue. This is to prevent situations where the popup menu takes too long to show. In other words, a user that is unaware of this feature/functionality would not experience slowdown because he does not know. On the opposite, a user that actually needs to evaluate many files will be able to control the app and for example, increase the "default" limit size.

I am not a fan of relying on plugins in an application for every aspect of a feature. It does provide maximum flexibility but it certainly increase the complexity and limits this ability to highly technical users.

I see 2 ways of limiting this risk:

  1. We can limit how many files this feature should evaluate before it stops peeking into files.
  2. We can limit how long this feature can take before it stops peeking into files.

I think the easiest way would be to define properties for such limitations. Properties can be modified in Configuration Files easily. They can be documented in the User Manual so that advanced users can modify their values.
For example we could implement the following new properties: selection.mimetype.maxfiles and selection.mimetype.timeout.

From my experience, a 2 seconds delay before the menu shows up is way too long. I like my system to as responsive as possible.
Also, my previous proposition of a maximum of 10 files is a bit ridiculous.

I think default values for each property should be something similar to these:

  1. selection.mimetype.maxfiles = 2000
  2. selection.mimetype.timeout = 5000 ms

Does that look like good alternative to you ? Do you have another better idea ? How to do you feel about those values ?
Again, looking for your feedback.

@end2endzone
Copy link
Owner Author

Any update on the proposed new variables to limit the "application is not responding" use cases ?
What do you think of

  1. selection.mimetype.maxfiles = 2000
  2. selection.mimetype.timeout = 5000 ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants