Bug 71287

Okay, finally can introduce the initial code for this feature.

http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=new-size-selection-mechanism

I did test this by having the following recipe so that I don't have fonts containing those properties in OS/2 table.

<fontconfig>
<match target="scan">
  <test name="file" compare="eq">
    <string>/path/to/fixed.pcf</string>
  </test>
  <edit name="sizerange" mode="assign">
    <range>
      <double>10</double>
      <double>12</double>
    </range>
  </edit>
</match>
</fontconfig>

% ./fc-match/fc-match -f "%{family}:file=%{file}\n" "Fixed:size=9"
Fixed:file=/usr/share/fonts/knm-new-fixed/kaname-latin1.pcf.gz
% ./fc-match/fc-match -f "%{family}:file=%{file}\n" "Fixed:size=10"
Fixed:file=/path/to/fixed.pcf
% ./fc-match/fc-match -f "%{family}:file=%{file}\n" "Fixed:size=11"
Fixed:file=/path/to/fixed.pcf
% ./fc-match/fc-match -f "%{family}:file=%{file}\n" "Fixed:size=12"
Fixed:file=/usr/share/fonts/knm-new-fixed/kaname-latin1.pcf.gz

Okay, take 2 according to feedback on the list:
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=new-size-selection-mechanism-2

<fontconfig>
<match target="scan">
  <test name="file" compare="eq">
    <string>/path/to/fixed.pcf</string>
  </test>
  <edit name="size" mode="assign">
    <range>
      <double>10</double>
      <double>12</double>
    </range>
  </edit>
</match>
</fontconfig>

same results to comment#1

Thanks.  Some more feedback:

- Why the renaming in FcConfigEvaluate()?  It takes a large chunk of the patch unnecessarily,

- In _FcValuePrintFile, perhaps better to print range using () instead of {}?

- I think we want to make the FC_SIZE to be able to accept both double and range.  I think you've got that already.  As such, please limit FcDefaultSubstitute changes to use double when a range is not necessary.  Ideally we shouldn't see any changes to that function whatsoever.  In fact, I think it would be fine to expect that the query-pattern has a double size, not a range size.  Think of this as FC_LANG.  The font patterns have a FcLangSet object for FC_LANG, but query-pattern has a string FC_LANG.

- I'd separate the cache version jump to a separate commit.

- FcCompareSizeRange() should return the minimum distance from one range to another.  If the two ranges overlap, that's zero.  Otherwise, return the minimum distance from one range's end to the other's.

- Again, in FcNameParse, keep thee AddDouble as is.  Possibly add a way to specify a range, but that's not really important at least initially.

