Provide ability to configure size cut-off for local file previews
ClosedPublic

Authored by harogaston on Mar 29 2020, 11:28 AM.

Details

Summary

FileWidgets read from kdeglobals the property "MaximumSize" under "PreviewSettings" to decide if a preview will be generated for that file.
There is no current GUI to change that file size limit. On the other hand Dolphin ignores it.

This patch aims to fix that by adding new configuration options to Dolphin. That is a new spinbox in Dolphin settings under General -> Previews tab.

Test Plan

1 - Set up a local folder with 2 jpg images of less and more than 1 MB respectively.
2 - Go to Dolphin Preferences. General -> Previews and check "JPEG Images" from

the list. And set "Skip previews for files above:" to 1MB.

3 - Navigate to the afore mentioned folder. Only the image of size less than 1 MB should

show a preview.

BUG: 331240

Diff Detail

Repository
R318 Dolphin
Branch
gui-setting-to-limit-local-file-preview (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26364
Build 26382: arc lint + arc unit
harogaston created this revision.Mar 29 2020, 11:28 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 29 2020, 11:28 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
harogaston requested review of this revision.Mar 29 2020, 11:28 AM
harogaston retitled this revision from Summary: Provide ability to configure size cut-off for local file previews to Provide ability to configure size cut-off for local file previews.Mar 29 2020, 11:41 AM
harogaston edited the summary of this revision. (Show Details)
harogaston added reviewers: ngraham, Dolphin.

Any feedback/help on this?

Sorry, it's on my to-do list but I've been quite busy lately. I'll try to make some time soon.

This comment was removed by ngraham.
ngraham added inline comments.
src/settings/general/previewssettingspage.cpp
73

I would put the whole text in this checkbox ("Skip previews for local files above:") rather than splitting it between a checkbox and a label

harogaston added inline comments.Apr 16 2020, 12:43 AM
src/settings/general/previewssettingspage.cpp
73

Yes, I guess it can be done. It is related to KIO code in the following way

src/widgets/previewjob.cpp:492

if (itemUrl.isLocalFile() || KProtocolInfo::protocolClass(itemUrl.scheme()) == QLatin1String(":local")) {
    skipCurrentItem = !d->ignoreMaximumSize && size > d->maximumLocalSize
                      && !d->currentItem.plugin->property(QStringLiteral("IgnoreMaximumSize")).toBool();
} else {
    // For remote items the "IgnoreMaximumSize" plugin property is not respected
    skipCurrentItem = !d->ignoreMaximumSize && size > d->maximumRemoteSize;

    // Remote directories are not supported, don't try to do a file_copy on them
    if (!skipCurrentItem) {
        // TODO update item.mimeType from the UDS entry, in case it wasn't set initially
        // But we don't use the mimetype anymore, we just use isDir().
        if (d->currentItem.item.isDir()) {
            skipCurrentItem = true;
        }
    }
}

Do you think it is correct to allow for the "Skip previews for local files above:" spinner to take value 0 to note that no limit should be imposed for previews? Or any other suggestions?

ngraham added inline comments.Apr 16 2020, 7:05 PM
src/settings/general/previewssettingspage.cpp
73

If you want for there to be no file size limit, you would just uncheck the checkbox, no?

harogaston added inline comments.Apr 16 2020, 11:53 PM
src/settings/general/previewssettingspage.cpp
73

Oh my bad, I though you were asking me to remove the checkbox and only have the spinner, I miss read you.
I will do that and leave the spinner without label.

harogaston edited the summary of this revision. (Show Details)

Removed spinner label as per comments

Thanks, much better.

I see what you mean about the setting only taking effect after a restart though. I'll have to defer to @meven or @elvisangelaccio on that.

src/settings/general/previewssettingspage.cpp
78

A minimum of zero doesn't really make sense; if a person wants to turn off all previews, there's already a button for that in the main toolbar. IMO a minimum of 1 (or even higher) would make more sense here.

cfeck added a subscriber: cfeck.Apr 17 2020, 5:15 PM

Many file formats don't require the complete file to be read to generate a preview. Do generators actually read these settings?

harogaston added inline comments.Apr 17 2020, 9:38 PM
src/settings/general/previewssettingspage.cpp
78

Ok, I'll make it 1 then.

harogaston updated this revision to Diff 80429.EditedApr 17 2020, 9:53 PM

Changes after review

  • Renamed constants to more clearly indicate their usage
  • Set default value for local file size preview to 50 MB (this does not take action unless the user intentionally enables 'Skip preview for files above: ' so it is arbitrary as much as innocuous
  • Set minimum value for local file size preview limit spinner to 1 (MB)
harogaston marked 6 inline comments as done.Apr 17 2020, 9:54 PM
ngraham accepted this revision.Apr 19 2020, 7:10 PM

I see that previews being disabled when these settings are changes also requires a refresh for remote files, so this isn't a new problem you've introduced or a deficiency with your patch; rather, it's a missing feature in kfileitemmodelrolesupdater. Fixing that for both local and remove previews might be a good follow-up patch. ;-)

Just one little nitpick left to resolve from me:

src/settings/general/previewssettingspage.h
23

Just forward-declare this, as with the below class [thing] lines. Also, no need to include the QtWidgets/ prefix.

This revision is now accepted and ready to land.Apr 19 2020, 7:10 PM
harogaston updated this revision to Diff 80948.Apr 23 2020, 3:23 AM

Fixed imports

harogaston marked an inline comment as done.Apr 23 2020, 3:23 AM
meven added a comment.Apr 23 2020, 6:58 AM

Should we make the information panel follow those settings as well (currently the preview there has no limit, nor plugin limitations) ?
Same question about tooltips.

I think we should. Essentially copying to there code :

const KConfigGroup globalConfig(KSharedConfig::openConfig(), "PreviewSettings");
m_enabledPlugins = globalConfig.readEntry("Plugins", KIO::PreviewJob::defaultPlugins());
m_localFileSizeLimit = globalConfig.readEntry("LimitLocalFiles", false);

Should we make the information panel follow those settings as well (currently the preview there has no limit, nor plugin limitations) ?
Same question about tooltips.

I think we should. Essentially copying to there code :

const KConfigGroup globalConfig(KSharedConfig::openConfig(), "PreviewSettings");
 m_enabledPlugins = globalConfig.readEntry("Plugins", KIO::PreviewJob::defaultPlugins());
 m_localFileSizeLimit = globalConfig.readEntry("LimitLocalFiles", false);

I think I agree with you even though I didn't get your point regarding the code snippet.
In any case I guess that idea should be part of a new patch, do you agree?

meven accepted this revision.Apr 24 2020, 6:04 AM

Should we make the information panel follow those settings as well (currently the preview there has no limit, nor plugin limitations) ?
Same question about tooltips.

I think we should. Essentially copying to there code :

const KConfigGroup globalConfig(KSharedConfig::openConfig(), "PreviewSettings");
 m_enabledPlugins = globalConfig.readEntry("Plugins", KIO::PreviewJob::defaultPlugins());
 m_localFileSizeLimit = globalConfig.readEntry("LimitLocalFiles", false);

I think I agree with you even though I didn't get your point regarding the code snippet.

It's the code the toolptipmanager and the information panel need to support limiting its previews as the rest.

In any case I guess that idea should be part of a new patch, do you agree?

As you want, it can be in another patch.

As I am a noob it currently has a flaw.
It is that it doesn't pick up all the configuration changes 'on the fly' but only after restarting Dolphin.
If the check "Local files" is checked then the file size limit affects immediately.

But not a new size limit...

But changes to the checkbox itself only take effect after restarting dolphing. I know why it happens but I > couldn't code a solution. Maybe Nate can give me a hand or point to someone that can.

You need to move m_localFileSizeLimit = globalConfig.readEntry("LimitLocalFiles", false); in KFileItemModelRolesUpdater::startPreviewJob to keep the m_localFileSizeLimit value fresh.
The other two values are read from the Job anyway.

Please remove those lines from the commit comment before merging (you can edit here and use arc amend)
You can move it to a comment.

elvisangelaccio requested changes to this revision.Apr 26 2020, 9:41 PM

Please add a #include <QCheckBox> in previewssettingspage.cpp, otherwise it doesn't build with Qt 5.15.

Is there a reason why we are adding a checkbox only for local files? If yes, please write it in the commit message. And while at it, please remove the "I am a noob" part ;)

src/settings/general/previewssettingspage.h
59

This shuld be in the private slots section. While at it, please call it toggleLocalFileSize() or similar, since this is a slot and not a signal.

This revision now requires changes to proceed.Apr 26 2020, 9:41 PM

@elvisangelaccio @meven I will address your comments next week (I couldn't find time yet).
Thank you for your feedback.

harogaston edited the summary of this revision. (Show Details)Apr 29 2020, 10:20 PM

Changes after requests

  • Removed checkbox and lowered spinbox limit down to 0. A value of 0 disables previews for all local files. This mimics the behaviour of the preexisting spinbox for remote files.
  • By removing the checkbox all changes now affect immediately and logic is simplified.
harogaston edited the summary of this revision. (Show Details)Apr 30 2020, 12:39 AM
harogaston edited the test plan for this revision. (Show Details)
harogaston marked an inline comment as done and an inline comment as not done.Apr 30 2020, 12:45 AM
harogaston added inline comments.
src/settings/general/previewssettingspage.cpp
78

@ngraham I don't know how you would feel about the new changes. I removed the checkbox in order to simplify things and allowed the spinbox to take value 0 (for which no previews will be shown at all).
Previously you said that doing that (setting a value of 0) would be equivalent to unselecting "Show Previews" from the main toolbar. I though you were right but in fact it is different. The button on the main toolbar will affect both local and remote files, while each spinbox now affects each file type separately.

Let me know if you have any other thoughts on this.

PS: Also the spinbox for Remote Files Size Limit already takes 0 (in fact that is the default value) so this also brings consistency in my opinion and makes the settings simpler.

If you remove the checkbox, then a good UI for letting a zero value turn off the feature would be to replace the text in the spinbox with "No maximum size" or something when it would otherwise display zero. Spectacle does this with its timer feature.

If you remove the checkbox, then a good UI for letting a zero value turn off the feature would be to replace the text in the spinbox with "No maximum size" or something when it would otherwise display zero. Spectacle does this with its timer feature.

Good, I will look into it. In this case it does not disable the feature, instead it means no previews will be rendered regardless for any file size (the upper limit is 0).

If you remove the checkbox, then a good UI for letting a zero value turn off the feature would be to replace the text in the spinbox with "No maximum size" or something when it would otherwise display zero. Spectacle does this with its timer feature.

@ngraham if you are ok will this I will submit the change. I tried other wordings, this is most suitable one I could come up with. I'm open to suggestions of course.

meven added a comment.May 1 2020, 5:19 PM

If you remove the checkbox, then a good UI for letting a zero value turn off the feature would be to replace the text in the spinbox with "No maximum size" or something when it would otherwise display zero. Spectacle does this with its timer feature.

@ngraham if you are ok will this I will submit the change. I tried other wordings, this is most suitable one I could come up with. I'm open to suggestions of course.

Please push your changes.
Another text would be "Preview Disabled", "Never Preview" but "No Preview" for remote files seems better to me.
And for local files "No Size limit" ?

Please push your changes.
Another text would be "Preview Disabled", "Never Preview" but "No Preview" for remote files seems better to me.
And for local files "No Size limit" ?

Hi! I'm ok with whichever text we decide to go with but I don't understand why you suggested using different ones for each spin-box.
Functionality is exactly the same for both files categories.

Maybe I failed to understand what you were saying.

Please let me know.
Cheers!

meven added a comment.May 2 2020, 6:23 AM

Please push your changes.
Another text would be "Preview Disabled", "Never Preview" but "No Preview" for remote files seems better to me.
And for local files "No Size limit" ?

Hi! I'm ok with whichever text we decide to go with but I don't understand why you suggested using different ones for each spin-box.
Functionality is exactly the same for both files categories.

In fact they are not :
When 0 is set for remote file spinbox it means that only remote files a file smaller than 0 will get a preview (i.e never).
When 0 is set for local file spinbox it means that all local files file will get a preview because in that case we set IgnoreMaximumSize :

job->setIgnoreMaximumSize(itemSubSet.first().isLocalFile() && m_localFileSizePreviewLimit <= 0

This inverse the meaning of the value 0 basically. (The KPreviewJob use the max int value as default for MaximumSize, meaning all local files should have a preview).
If this line was not set, it would be true.

harogaston updated this revision to Diff 81965.May 5 2020, 1:16 AM
  • Changed spin-box special value text
  • Update MaximumSize dynamically

In fact they are not :
When 0 is set for remote file spinbox it means that only remote files a file smaller than 0 will get a preview (i.e never).
When 0 is set for local file spinbox it means that all local files file will get a preview because in that case we set IgnoreMaximumSize :

job->setIgnoreMaximumSize(itemSubSet.first().isLocalFile() && m_localFileSizePreviewLimit <= 0

This inverse the meaning of the value 0 basically. (The KPreviewJob use the max int value as default for MaximumSize, meaning all local files should have a preview).
If this line was not set, it would be true.

You were right, of course. Which got me thinking why did I have the wrong idea. It turned out I was not updating the value properly so the behaviur I my testing was misleading.
I went for "No limit" and "No previews" as the spin-box special value (0) text for local and remote files respectively.

Any other comments please let me know.

elvisangelaccio accepted this revision.Sun, Jun 7, 10:59 PM

@ngraham Does the UI look good? Can we ship this?

This revision is now accepted and ready to land.Sun, Jun 7, 10:59 PM

Yep, looks great!

ngraham closed this revision.Tue, Jun 9, 5:06 PM