bug#17973: Thin space not thin at all

bug#17973: Thin space not thin at all

[Stefan Monnier]

Package: Emacs
Version: 24.3.92
Severity: important


   % emacs-24/src/emacs -Q -fn '-misc-fixed-*-r-semicondensed--13-*-*-*-*-*-*'
   M-< C-f C-x SPC

At this point I should see a thin space before the cursor (highlighted
with the region face).  Instead I see a full space.

   C-u C-x =

indeed shows that there's an overlay at point with an after-string of:

   #(" " 0 1 (face (region (:height 0.2))))

Now admittedly, ":height 0.2" does not specify a width, but Emacs should
be able to find a font that's about 2½ pixels high but not
6 pixels wide.

This is related to bug#16403, in that this is probably a long-standing
bug, but the new rectangular region makes it more visible.
Maybe it's actually the same bug as bug#9787 as well, which is also more
visible since *VC-log* uses such a "thin line" to separate the header
from the body.


        Stefan

bug#17973: Thin space not thin at all

[K. Handa]

In article <jwv1ttvpvuu.fsf@iro.umontreal.ca>, Stefan Monnier <monnier@iro.umontreal.ca> writes:

>    % emacs-24/src/emacs -Q -fn '-misc-fixed-*-r-semicondensed--13-*-*-*-*-*-*'
>    M-< C-f C-x SPC

> At this point I should see a thin space before the cursor (highlighted
> with the region face).  Instead I see a full space.

>    C-u C-x =

> indeed shows that there's an overlay at point with an after-string of:

>    #(" " 0 1 (face (region (:height 0.2))))

> Now admittedly, ":height 0.2" does not specify a width, but Emacs should
> be able to find a font that's about 2½ pixels high but not
> 6 pixels wide.

> This is related to bug#16403, in that this is probably a long-standing
> bug, but the new rectangular region makes it more visible.
> Maybe it's actually the same bug as bug#9787 as well, which is also more
> visible since *VC-log* uses such a "thin line" to separate the header
> from the body.

Yes, I agree that they are all the same bug.  As far as I
know, the problem is with the mechanism of face attribute
merging.  When the control reaches the font selection
function font_find_for_lface, font related attributes
(family, foundry, weight, ..., height, ...) are already
merged.  As font_find_for_lface doesn't know which attribute
to respect more (except for what specified in
face-font-selection-order), it at first tries to get a list
of fonts whose family, foundry, registry, adstyle are the
same as merged attributes, and selects a font most close to
the specified height.

To fix this problem, perhaps we must propagate the
information about how font related attributes are merged;
i.e. which one comes from the specific face directly and
which one is inherited from the other faces (including the
default face).  It's not a trivial work.

---
Kenichi Handa
handa@gnu.org

bug#17973: Thin space not thin at all

[Stefan Monnier]

forcemerge 9787 17973
thanks

> Yes, I agree that they are all the same bug.  As far as I
> know, the problem is with the mechanism of face attribute
> merging.  When the control reaches the font selection
> function font_find_for_lface, font related attributes
> (family, foundry, weight, ..., height, ...) are already
> merged.  As font_find_for_lface doesn't know which attribute
> to respect more (except for what specified in
> face-font-selection-order), it at first tries to get a list
> of fonts whose family, foundry, registry, adstyle are the
> same as merged attributes, and selects a font most close to
> the specified height.

So, IIUC you're saying that when we get to selecting a font, we have
specifications such as

  family = fixed
  foundry = misc
  height = 2½ pixels

and we end up choosing the 13pixel-high font because it's the only one
that matches "misc-fixed"?  I do have a 6pixel-high misc-fixed font
(tho not semicondensed), so it seems like it's not the
whole explanation.

Or is the "13pixel high" specification kept somewhere (elsewhere than in
the "height", obviously)?

I'm beginning to sense that the "13pixel high" specification is indeed
kept elsewhere, and it is kept so as to avoid other problems I've had in
the past, where the 13pixel spec was turned into a height spec and that
this height spec was then used to look for a font and it occasionally
found *another* font with the same height in points but not in pixels.

