Parse global config files. Remove 'Vendor default' option. Fix changes not recognized.
ClosedPublic

Authored by progwolff on May 16 2018, 9:16 AM.

Details

Reviewers
mart
ngraham
Group Reviewers
Plasma
Summary

The fonts kcm did only parse a local config file. Global and default settings were not recognized.
With this patch, all global config files are parsed before the local file is parsed.
This allows us to drop the "Vendor default" option.

This patch also fixes some changes not recognized (apply button disabled).

BUG: 386566

Depends on D14480

Test Plan

Delete ~/.config/fontconfig/fonts.conf and ~/.config/kcmfonts .
Run kcmshell5 fonts. The default/global settings should be displayed.
Change some settings. The apply button should be enabled.
Save. A new config file should be created.

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D12925
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1362
Build 1380: arc lint + arc unit
progwolff created this revision.May 16 2018, 9:16 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 16 2018, 9:16 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
progwolff requested review of this revision.May 16 2018, 9:16 AM
progwolff updated this revision to Diff 34274.May 16 2018, 9:25 AM
  • Fix 'disabled from'
mart added a subscriber: mart.May 16 2018, 10:05 AM

if it removes the vendor default option, how can i ask to not touch the global/local defaults in any way?

progwolff added a comment.EditedMay 16 2018, 11:29 AM
In D12925#263459, @mart wrote:

if it removes the vendor default option, how can i ask to not touch the global/local defaults in any way?

Just don't change anything in the kcm.
The global config files are never touched and the kcm controls are set to the global config by default.

See also: https://phabricator.kde.org/D12849#262782

+1 conceptually; "Vendor Default" was always really awkward to explain to people.

ngraham edited the summary of this revision. (Show Details)May 16 2018, 5:34 PM
progwolff updated this revision to Diff 34315.May 16 2018, 6:42 PM
  • remove unintentionally committed debug lines
rkflx resigned from this revision.May 16 2018, 9:34 PM

Sorry for resigning as a reviewer, but currently I lack the time to review this in depth. Maybe I can do some basic testing tomorrow, but no promises.

As said before though, conceptually this makes sense to me. IIUC, this is a GUI for the fontconfig setting, and no GUI to bypass the fontconfig setting.

rkflx added a subscriber: rkflx.May 17 2018, 10:19 AM

You might want to rebase on the latest changes to D12849 (currently the patch only applies against arc pat --diff 34209, i.e. a previous version of D12849), and here set D12849 as a dependent Diff. See also https://community.kde.org/Infrastructure/Phabricator#Marking_patches_as_dependent_on_other_patches.

Hm, repeatedly pressing the Defaults button works really weird for me, and in some cases clicking on Disabled from… does not activate the option and only enables the widgets below (I need a second click for actually switching the radio selection).

I'll resume testing once this works a bit better. I'd say let's not rush this, and get it right for 5.14.

I'll resume testing once this works a bit better. I'd say let's not rush this, and get it right for 5.14.

I agree. Thanks for investigating!

progwolff planned changes to this revision.May 17 2018, 11:53 AM

Any update on this patch?

progwolff updated this revision to Diff 38757.Jul 30 2018, 9:02 AM

Rebase on master. Partial rewrite.

progwolff added a comment.EditedJul 30 2018, 9:20 AM

Big sorry for being away for so long. I had some things to do that took much more time than initally planned.
Many thanks to those who fixed the bugs I introduced or created workarounds...

I tried to get this patch working now. First of all, I want to say a few words about how this patch will remove the "vendor default" option.
Previously "vendor default" ment that no anti-aliasing entry is written to the local config file. There could however be a global config file that has such an entry.
As it was already said in some older comments, this is nothing special. There are many kcms and other applications where we can have both local and global config files. So, we could drop the "vendor default" option here, if we are able to parse the global configs.

This patch tries to do this by using all config files given by FcConfigGetConfigFiles.
Ater parsing all the config files we know if there is a local anti-aliasing-entry. If there is, any changes will be written to this entry in the local config file.
If there is not a local anti-aliasing-entry, none will be created unless the option is actually changed in the kcm. If it is changed, a new entry is created in the local config file.
If the user resets the defaults, the local entry is removed.

I tested this on my machine without any problems. We should however test this carefully on different systems. There might be some cases I did not think about yet.

progwolff updated this revision to Diff 38764.Jul 30 2018, 11:31 AM
  • split changes to previewimageprovider.cpp to a separate commit
progwolff edited the summary of this revision. (Show Details)Jul 30 2018, 11:42 AM
progwolff edited the test plan for this revision. (Show Details)
progwolff removed a reviewer: rkflx.
ngraham requested changes to this revision.Jul 30 2018, 1:17 PM
ngraham added a reviewer: Plasma.

Nice! Two nitpicks:

  • Anti-aliasing: [x] Use antialiasing Either change the checkbox's label to "Enable", or remove the left label and give this whole section its own header.
  • We've still got a "Vendor Default" option for "Sub-pixel rendering type". Instead, there should really be a "none" option there to complement the options for the different types.
This revision now requires changes to proceed.Jul 30 2018, 1:17 PM
progwolff updated this revision to Diff 38828.Jul 31 2018, 8:54 AM
  • Merge branch 'master' of git://anongit.kde.org/plasma-desktop into arcpatch-D12925
  • Merge branch 'master' of git://anongit.kde.org/plasma-desktop into arcpatch-D12925
  • also drop vendor-default-option for subpixel and hint

I am not sure about the case when neither global nor local configs exist for subpixel or hint.
On my system it seems like rgb subpixel rendering with slight hinting is used when no configs exist. But I cannot find any documentation on this.

