Bug 17722 - Don't overwrite user's configurations in default config
Summary: Don't overwrite user's configurations in default config
Status: RESOLVED FIXED
Alias: None
Product: fontconfig
Classification: Unclassified
Component: conf (show other bugs)
Version: 2.4
Hardware: Other All
: medium normal
Assignee: Akira TAGOH
QA Contact: Behdad Esfahbod
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-22 15:27 UTC by Behdad Esfahbod
Modified: 2012-03-25 19:00 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Behdad Esfahbod 2008-09-22 15:27:25 UTC
Currently default config files have things like:

  <match target="font">
    <edit name="rgba" mode="assign"><const>none</const></edit>
  </match>

This is wrong as it overwrites the rgba element unconditionally.  Even if the user had it set to other values.  Conf should only set these if not currently set.
Comment 1 Akira TAGOH 2012-03-12 23:10:45 UTC
Is it maybe a good idea to have is_set and not_set for compare attribute?

<match target="font">
  <test name="rgba" compare="not_set" />
  <edit name="rgba" mode="assign"><const>none</const></edit>
</match>

Even though it may be hard to determine which one (no-sub-pixel, sub-pixel-{bgr,rgb,vbgr,vrgb}) is better for. I guess this kind of config is never enabled by default and should simply leave it to the desktop preferences.
Comment 2 Karl Tomlinson 2012-03-13 14:37:20 UTC
qual="all" can actually already be used to test that a value is not set because it always returns true when there are no values to test.  i.e. All zero values match.

For the rgba case, overriding a value of "unknown" seems quite reasonable.

<match target="font">
  <test name="rgba" qual="all"><const>unknown</const></test>
  <edit name="rgba" mode="assign"><const>none</const></edit>
</match>

The match will apply when rgba is set to "unknown" or when rgba is not set.

For some other properties there is no unknown value, and so using qual="all" gets more obscure because we have to compare with a value that will never match an appropriately set value.  e.g.

<match target="font">
  <test name="hintstyle" qual="all"><int>-1</int></test>
  <edit name="hintstyle" mode="assign"><const>hintmedium</const></edit>
</match>

I found that making these "set when not already set" rules work appropriately with Qt3 required an additional hack.  Qt3 was calling FcFontMatch (and thus FcFontRenderPrepare) before XftFontMatch (which calls FcFontMatch again after
XftDefaultSubstitute).  Xft screen options had not been applied in the
first FcFontRenderPrepare (before XftFontMatch), and so the rules were matching and setting values that would take precedence over Xft screen options.  This situation was detectable through missing pixelsize as FcDefaultSubstitute had not been called before the first FcFontRenderPrepare.  I haven't looked at Qt4.

This test matches only when pixelsize is set (-1.0 is considered an inappropriate value).

  <test name="pixelsize" target="pattern" compare="not_eq">
    <double>-1.0</double>
  </test>
Comment 3 Karl Tomlinson 2012-03-13 14:38:58 UTC
(In reply to comment #1)
> I guess this kind of config is
> never enabled by default and should simply leave it to the desktop preferences.

You would hope so, but some distribution(s) where enabling these by default.
Hopefully they have stopped doing that now.
Comment 4 Akira TAGOH 2012-03-13 20:38:48 UTC
(In reply to comment #2)
> qual="all" can actually already be used to test that a value is not set because
> it always returns true when there are no values to test.  i.e. All zero values
> match.

Hmm, that sounds like a bug though. IMHO we should drop such config entirely instead of ignoring the invalid element only or warn it at least, because there may be a typo and it's hard to track down when something behaves unexpectedly.
Comment 5 Karl Tomlinson 2012-03-14 02:25:55 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > qual="all" can actually already be used to test that a value is not set because
> > it always returns true when there are no values to test.  i.e. All zero values
> > match.
> 
> Hmm, that sounds like a bug though.

Adding "set" and "not_set" comparisons sounds sensible to make things clearer,
but I don't think the "all" behavior is a bug.

Please don't change the behavior of all.
It is this behavior that makes all,not_eq the complement of any,eq.
Comment 6 Akira TAGOH 2012-03-14 02:57:11 UTC
(In reply to comment #5)
> Adding "set" and "not_set" comparisons sounds sensible to make things clearer,
> but I don't think the "all" behavior is a bug.

the behavior of qual="all" itself isn't a problem. I just think either of cases should be checked with qual="all" compare="not_eq" even if the testcase contains any invalid thing, as you indicated in the last example. that really makes sense for what you want to. but relying on "All zero values match" isn't a good idea, because it's the side-effect because of no-error-handling.

> Please don't change the behavior of all.
> It is this behavior that makes all,not_eq the complement of any,eq.

If there are too much cases relying on this behavior, we should show a warning at least if something failed to parse, because it's difficult to see whether something might looks like a typo is desired or not. let's see how much things using this trick and we can avoid an regression as much as possible.
Comment 7 Behdad Esfahbod 2012-03-17 12:10:13 UTC
An alternative may be to simply make these configs append values instead of assign.  Most clients I guess care about the first value only.
Comment 8 Akira TAGOH 2012-03-22 04:32:08 UTC
(In reply to comment #7)
> An alternative may be to simply make these configs append values instead of
> assign.  Most clients I guess care about the first value only.

Aha. that sounds reasonable.
Comment 9 Behdad Esfahbod 2012-03-22 07:34:12 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > An alternative may be to simply make these configs append values instead of
> > assign.  Most clients I guess care about the first value only.
> 
> Aha. that sounds reasonable.

If you do that, please also add a big comment stating what's going on.
Comment 10 Akira TAGOH 2012-03-22 07:51:09 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > An alternative may be to simply make these configs append values instead of
> > > assign.  Most clients I guess care about the first value only.
> > 
> > Aha. that sounds reasonable.
> 
> If you do that, please also add a big comment stating what's going on.

Sure. I'll do that.
Comment 11 Akira TAGOH 2012-03-23 00:23:38 UTC
I guess we need to do similar for 10-autohint.conf, 10-unhinted.conf and 11-lcd-filter-*.conf too?
Comment 12 Akira TAGOH 2012-03-23 00:30:56 UTC
the proposed patch:
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz17722
Comment 13 Behdad Esfahbod 2012-03-24 10:17:56 UTC
(In reply to comment #11)
> I guess we need to do similar for 10-autohint.conf, 10-unhinted.conf and
> 11-lcd-filter-*.conf too?

Yes, I think so.
Comment 14 Akira TAGOH 2012-03-25 19:00:46 UTC
Fixed in 1aaf8b77.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.