Is that right?

OTOH, doing a M-x customize-face REt default RET, then setting family to
`fixed' and foundry to `misc', and then playing with `height' is pretty
scary: height=2000 gives me a 9pixel-high font (!) whereas setting it to
200 gives a more reasonable 20pixel-high font.


        Stefan

bug#17973: Thin space not thin at all

[K. Handa]

In article <jwvion6nz07.fsf-monnier+emacsbugs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So, IIUC you're saying that when we get to selecting a font, we have
> specifications such as

>   family = fixed
>   foundry = misc
>   height = 2½ pixels

> and we end up choosing the 13pixel-high font because it's the only one
> that matches "misc-fixed"?

Yes.  At least that is what I observed in my environment.

> I do have a 6pixel-high misc-fixed font

Hmmm, then your environment is different from mine.

> (tho not semicondensed), so it seems like it's not the
> whole explanation.

Do you customize face-font-selection-order?  It's default
value is (:width :height :weight :slant), which means that
the function font_select_entity (called from
font_find_for_lface) selects a font whose :width is
semicondensed even if that font has very different height
from what specified.

> Or is the "13pixel high" specification kept somewhere (elsewhere than in
> the "height", obviously)?

I don't think so.

> OTOH, doing a M-x customize-face REt default RET, then setting family to
> `fixed' and foundry to `misc', and then playing with `height' is pretty
> scary: height=2000 gives me a 9pixel-high font (!) whereas setting it to
> 200 gives a more reasonable 20pixel-high font.

Ummm, then perhaps font_score has a bug.  As far as I
remember, that function treats such a very big font size
specially.  I'll check the code.

---
Kenichi Handa
handa@gnu.org

bug#17973: Thin space not thin at all

[Stefan Monnier]

> Do you customize face-font-selection-order?  It's default
> value is (:width :height :weight :slant), which means that
> the function font_select_entity (called from
> font_find_for_lface) selects a font whose :width is
> semicondensed even if that font has very different height
> from what specified.

Ah, right, that explains it.  Hmm... I guess ideally, Emacs should
consider a height that's "too far" from the requested one as a failure
and then try again ignoring some of the specs.

The patch below expresses the first part, but it looks like the second
part doesn't exit: Emacs just doesn't find any font to use for the "thin
space" of C-x SPC and indicates it to me with one of those big squares
that say "0020", which is a lot more intrusive than the problem I'm
trying to fix.

And (setq scalable-fonts-allowed t) doesn't help either.

Maybe a solution/workaround for the specific problem of such "thin
lines" is to replace the (:height 0.2) face with another face that
specifies "any family".  If I use (:height 0.2 :family "Monospace"),
indeed, the problem disappears.  Is there a better "any family" than
"Monospace"?


        Stefan


PS: The patch below also happens to give me assertion failures
fontset.c:897: Emacs fatal error: assertion failed: fontset_id_valid_p (face->fontset)
I haven't investigated any further, tho (and the line number might be off
because of local changes anyway).


=== modified file 'src/font.c'
--- src/font.c	2014-07-09 13:45:53 +0000
+++ src/font.c	2014-07-10 16:11:04 +0000
@@ -2165,10 +2165,14 @@
 	 lowest bit is set if the DPI is different.  */
       EMACS_INT diff;
       EMACS_INT pixel_size = XINT (spec_prop[FONT_SIZE_INDEX]);
+      EMACS_INT entity_size = XINT (AREF (entity, FONT_SIZE_INDEX));
 
       if (CONSP (Vface_font_rescale_alist))
 	pixel_size *= font_rescale_ratio (entity);
-      diff = eabs (pixel_size - XINT (AREF (entity, FONT_SIZE_INDEX))) << 1;
+      if (pixel_size * 2 < entity_size || entity_size * 2 < pixel_size)
+	/* This size is wrong by more than a factor 2: reject it!  */
+	return 0xFFFFFFFF;
+      diff = eabs (pixel_size - entity_size) << 1;
       if (! NILP (spec_prop[FONT_DPI_INDEX])
 	  && ! EQ (spec_prop[FONT_DPI_INDEX], AREF (entity, FONT_DPI_INDEX)))
 	diff |= 1;