We also need to make sure that we don't reintroduce the bug fixed in https://phabricator.kde.org/R119:f02df03cb87b4bb5724eec668d49126a5f52a1e7
I think we can safely remove the vendor default option that was introduced to fix this bug, if we make sure to never save unchanged configurations.

progwolff updated this revision to Diff 38829.Jul 31 2018, 9:01 AM
  • fix previews

I am not sure about the case when neither global nor local configs exist for subpixel or hint.
On my system it seems like rgb subpixel rendering with slight hinting is used when no configs exist. But I cannot find any documentation on this.

Yep me too. This makes sense since it's a sensible default. It looks like the old Vendor Default setting was actually introducing a bug and overriding the actual "vendor default"! As such, this patch fixes https://bugs.kde.org/show_bug.cgi?id=389598

ngraham requested changes to this revision.Jul 31 2018, 2:31 PM

On the other hand, no matter what combination of sub-pixel antialiasing and hinting style I suggest, and regardless of what default value is shown in the UI, the result seems to be the functional equivalent of the None values. i.e. now sub-pixel anti-aliasing doesn't work at all.

This revision now requires changes to proceed.Jul 31 2018, 2:31 PM

Works for me....

Could you show me your ~/.config/fontconfig/fonts.conf and the content of /etc/fonts/conf.d?

cat ~/.config/fontconfig/fonts.conf 
<?xml version='1.0'?>
<!DOCTYPE fontconfig SYSTEM 'fonts.dtd'>
<fontconfig>
 <dir>~/.fonts</dir>
 <match target="font">
  <edit mode="assign" name="antialias">
   <bool>true</bool>
  </edit>
 </match>
</fontconfig>
ls /etc/fonts/conf.d
10-antialias.conf                          57-dejavu-serif.conf
10-hinting-slight.conf                     58-dejavu-lgc-sans-mono.conf
10-hinting.conf                            58-dejavu-lgc-sans.conf
10-scale-bitmap-fonts.conf                 58-dejavu-lgc-serif.conf
11-lcdfilter-default.conf                  60-latin.conf
20-unhint-small-dejavu-lgc-sans-mono.conf  64-language-selector-prefer.conf
20-unhint-small-dejavu-lgc-sans.conf       65-fonts-lmodern.conf
20-unhint-small-dejavu-lgc-serif.conf      65-fonts-persian.conf
20-unhint-small-dejavu-sans-mono.conf      65-fonts-takao-pgothic.conf
20-unhint-small-dejavu-sans.conf           65-khmer.conf
20-unhint-small-dejavu-serif.conf          65-nonlatin.conf
20-unhint-small-vera.conf                  69-language-selector-zh-cn.conf
30-cjk-aliases.conf                        69-language-selector-zh-hk.conf
30-metric-aliases.conf                     69-language-selector-zh-mo.conf
30-urw-aliases.conf                        69-language-selector-zh-sg.conf
40-nonlatin.conf                           69-language-selector-zh-tw.conf
45-latin.conf                              69-unifont.conf
49-sansserif.conf                          70-no-bitmaps.conf
50-user.conf                               80-delicious.conf
51-local.conf                              90-synthetic.conf
56-neon-hack.conf                          99-language-selector-zh.conf
56-neon-noto.conf                          99pdftoopvp.conf
57-dejavu-sans-mono.conf                   README

I deliberately did not delete ~/.config/fontconfig/fonts.conf and ~/.config/kcmfonts as specified in the test plan because I wanted to test the upgrade case. :)

could you give me all those files?

Sure, here you go:

progwolff updated this revision to Diff 38969.Aug 2 2018, 6:23 PM
  • fix writing config temporarily (previews) with missing local config entries

With this latest version, the issue I mentioned earlier persists for me. I did briefly get sub-pixel rendering again to work by switching the hinting style to Full, but then after switching back to something else and then back to Full again, it no longer worked and I was back to no sub-pixel rendering again. Feels kinda buggy.

progwolff planned changes to this revision.Aug 3 2018, 6:12 AM

With this latest version, the issue I mentioned earlier persists for me. I did briefly get sub-pixel rendering again to work by switching the hinting style to Full, but then after switching back to something else and then back to Full again, it no longer worked and I was back to no sub-pixel rendering again. Feels kinda buggy.

Very strange...

I still cannot reproduce your issue. Any idea what I could try?

I'm doing all my testing in a KDE Neon VM, FWIW. Would you be able to set that one up and try it there to see if you can reproduce it?

I installed a fresh Neon (dev unstable), removed the preinstalled files from /etc/fonts, copied your files to /etc/fonts and applied this patch.
Still cannot reproduce your issue.

ngraham resigned from this revision.Aug 26 2018, 4:34 PM

OK, maybe there's something weird with my config somehow. :/

Can anyone else reproduce? In the meantime @progwolff, maybe it would be best to continue anyway until and unless anyone else can confirm my issue. Is this reviewable now, or are you planning more changes?

progwolff requested review of this revision.Aug 26 2018, 4:53 PM

Are there any other outstanding issues, or is this ready for review?

Ping? Is this ready for review again?

Ping? @progwolff? I just tried this out again in a fresh Neon dev unstable VM after rebasing on master and fixing the merge conflicts and got the same result: the settings in the KCM accurately depict the fontconfig's default settings, but they're not actually applied in the UI.

Are you planning any further work on this?

mart accepted this revision.May 23 2019, 1:50 PM
This revision is now accepted and ready to land.May 23 2019, 1:50 PM
bshah closed this revision.May 27 2019, 9:20 AM
bshah added a subscriber: bshah.

This was merged in https://phabricator.kde.org/D21362 , thanks for your patch.