Enable preview by default in the filepicker dialog
ClosedPublic

Authored by anemeth on Apr 18 2018, 7:57 PM.

Details

Summary

Enable icon thumbnail preview in the Open/Save dialog by default.
This also enables it for users who previously disabled it.

Because of D12321: Hide file preview when icon is too small, it will only appear where it will actually be useful.

Depends on D12321

Test Plan
  • Open any filepicker dialog
  • Preview will be enabled

Diff Detail

Repository
R241 KIO
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth created this revision.Apr 18 2018, 7:57 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 18 2018, 7:57 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
anemeth requested review of this revision.Apr 18 2018, 7:57 PM
anemeth edited the summary of this revision. (Show Details)Apr 18 2018, 8:03 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: Frameworks, VDG.
anemeth added a subscriber: ngraham.
rkflx requested changes to this revision.Apr 18 2018, 11:42 PM
rkflx added a subscriber: rkflx.

This also enables it for users who previously disabled it.

Hm, normally changing a default is allowed, while overriding custom user settings is discouraged. However, for a bool it does not make a difference, really ;)

For KConfigXT settings, changing a default is easy, because defaults are not repeated in the config file, so it's enough to change the default in the code. Here, it's more complicated, as you had to change the name to get that effect.

However, the old key should not stay around in the config file, it should be removed. We have kconf_update for that. Could you provide or update such a script for ~/.config/kdeglobals?

This revision now requires changes to proceed.Apr 18 2018, 11:42 PM
abetts added a subscriber: abetts.Apr 19 2018, 12:08 AM

What would it look like if it was enabled by default? Screenshot?

This depends on D12321 and enables D12326; there are a lot of changes in flight here. But the overall goal here is explained in T8552: we want to make Short View display large icons with previews, and Detailed Tree View show a list with detail columns.

However, the old key should not stay around in the config file, it should be removed. We have kconf_update for that. Could you provide or update such a script for ~/.config/kdeglobals?

I couldn't get enough information out of that site, the kconf_update documentation and examples is awful.
However I tried doing something, could you check please if it's correct?

What would it look like if it was enabled by default? Screenshot?

rkflx added a comment.EditedApr 19 2018, 12:38 PM

However, the old key should not stay around in the config file, it should be removed. We have kconf_update for that. Could you provide or update such a script for ~/.config/kdeglobals?

I couldn't get enough information out of that site, the kconf_update documentation and examples is awful.

It's a wiki, you can improve it ;) I admit that at first it seems a bit daunting, but with a bit of patience in the end it's quite simple and neat, once you used this once or twice.

However I tried doing something, could you check please if it's correct?

Could not run this yet, but IIRC deleting a key does not need a separate script, you should be able to do it inline in the upd with RemoveKey.

Could not run this yet, but IIRC deleting a key does not need a separate script, you should be able to do it inline in the upd with RemoveKey.

how about this then:

rkflx added a comment.Apr 19 2018, 8:43 PM

Could not run this yet, but IIRC deleting a key does not need a separate script, you should be able to do it inline in the upd with RemoveKey.

how about this then:

Looks good on first sight, but I'll have to test it. Please add it to the patch as a new file, and also install it via CMake (see https://phabricator.kde.org/source/gwenview/browse/master/kconf_update/ for an example).

anemeth updated this revision to Diff 32611.Apr 19 2018, 9:10 PM
  • Add kconf_update files

Hmm, I'm not sure the kconf update script is working. I just tried out the patch in its current state, and got this:

Before:

dev@dev-pc:~/repos/kio$ grep Preview ~/.config/kdeglobals 
Preview Width=269
Previews=true
Show Preview=false
[PreviewSettings]

After:

dev@dev-pc:~/repos/kio$ grep Preview ~/.config/kdeglobals 
Preview Thumbnails=true
Preview Width=269
Previews=false
Show Preview=false
[PreviewSettings]

I manually copied the .upd file to /home/alex/.kde/share/apps/kconf_update and to /usr/share/apps/kconf_update but it doesn't seem like kconf_update is doing its thing.
What am I doing wrong?

Rebase on D12321

While the dependency tree is now looking good, the rebase did not work out correctly. While it might look correct on your local branch, on Phabricator you can clearly see that in this Diff the changes from the parent revision are visible, which should not be there. Please make sure to always check what ends up on Phabricator, as that's what your reviewers are seeing.

arc diff will upload the commit range between HEAD and the upstream tracking branch. Here the upstream branch apparently is master, while it should be your local branch where D12321 is living (you have a separate branch per Diff as written in the wiki, right?). You may want to look at my advice in D12321#249646 again.


I manually copied the .upd file to /home/alex/.kde/share/apps/kconf_update and to /usr/share/apps/kconf_update but it doesn't seem like kconf_update is doing its thing.
What am I doing wrong?