(In reply to comment #3)
> - Why the renaming in FcConfigEvaluate()?  It takes a large chunk of the
> patch unnecessarily,

Because those vl and vr is destroyed at the end but FcValue returned by FcConfigPromote() isn't malloc'd one. why it wasn't a problem ever is this promotion won't be happened for FcLangSet in FcConfigEvaluate() but FcConfigCompareValue().

> - In _FcValuePrintFile, perhaps better to print range using () instead of {}?

Aha. well, I don't have strong opinion on that format. just followed on something we had a discussion for the range on the list recently.

> - I think we want to make the FC_SIZE to be able to accept both double and
> range.  I think you've got that already.  As such, please limit
> FcDefaultSubstitute changes to use double when a range is not necessary. 
> Ideally we shouldn't see any changes to that function whatsoever.  In fact,
> I think it would be fine to expect that the query-pattern has a double size,
> not a range size.  Think of this as FC_LANG.  The font patterns have a
> FcLangSet object for FC_LANG, but query-pattern has a string FC_LANG.

Sure.

> - I'd separate the cache version jump to a separate commit.

Okay.

> - FcCompareSizeRange() should return the minimum distance from one range to
> another.  If the two ranges overlap, that's zero.  Otherwise, return the
> minimum distance from one range's end to the other's.

Aha. sounds good.

> - Again, in FcNameParse, keep thee AddDouble as is.  Possibly add a way to
> specify a range, but that's not really important at least initially.

will fix that.

Thanks.

Okay, take 3 here:
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=new-size-selection-mechanism-3

The FreeType changes are in 2.5.1 FWIW.

+ lower_size = os2->usLowerOpticalPointSize / 20;
+ upper_size = os2->usUpperOpticalPointSize / 20;

Float division instead?


+ if (os2)
+ {
+ r = FcRangeCreateDouble (lower_size, upper_size);
+ if (!FcPatternAddRange (pat, FC_SIZE, r))

If the range is not available (either old os2 or that the range is all-inclusive), then, NOT set size?  That would preserve most backward-compatibility.


+ if (FcRangeIsInRange (r1, r2))
+ ret = 0.0;
+ else
+ ret = fabs (r1->u.d.end - r2->u.d.end);

This sounds wrong.  If not in-range, then we want min((r1.end - r2.start), abs(r1.start - r2.end)).  No?


+FcRange *
+FcRangeEvaluate (FcOp op, const FcRange *left, const FcRange *right)
+{
+ FcRange *ret = NULL;
+ FcRange cl = FcRangeCanonicalize (left), cr = FcRangeCanonicalize (right);
+
+ switch ((int) op) {
+ case FcOpOr:
+ ret = FcRangeCreateDouble (FC_MIN (cl.u.d.begin, cr.u.d.begin),
+ FC_MAX (cl.u.d.end, cr.u.d.end));
+ break;
+ case FcOpAnd:
+ ret = FcRangeCreateDouble (FC_MAX (cl.u.d.begin, cr.u.d.begin),
+ FC_MIN (cl.u.d.end, cr.u.d.end));
+ break;
+ case FcOpPlus:
+ ret = FcRangeCreateDouble (cl.u.d.begin + cr.u.d.begin,
+ cl.u.d.end + cr.u.d.end);
+ break;
+ case FcOpMinus:
+ ret = FcRangeCreateDouble (cl.u.d.begin - cr.u.d.begin,
+ cl.u.d.end - cr.u.d.end);
+ break;
+ case FcOpTimes:
+ ret = FcRangeCreateDouble (cl.u.d.begin * cr.u.d.begin,
+ cl.u.d.end * cr.u.d.end);
+ break;
+ case FcOpDivide:
+ ret = FcRangeCreateDouble (cl.u.d.begin / cr.u.d.begin,
+ cl.u.d.end / cr.u.d.end);
+ break;

These don't make much sense.  I suppose we can initially not implement any of these, and add as we need them.


I haven't done any testing or a full close review.  It's a bit unsettling how much boilerplate changes we need for this.  But, again, thanks for working on this.

(In reply to comment #8)
> + lower_size = os2->usLowerOpticalPointSize / 20;
> + upper_size = os2->usUpperOpticalPointSize / 20;
> 
> Float division instead?

Ah, you're right. fixed.

> + if (os2)
> + {
> + r = FcRangeCreateDouble (lower_size, upper_size);
> + if (!FcPatternAddRange (pat, FC_SIZE, r))
> 
> If the range is not available (either old os2 or that the range is
> all-inclusive), then, NOT set size?  That would preserve most
> backward-compatibility.

The default value for lower_size and upper_size is set to 0 and DBL_MAX as same as freetype does for old os2 or the case where those properties are not available. so should be no problem even with setting size. why we have to set any value to SIZE would be rather not to get confused in the matcher because it's the matter to affect the score now.
not sure for all-inclusive. after think of the use-case for bitmap fonts where it may be more likely to see this situation, that sounds sane to behave like this.

> + if (FcRangeIsInRange (r1, r2))
> + ret = 0.0;
> + else
> + ret = fabs (r1->u.d.end - r2->u.d.end);
> 
> This sounds wrong.  If not in-range, then we want min((r1.end - r2.start),
> abs(r1.start - r2.end)).  No?

indeed. fixed.

> These don't make much sense.  I suppose we can initially not implement any
> of these, and add as we need them.

Sure. not too much objection on it.

> I haven't done any testing or a full close review.  It's a bit unsettling
> how much boilerplate changes we need for this.  But, again, thanks for
> working on this.

Thank you for the comments!

http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=new-size-selection-mechanism-4

Any other comments on the changes?
I'm pondering to push this into next major release.

Lets commit and see what breaks? ;)

Okay, merged into master. let's see how's it going :)