bug#17973: Thin space not thin at all

[K. Handa]

In article <jwvzjghmeli.fsf-monnier+emacsbugs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Ah, right, that explains it.  Hmm... I guess ideally, Emacs should
> consider a height that's "too far" from the requested one as a failure
> and then try again ignoring some of the specs.

font_find_for_lface has a code to do that, but I found that
it is called with full font properties in SPEC.  This
function does not try a font of different properties
(foundry, family, registry, adstyle).

> The patch below expresses the first part, but it looks like the second
> part doesn't exit: Emacs just doesn't find any font to use for the "thin
> space" of C-x SPC and indicates it to me with one of those big squares
> that say "0020", which is a lot more intrusive than the problem I'm
> trying to fix.

In addition to your patch, could you please try the following patch?

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2014-07-07 23:33:05 +0000
+++ src/xfaces.c	2014-07-13 15:09:21 +0000
@@ -5547,7 +5547,7 @@
 	}
       if (! FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
 	attrs[LFACE_FONT_INDEX]
-	  = font_load_for_lface (f, attrs, attrs[LFACE_FONT_INDEX]);
+	  = font_load_for_lface (f, attrs, Ffont_spec (0, NULL));
       if (FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
 	{
 	  face->font = XFONT_OBJECT (attrs[LFACE_FONT_INDEX]);

> PS: The patch below also happens to give me assertion failures
> fontset.c:897: Emacs fatal error: assertion failed: fontset_id_valid_p (face->fontset)
> I haven't investigated any further, tho (and the line number might be off
> because of local changes anyway).

I didn't face with that error, but I'll keep on checking the code.

---
Kenichi Handa
handa@gnu.org

bug#17973: Thin space not thin at all

[Stefan Monnier]

> In addition to your patch, could you please try the following patch?

> === modified file 'src/xfaces.c'
> --- src/xfaces.c	2014-07-07 23:33:05 +0000
> +++ src/xfaces.c	2014-07-13 15:09:21 +0000
> @@ -5547,7 +5547,7 @@
>  	}
>        if (! FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
>  	attrs[LFACE_FONT_INDEX]
> -	  = font_load_for_lface (f, attrs, attrs[LFACE_FONT_INDEX]);
> +	  = font_load_for_lface (f, attrs, Ffont_spec (0, NULL));
>        if (FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
>  	{

Looks like this works, indeed!  Yay!


        Stefan

bug#17973: Thin space not thin at all

[K. Handa]

In article <jwvha2danew.fsf-monnier+emacsbugs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > In addition to your patch, could you please try the following patch?
[...]
> Looks like this works, indeed!  Yay!

I've just committed both changes to the trunk.  But, in this part:

      if (pixel_size * 2 < entity_size || entity_size * 2 < pixel_size)
	/* This size is wrong by more than a factor 2: reject it!  */
	return 0xFFFFFFFF;

the factor 2 is too arbitrary.  Don't we need some
user-controllable variable here?

---
Kenichi Handa
handa@gnu.org

bug#17973: Thin space not thin at all

[Stefan Monnier]

>> > In addition to your patch, could you please try the following patch?
> [...]
>> Looks like this works, indeed!  Yay!
> I've just committed both changes to the trunk.  But, in this part:

>       if (pixel_size * 2 < entity_size || entity_size * 2 < pixel_size)
> 	/* This size is wrong by more than a factor 2: reject it!  */
> 	return 0xFFFFFFFF;

> the factor 2 is too arbitrary.  Don't we need some
> user-controllable variable here?

It's indeed arbitrary.  It might deserve a CPP macro, but I'd rather not
add a configurable variable until there's a clear need for it.
2 seems to be large enough that it is hard to imagine a case where it
will rule out a font that the user would want to use, yet it's small
enough that it should solve the problem in the vast majority of cases
where it matters.


        Stefan