Did you read https://techbase.kde.org/Development/Tools/Using_kconf_update#Debugging_and_testing? Or maybe you need to manually call the kconf_update executable? Checking kded's output should tell you whether it ran.

rkflx requested changes to this revision.Apr 25 2018, 6:52 PM

Rebase on D12321

While the dependency tree is now looking good, the rebase did not work out correctly. While it might look correct on your local branch, on Phabricator you can clearly see that in this Diff the changes from the parent revision are visible, which should not be there. Please make sure to always check what ends up on Phabricator, as that's what your reviewers are seeing.

arc diff will upload the commit range between HEAD and the upstream tracking branch. Here the upstream branch apparently is master, while it should be your local branch where D12321 is living (you have a separate branch per Diff as written in the wiki, right?). You may want to look at my advice in D12321#249646 again.

@anemeth Ping. I won't be able to help you with kconf_update or review the patch unless Phab shows me the proper patch!

src/filewidgets/kdiroperator.cpp
2165

There's still some confusion in the wording wrt. Show Preview.

How about naming your setting Show Inline Previews (inspired by _k_toggleInlinePreview())?

(The other option could be renamed to Show Aside Preview in a followup patch.)

This revision now requires changes to proceed.Apr 25 2018, 6:52 PM
anemeth marked an inline comment as done.Apr 25 2018, 7:56 PM
rkflx added a comment.Apr 25 2018, 8:01 PM

Thanks for the update, but please make sure not to lose the kconf_update changes…

src/filewidgets/kdiroperator.cpp
2166

Ah, here it is. This really belongs in the other patch.

anemeth marked an inline comment as done.Apr 25 2018, 8:32 PM
anemeth marked an inline comment as not done.
anemeth updated this revision to Diff 33102.Apr 25 2018, 8:36 PM
  • Merge branch 'conditional_preview' into preview_default
  • Added kconf_update
anemeth updated this revision to Diff 33103.Apr 25 2018, 8:37 PM
  • Actually add kconf_update files

The .upd is being automatically copied to /usr/share/apps/kconf_update/
In the kconf_update log file /home/alex/.kde4/share/apps/kconf_update/log/update.log I found the following entry:

2018-04-25T21:41:56 Checking update-file '/usr/share/apps/kconf_update/filepicker-remove-previews-conf.upd' for new updates
2018-04-25T21:41:56 filepicker-remove-previews-conf.upd: Found new update 'kdeglobals-filepicker-remove-previews-conf'

The entry in the file /home/alex/.config/kdeglobals is still there.
Is there something wrong with the .upd file?

rkflx requested changes to this revision.Apr 26 2018, 7:06 PM

Thanks, arc patch now works great for me, automatically applying the dependent revision as it should.

In the kconf_update log file /home/alex/.kde4/share/apps/kconf_update/log/update.log I found the following entry:

.kde4 likely won't contain anything relevant for a KF5 KIO ;)

I could not find an equivalent file in ~/.config, but got some warnings here:

journalctl /usr/lib64/libexec/kf5/kconf_update --follow

Is there something wrong with the .upd file?

See inline comment on what I had to fix to make it work…

kconf_update/CMakeLists.txt
3

I'd name this file just filepicker.upd, as this way more updates could be added later on, and it would be more consistent with the naming of the other upd files.

kconf_update/filepicker-remove-previews-conf.upd
2

Most of the update files seem to have Version=5 in the first line, and a newline after that.

And indeed, this is exactly the reason why it did not work:

kconf_update[5695]: "Missing \"Version=5\", file 'share/kconf_update/filepicker-remove-previews-conf.upd' will be skipped."
This revision now requires changes to proceed.Apr 26 2018, 7:06 PM
anemeth updated this revision to Diff 33167.Apr 26 2018, 8:10 PM
  • Rename .upd file
  • Add Version=5 to .upd file
anemeth marked an inline comment as done.Apr 26 2018, 8:11 PM
rkflx accepted this revision.Apr 26 2018, 8:15 PM
rkflx added a subscriber: elvisangelaccio.

LGTM now (even though you ignored the "newline" I suggested…)

I'd appreciate a second opinion from Frameworks on the new kconf_update directory, though.

Also, the very idea of enabling previews by default in the file picker (as was done in Dolphin for 18.04) should get some more buy-in. I feel bad for tagging @elvisangelaccio again, but maybe you have suggestions for someone else who might help out in reviewing some of the patches for the file dialog?

This revision is now accepted and ready to land.Apr 26 2018, 8:15 PM
ngraham accepted this revision.Apr 26 2018, 8:25 PM

The idea behind D12321 was to increase buy-in, since one of the objections was that most previews are a useless regression at small icon sizes. Text files still won't be previewed by default at any size since we read from Dolphin's config file and they're off by default there.

I feel pretty confident that with these two polish points, previews in the open/save dialogs are a clear win. Of course that's just my opinion! :)

This revision was automatically updated to reflect the committed changes.