Abandoned in favor of https://invent.kde.org/system/dolphin/-/merge_requests/203

Finally merge request opened at https://invent.kde.org/system/dolphin/-/merge_requests/203

Hi all!

In D29115#677038, @elvisangelaccio wrote:

In D29115#663669, @harogaston wrote:

In D29115#662313, @elvisangelaccio wrote:

You said it yourself: every setting needs a good amount of clutter code, which makes the overall codebase harder to test and harder to maintain. That's why we try to avoid adding a new setting every time we add a new feature.

We can reconsider if enough people complain about this new behavior not being optional.

So is that a let's go with it? I'm sorry I'm kinda lost as per what you want to do exactly (if anything at all), I don't have much of a say here. My opinion I already gave.

Sorry for the delay. Yes, please remove the option and make the new behavior the default.

You may also want to move this patch to gitlab: https://invent.kde.org

In D29115#663277, @ngraham wrote:

In D29115#662313, @elvisangelaccio wrote:

You said it yourself: every setting needs a good amount of clutter code, which makes the overall codebase harder to test and harder to maintain. That's why we try to avoid adding a new setting every time we add a new feature.

We can reconsider if enough people complain about this new behavior not being optional.

Like https://bugs.kde.org/show_bug.cgi?id=404955 :(

In D29115#662313, @elvisangelaccio wrote:

You said it yourself: every setting needs a good amount of clutter code, which makes the overall codebase harder to test and harder to maintain. That's why we try to avoid adding a new setting every time we add a new feature.

We can reconsider if enough people complain about this new behavior not being optional.

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.

• Changed spin-box special value text
• Update MaximumSize dynamically

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" ?

In D28402#660317, @ngraham wrote:

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.

In D28402#660317, @ngraham wrote:

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.

In D29115#658465, @meven wrote:

About testing, you need to install the tool ctest then go to build dir and run the whole tests with ctest (it has many options to be more clever for instance ctest -R kfileitemmodeltest

Changes after requests

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

In D28402#655312, @meven wrote:

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

Well for me it is pretty clear that this should be at least an option, if not the default. If you don't work with hidden files visible then this does nothing. But if you do, like myself, which view do you think is more useful/productive:

Ammend previous commit. Remove unrelated change

Fixed imports

Changes after review

Removed spinner label as per comments

In D28397#643937, @ngraham wrote:

Great patches, @harogaston! Keep 'em coming! :)

Removed unrelated changes

Sorry @ngraham I couldn't find who to assign as reviewer. I mean not anything that seemed appropriate. If you know who it would be great. Cheers!

Any feedback/help on this?

Removed unnesessary changes to .desktop as indicated

In D28397#637602, @pino wrote:

please do not change translated keys (eg Name[lang]) in desktop-alike files when you change the English string: we have a system in place to handle translations

In D28397#637193, @IlyaBizyaev wrote:

It seems that of released distros, only Fedora has vokoscreenNG in repositories so far

And at last I found what broke my icon theme 😆

@ngraham I could. I went scouting for examples of "the competition" trying to go borderless. Results vary, in form and execution. Some have rounder bottom corners and some don't, some have an outline and others have not. I went through the list of top window managers (taken from https://www.slant.co/topics/390/~best-window-managers-for-linux and https://wiki.archlinux.org/index.php/Window_manager) to find examples of borderless themes. Unsurprisingly I left out tiling window managers.

I am surprised to see how much (almost all) of the discussion here goes around personal opinions and no effort or intention is seen in bringing in objective facts, research papers, articles, examples, trends etc. I found it irresponsible to decide upon a product used by millions of people this way.