bug#26552: [patch] nlinum margin width calculation (emacs <25)

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Christophe Rhodes]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

Hi,

I think the calculation that nlinum-mode does to guess the margin size
in the absence of font width information (in emacs versions less than
25) uses the wrong bit of font information: the pixel size, rather than
the font height.  Trivial patch below, which fixes the problem for me.

Best wishes,

Christophe


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: nlinum.diff --]
[-- Type: text/x-diff, Size: 384 bytes --]

--- nlinum.el~	2017-04-17 20:41:35.595122970 +0100
+++ nlinum.el	2017-04-17 21:46:37.713053714 +0100
@@ -71,7 +71,7 @@
   (nlinum--setup-windows))
 
 (defun nlinum--face-height (face)
-  (aref (font-info (face-font face)) 2))
+  (aref (font-info (face-font face)) 3))
 
 (defun nlinum--face-width (face)        ;New info only in Emacs>=25.
   (let ((fi (font-info (face-font face))))

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Eli Zaretskii]

> From: Christophe Rhodes <csr21@cantab.net>
> Date: Mon, 17 Apr 2017 22:26:56 +0100
> 
> I think the calculation that nlinum-mode does to guess the margin size
> in the absence of font width information (in emacs versions less than
> 25) uses the wrong bit of font information: the pixel size, rather than
> the font height.  Trivial patch below, which fixes the problem for me.

Isn't it a mere coincidence that this fixes your problem?  How can
height be a better approximation for width than size?  What are the
actual numbers in your case?

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Christophe Rhodes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Christophe Rhodes <csr21@cantab.net>
>> Date: Mon, 17 Apr 2017 22:26:56 +0100
>> 
>> I think the calculation that nlinum-mode does to guess the margin size
>> in the absence of font width information (in emacs versions less than
>> 25) uses the wrong bit of font information: the pixel size, rather than
>> the font height.  Trivial patch below, which fixes the problem for me.
>
> Isn't it a mere coincidence that this fixes your problem?

I don't think so.

The calculation is trying to compute how many characters should be
reserved in the margin, which will on a graphical display (I presume)
reserve space in units of frame-char-width.

In the case that the face for linum/nlinum is the same as the face for
everything else (in particular, the face used for frame-char-height), we
want to reserve the number of character cells that is equal to the
length of the strings we will need to format, nlinum--width.  But
frame-char-height is the height (in pixels) of the lines in the frame,
not the height of the characters in the font -- it includes leading.
So, when computing the number of characters to reserve, the inclusion of
leading in the denominator but not the numerator eventually leads to an
underestimate in the number of characters to reserve.

> How can height be a better approximation for width than size?

It's not, but the ratio of one height (the font height) to another
height (frame-char-height) is a better approximation to the scaling
ratio between widths, which is what really matters, than the ratio of
the font pixel size to the frame-char-height (which are two quantities
without a direct relationship).

> What are the actual numbers in your case?

My font is Source Code Pro 10, which has pixel size 13 and height 17;
frame-char-height is 17.  So once nlinum--width is 5, the current
computation underestimates the space needed; with my patch, the
calculation is correct for all values of nlinum--width.  I haven't
tested with different sizes for the linum face and the normal face; I
expect my patched calculation to approximate better than the existing
one in essentially all cases.

Best wishes,

Christophe

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Christophe Rhodes]

Christophe Rhodes <csr21@cantab.net> writes:

> I haven't tested with different sizes for the linum face and the
> normal face; I expect my patched calculation to approximate better
> than the existing one in essentially all cases.

I have now tested with different sizes for the linum and normal face,
and in my testing the code with my patch performs as I expect, reserving
enough space in all cases for the line numbers in the margin.

Please could the patch I sent be merged, or else please let me know what
further investigation needs to be done?

Thank you,

Christophe

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Eli Zaretskii]

> From: Christophe Rhodes <csr21@cantab.net>
> Date: Sat, 22 Apr 2017 09:33:30 +0100
> 
> Christophe Rhodes <csr21@cantab.net> writes:
> 
> > I haven't tested with different sizes for the linum face and the
> > normal face; I expect my patched calculation to approximate better
> > than the existing one in essentially all cases.
> 
> I have now tested with different sizes for the linum and normal face,
> and in my testing the code with my patch performs as I expect, reserving
> enough space in all cases for the line numbers in the margin.
> 
> Please could the patch I sent be merged, or else please let me know what
> further investigation needs to be done?

Stefan?

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Stefan Monnier]

>> Please could the patch I sent be merged, or else please let me know what
>> further investigation needs to be done?
> Stefan?

Fine by me.  Those subtleties of fonts tend to elude me,


        Stefan

bug#26552: [patch] nlinum margin width calculation (emacs <25)

[Noam Postavsky]

tags 26552 fixed
close 26552 
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Please could the patch I sent be merged, or else please let me know what
>>> further investigation needs to be done?
>> Stefan?
>
> Fine by me.  Those subtleties of fonts tend to elude me,

It looks to me like it's a trivial typo/off-by-one error.  I pushed to
elpa, and bumped the version to 1.8.1.

[1: b14a6c963]: 2017-11-04 10:20:08 -0400
  Fix nlinum face height function (Bug#26552)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b14a6c9632e8a1016f994b6e2ca642d9a6bb30cd

[2: 77cf1c3bb]: 2017-11-04 10:34:57 -0400
  * packages/nlinum/nlinum.el: Bump version to 1.8.1.
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=77cf1c3bbbe7a18d08f380693d63e04b9ca60c80