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?
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.
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).
Hmm, I'm not sure the kconf update script is working. I just tried out the patch in its current state, and got this:
dev@dev-pc:~/repos/kio$ grep Preview ~/.config/kdeglobals Preview Width=269 Previews=true Show Preview=false [PreviewSettings]
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?
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.
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.
@anemeth Ping. I won't be able to help you with kconf_update or review the patch unless Phab shows me the proper patch!
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.)
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?
Thanks, arc patch now works great for me, automatically applying the dependent revision as it should.
.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…
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.
|1 ↗||(On Diff #33103)|
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: "Missing \"Version=5\", file 'share/kconf_update/filepicker-remove-previews-conf.upd' will be skipped."
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?
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! :)