bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

With the new display-line-numbers-mode, if you customize faces to
something like, say:

 '(line-number ((t (:height 0.85))))
 '(line-number-current-line ((t (:height 0.85))))

Then the cursor will be ill-aligned (a bit shifted to the right) when
it's in the last line of the buffer. This line has no number and it
seems to be that the spacing is miscalculated using the default face
size.

---

In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
 of 2018-07-05 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12000000
Recent messages:
Whitespace mode disabled in current buffer
Auto-saving...done
Quit [2 times]
C-h C-g is undefined
Quit [3 times]
apply: Command attempted to use minibuffer while in minibuffer
ESC M-x is undefined
ESC ESC <mouse-1> is undefined
ESC ESC M-x is undefined
previous-line: Beginning of buffer
Quit [4 times]
Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-modules
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong
 -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS
GTK3 X11 MODULES THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: C/*l

Minor modes in effect:
  global-magit-file-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  global-display-line-numbers-mode: t
  display-line-numbers-mode: t
  pdf-occur-global-minor-mode: t
  diff-auto-refine-mode: t
  pyvenv-mode: t
  shell-dirtrack-mode: t
  xclip-mode: t
  winner-mode: t
  show-paren-mode: t
  ido-ubiquitous-mode: t
  ido-everywhere: t
  global-company-mode: t
  company-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  abbrev-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs debug
ob-ditaa ob-plantuml org-archive org-bibtex bibtex org-capture org-clock
org-colview org-crypt org-habit org-agenda org-indent org-table cl-print
completion dabbrev autoload lisp-mnt magit-bookmark magit-obsolete
magit-blame magit-stash magit-bisect magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-collab ghub-graphql ghub url-http tls
gnutls url-gw nsm url-auth url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util mailcap
magit-files magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log which-func magit-diff smerge-mode magit-core
magit-autorevert autorevert filenotify magit-process magit-margin
magit-mode git-commit magit-git magit-section magit-utils crm
magit-popup log-edit message rmc puny rfc822 mml mml-sec epa derived epg
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader pcvs-util add-log with-editor async-bytecomp async dash
tabify misearch multi-isearch face-remap vc-git whitespace server
display-line-numbers checkdoc pdf-occur ibuf-ext ibuffer
ibuffer-loaddefs tablist tablist-filter semantic/wisent/comp
semantic/wisent semantic/wisent/wisent semantic/util-modes semantic/util
semantic semantic/tag semantic/lex semantic/fw mode-local cedet dired
dired-loaddefs pdf-isearch let-alist pdf-misc imenu pdf-tools pdf-view
bookmark pp jka-compr pdf-cache pdf-info tq pdf-util image-mode
org-protocol org-element avl-tree generator org org-macro org-footnote
org-pcomplete org-list org-faces org-entities noutline outline
org-version ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob ob-table
ob-keys ob-exp ob-comint ob-core ob-eval org-compat org-macs
org-loaddefs find-func cal-menu calendar cal-loaddefs cl-extra yasnippet
elec-pair highlight-indentation flymake-proc flymake warnings help-fns
radix-tree help-mode elpy find-file-in-project ivy delsel colir color
ivy-overlay ffap thingatpt windmove diff-mode easy-mmode elpy-shell
pyvenv esh-var esh-io esh-cmd esh-opt esh-ext esh-proc esh-arg
esh-groups eshell esh-module esh-mode esh-util elpy-profile elpy-django
elpy-refactor subr-x python tramp-sh tramp tramp-compat tramp-loaddefs
trampver ucs-normalize shell pcomplete parse-time format-spec advice
json map grep compile comint ansi-color files-x doom-themes-org
doom-tomorrow-night-theme doom-themes doom-themes-common company-oddmuse
company-keywords company-etags etags xref project company-gtags
company-dabbrev-code company-dabbrev company-files company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-bbdb xclip winner ring paren
ido-completing-read+ memoize s cus-edit minibuf-eldef ido gnus nnheader
gnus-util rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums time-date
mail-utils mm-util mail-prsvr wid-edit company edmacro kmacro pcase
cus-start cus-load finder-inf info package easymenu epg-config
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 1265435 39917)
 (symbols 48 52701 1)
 (miscs 40 4729 1375)
 (strings 32 163288 1415)
 (string-bytes 1 4870943)
 (vectors 16 85036)
 (vector-slots 8 1342564 39156)
 (floats 8 612 542)
 (intervals 56 5026 459)
 (buffers 992 53))

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Maybe this line should also get a line number when the cursor is there.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Tue, 31 Jul 2018 16:19:39 -0300
> 
> With the new display-line-numbers-mode, if you customize faces to
> something like, say:
> 
>  '(line-number ((t (:height 0.85))))
>  '(line-number-current-line ((t (:height 0.85))))
> 
> Then the cursor will be ill-aligned (a bit shifted to the right) when
> it's in the last line of the buffer. This line has no number and it
> seems to be that the spacing is miscalculated using the default face
> size.

The empty "line" beyond EOB has the line-number field displayed using
the default face, indeed.  That's by design: using the same face as
for line numbers produces ugly effects, IMO, e.g., if the line-number
face defines a background color.  So I decided to use the default face
for that single place.  It doesn't look too bad, IMO, even in the case
you reported, as there's no text after the cursor.

I'd rather not introduce more complexity (like user options etc.),
unless the current display is deemed entirely unacceptable in such
cases.

Thanks.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Tue, 31 Jul 2018 16:26:28 -0300
> 
> Maybe this line should also get a line number when the cursor is there.

No, I don't think so.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

And what about showing the eof unicode glyph, it's just one character so it
can't affect width calculations and it's a useful indicator that says
"there is really nothing here".

On Wed, Aug 1, 2018, 4:48 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Carlos Pita <carlosjosepita@gmail.com>
> > Date: Tue, 31 Jul 2018 16:26:28 -0300
> >
> > Maybe this line should also get a line number when the cursor is there.
>
> No, I don't think so.
>

[-- Attachment #2: Type: text/html, Size: 820 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

Well, it's eot (2404). Also a predetermined ascii character could be given
that conventional meaning (given your reluctance to add an option).

[-- Attachment #2: Type: text/html, Size: 169 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 1 Aug 2018 07:33:07 -0300
> Cc: 32337@debbugs.gnu.org
> 
> And what about showing the eof unicode glyph, it's just one character so it can't affect width calculations and
> it's a useful indicator that says "there is really nothing here".

Showing it where?

We already have an EOB indication, see indicate-buffer-boundaries.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

I meant as a pseudo line-number so that the line-number face geometry is
used.

On Wed, Aug 1, 2018, 8:50 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Carlos Pita <carlosjosepita@gmail.com>
> > Date: Wed, 1 Aug 2018 07:33:07 -0300
> > Cc: 32337@debbugs.gnu.org
> >
> > And what about showing the eof unicode glyph, it's just one character so
> it can't affect width calculations and
> > it's a useful indicator that says "there is really nothing here".
>
> Showing it where?
>
> We already have an EOB indication, see indicate-buffer-boundaries.
>

[-- Attachment #2: Type: text/html, Size: 1018 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 1 Aug 2018 09:03:54 -0300
> Cc: 32337@debbugs.gnu.org
> 
> I meant as a pseudo line-number so that the line-number face geometry is used.

If people who use this mode don't object to seeing that marker always,
I don't mind, either, although as I said we already have another means
of indicating the end of the buffer.  Feel free to do a user poll.

In any case, we'd need to come up with an ASCII character, because
Unicode characters might not be available on TTYs.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

>
> Feel free to do a user poll.
>

Is there any established mean for this? Maybe reddit?

>

[-- Attachment #2: Type: text/html, Size: 484 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 1 Aug 2018 09:31:30 -0300
> Cc: 32337@debbugs.gnu.org
> 
>  Feel free to do a user poll.
> 
> Is there any established mean for this?

Not that I know of.

> Maybe reddit?

Should be fine, IMO.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

If someone seriously wants to do a poll, I can explain how we used to
do it, and set up the infrastructure.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

> If someone seriously wants to do a poll, I can explain how we used to
> do it, and set up the infrastructure.

Maybe it would be too much for this minor issue only, although if the
point is to set a precedent it's ok for me. But since that precedent
would have much more consequences for Eli than for me I don't think
this is my call.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Thu, 2 Aug 2018 20:06:46 -0300
> Cc: Eli Zaretskii <eliz@gnu.org>, 32337@debbugs.gnu.org
> 
> > If someone seriously wants to do a poll, I can explain how we used to
> > do it, and set up the infrastructure.
> 
> Maybe it would be too much for this minor issue only, although if the
> point is to set a precedent it's ok for me. But since that precedent
> would have much more consequences for Eli than for me I don't think
> this is my call.

I agree that this issue is too minor to justify a serious poll.  A
bunch of opinions should be enough for the initial decision.  A more
serious poll setup could only be justified if we discover some
controversy in the opinion, which I currently don't envision.

So a question posted on Redit should be good enough at this stage, I
think.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Done, I will aggregate and report the results here in a week.

That said, my personal preference is for $ since it is ASCII and has
the conventional meaning of last line, without being a number itself.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

Eli, it ocurred to me that the line-number indicator could be extended all
the way down to the bottom of the window so it doesn't look weird when
empty, since one of your objections was about having that hanging empty
last line indicator. What do you think about that option?

Btw, people in reddit seems to mostly prefer ~ as the ASCII char since it's
the vim empty line indicator.

On Fri, Aug 3, 2018, 3:40 PM Carlos Pita <carlosjosepita@gmail.com> wrote:

> Done, I will aggregate and report the results here in a week.
>
> That said, my personal preference is for $ since it is ASCII and has
> the conventional meaning of last line, without being a number itself.
>

[-- Attachment #2: Type: text/html, Size: 1027 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Fri, 3 Aug 2018 20:14:29 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> Eli, it ocurred to me that the line-number indicator could be extended all the way down to the bottom of the
> window so it doesn't look weird when empty, since one of your objections was about having that hanging
> empty last line indicator. What do you think about that option?

That looked weird (a.k.a. "bug-like") in my experiments, when the face
defines colors.  Moreover, currently we draw nothing at all beyond
EOB, we just clear the rest of the window.  So this would need special
tricky code that will complicate the display engine, something I'd
like to avoid.

> Btw, people in reddit seems to mostly prefer ~ as the ASCII char since it's the vim empty line indicator.

Fine with me ('$' might be confusing anyway, because it's the default
for the left truncation glyph on TTY frames).

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> Date: Sat, 04 Aug 2018 09:29:36 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> > From: Carlos Pita <carlosjosepita@gmail.com>
> > Date: Fri, 3 Aug 2018 20:14:29 -0300
> > Cc: rms@gnu.org, 32337@debbugs.gnu.org
> > 
> > Eli, it ocurred to me that the line-number indicator could be extended all the way down to the bottom of the
> > window so it doesn't look weird when empty, since one of your objections was about having that hanging
> > empty last line indicator. What do you think about that option?
> 
> That looked weird (a.k.a. "bug-like") in my experiments, when the face
> defines colors.

Yes.

> Moreover, currently we draw nothing at all beyond EOB, we just clear
> the rest of the window.  So this would need special tricky code that
> will complicate the display engine, something I'd like to avoid.

No, that was backwards: whatever we decide to do for the first line
beyond EOB will automatically happen for all the other lines till the
end of the window.  Avoiding that, i.e. displaying the special
character or whatever only on the first line beyond EOB, is what would
take tricky code.

Still, I stand by my opinion: displaying a line-number field beyond
EOB with non-default background color looked ugly in my experiments,
which led me to the current implementation that uses the default
face.  That is the most important issue people need to consider when
they ask to revert that decision.  I would even urge people who want
that to modify Emacs locally to use the line-number face and run with
it for some time (using a non-default background color), to see
whether the results are satisfactory.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Just to mention another related issue a reddit user pointed out: in an
empty buffer the cursor starts at a weird position because of the
"invisible" line number indicator in the EOB line. Possible
improvements on this: use the line number face with a non default bg
color and/or add a special char to the EOB line indicator.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Well there wasn't a lot of activity in the polls and you already have
read them, Eli, so I don't know, it doesn't seem to be interest on
fixing this nor consensus about how to do it.

The only alternative that wouldn't bother anyone (except for you and
other maintainers) is to add a new face for the inactive part of the
line number display that inherits from the default face. But you're
not willing to do that AFAICS so I would rather just close this and
live with the issue. Is it ok?

On Tue, Aug 7, 2018 at 2:12 PM, Carlos Pita <carlosjosepita@gmail.com> wrote:
> Just to mention another related issue a reddit user pointed out: in an
> empty buffer the cursor starts at a weird position because of the
> "invisible" line number indicator in the EOB line. Possible
> improvements on this: use the line number face with a non default bg
> color and/or add a special char to the EOB line indicator.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Fri, 31 Aug 2018 09:10:59 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> Well there wasn't a lot of activity in the polls and you already have
> read them, Eli, so I don't know, it doesn't seem to be interest on
> fixing this nor consensus about how to do it.
> 
> The only alternative that wouldn't bother anyone (except for you and
> other maintainers) is to add a new face for the inactive part of the
> line number display that inherits from the default face. But you're
> not willing to do that AFAICS so I would rather just close this and
> live with the issue. Is it ok?

Maybe we should just make the change and see if someone hollers.
Would you like me to send you a patch, and run with it for a couple of
weeks, in case there are some unintended consequences?  If not, I will
apply the change to the emacs-26 branch.

Thanks.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

>
>
> Would you like me to send you a patch, and run with it for a couple of
> weeks, in case there are some unintended consequences?
>

Sure, it would be great, thanks!

>

[-- Attachment #2: Type: text/html, Size: 571 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 1 Sep 2018 15:50:27 +0100
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
>  Would you like me to send you a patch, and run with it for a couple of
>  weeks, in case there are some unintended consequences?
> 
> Sure, it would be great, thanks!

Sorry for the delay, the patch, relative to the emacs-26 branch, is
below.  Please come back with your observations in a couple of weeks
(or earlier, if you discover some unexpected issues), and we will see
how to proceed with this.

One of the side effects of keeping the line-number face beyond EOB is
that the cursor on the EOB line will be smaller/larger than usual when
the line-number face has a non-default size.  Please tell whether you
see this is as a problem worth trying to fix (it isn't trivial).  Note
that similar effects happen in Emacs elsewhere, so this is not the
only situation where the cursor height is different from the default.

Thanks.

diff --git a/src/xdisp.c b/src/xdisp.c
index eccefa4..16b9261 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21175,14 +21175,11 @@ maybe_produce_line_number (struct it *it)
   for (const char *p = lnum_buf; *p; p++)
     {
       /* For continuation lines and lines after ZV, instead of a line
-	 number, produce a blank prefix of the same width.  Use the
-	 default face for the blank field beyond ZV.  */
-      if (beyond_zv)
-	tem_it.face_id = it->base_face_id;
-      else if (lnum_face_id != current_lnum_face_id
-	       && (EQ (Vdisplay_line_numbers, Qvisual)
-		   ? this_line == 0
-		   : this_line == it->pt_lnum))
+	 number, produce a blank prefix of the same width.  */
+      if (lnum_face_id != current_lnum_face_id
+	  && (EQ (Vdisplay_line_numbers, Qvisual)
+	      ? this_line == 0
+	      : this_line == it->pt_lnum))
 	tem_it.face_id = current_lnum_face_id;
       else
 	tem_it.face_id = lnum_face_id;

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Hi Eli,

some feedback: one problem I find with this approach is that, when
leaving the buffer, the filled up square cursor turns into an empty
box and then, since the face is smaller, it becomes apparent that it's
being cropped at the top and at the bottom (it looks like two vertical
lines, not one box). Isn't it possible to use the smaller font just
for the beginning of the line, as it is for the lines above EOB?

Thanks!

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

> Isn't it possible to use the smaller font just
> for the beginning of the line, as it is for the lines above EOB?

I mean, you're already computing the width as if the numbers would go
down up to the bottom of the window and then filling them with spaces
instead of numbers when below EOB, it seems to me that treating
below-EOB lines the same way than above-EOB ones (in the sense of
limiting the display number face only to the first columns of the
buffer) isn't that removed from the current algorithm, although from a
cursory look I recognize I'm no quite following the code.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Tue, 25 Sep 2018 16:27:31 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> some feedback: one problem I find with this approach is that, when
> leaving the buffer, the filled up square cursor turns into an empty
> box and then, since the face is smaller, it becomes apparent that it's
> being cropped at the top and at the bottom (it looks like two vertical
> lines, not one box). Isn't it possible to use the smaller font just
> for the beginning of the line, as it is for the lines above EOB?

Not easily, because Emacs generally uses the face of the last
character drawn for the cursor.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

> Not easily, because Emacs generally uses the face of the last
> character drawn for the cursor.

Ahh, ok, I see the problem.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Tue, 25 Sep 2018 16:40:43 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> I mean, you're already computing the width as if the numbers would go
> down up to the bottom of the window and then filling them with spaces
> instead of numbers when below EOB, it seems to me that treating
> below-EOB lines the same way than above-EOB ones (in the sense of
> limiting the display number face only to the first columns of the
> buffer) isn't that removed from the current algorithm, although from a
> cursory look I recognize I'm no quite following the code.

I'm confused: you were talking about the cursor, not about characters,
right?  The problem is that an empty line at EOB doesn't have any
characters to determine the height of the line, and the height then
comes from the line-number display, which is lower.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

> I'm confused: you were talking about the cursor, not about characters,
> right?  The problem is that an empty line at EOB doesn't have any
> characters to determine the height of the line, and the height then
> comes from the line-number display, which is lower.

Yes, I understood that after your last remark about the cursor face, I
was just assuming that the "empty line" following the EOB could just
have its own face, I was wrongly thinking about it more as a sequence
of spaces and wondering why the default face wasn't being applied to
them.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> Date: Tue, 25 Sep 2018 22:46:49 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> > some feedback: one problem I find with this approach is that, when
> > leaving the buffer, the filled up square cursor turns into an empty
> > box and then, since the face is smaller, it becomes apparent that it's
> > being cropped at the top and at the bottom (it looks like two vertical
> > lines, not one box). Isn't it possible to use the smaller font just
> > for the beginning of the line, as it is for the lines above EOB?
> 
> Not easily, because Emacs generally uses the face of the last
> character drawn for the cursor.

I may have found a solution.  Would you please apply the following
additional patch on top of the previous one, and see if it solves the
problem with the hollow cursor at EOB, and doesn't introduce any new
problems?  Please run with it for a couple of weeks before you
conclude whether any problems are left.

TIA

diff --git a/src/xdisp.c b/src/xdisp.c
index eccefa4..06a15e8 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2304,7 +2304,7 @@ get_phys_cursor_geometry (struct window *w, struct glyph_row *row,
      ascent value, lest the hollow cursor looks funny.  */
   y = w->phys_cursor.y;
   ascent = row->ascent;
-  if (row->ascent < glyph->ascent)
+  if (!row->ends_at_zv_p && row->ascent < glyph->ascent)
     {
       y -= glyph->ascent - row->ascent;
       ascent = glyph->ascent;
@@ -2314,6 +2314,7 @@ get_phys_cursor_geometry (struct window *w, struct glyph_row *row,
   h0 = min (FRAME_LINE_HEIGHT (f), row->visible_height);
 
   h = max (h0, ascent + glyph->descent);
+  h = min (h, row->height);
   h0 = min (h0, ascent + glyph->descent);
 
   y0 = WINDOW_HEADER_LINE_HEIGHT (w);

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Thanks Eli! For now your magic seems to be working wonders. I will let
you know of my experience in two weeks. Thanks again!

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 26 Sep 2018 13:16:01 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> Thanks Eli! For now your magic seems to be working wonders. I will let
> you know of my experience in two weeks. Thanks again!

One more improvement (I hope).  Please apply the below on top of
everything else.

diff --git a/src/xdisp.c b/src/xdisp.c
index eccefa4..357f0fb 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21235,23 +21238,30 @@ maybe_produce_line_number (struct it *it)
 	}
     }
 
-  /* Update IT's metrics due to glyphs produced for line numbers.  */
-  if (it->glyph_row)
+  /* Update IT's metrics due to glyphs produced for line numbers.
+     Don't do that for rows beyond ZV, to avoid displaying a cursor of
+     different dimensions there.  */
+  if (!beyond_zv)
     {
-      struct glyph_row *row = it->glyph_row;
+      if (it->glyph_row)
+	{
+	  struct glyph_row *row = it->glyph_row;
 
-      it->max_ascent = max (row->ascent, tem_it.max_ascent);
-      it->max_descent = max (row->height - row->ascent, tem_it.max_descent);
-      it->max_phys_ascent = max (row->phys_ascent, tem_it.max_phys_ascent);
-      it->max_phys_descent = max (row->phys_height - row->phys_ascent,
-				  tem_it.max_phys_descent);
-    }
-  else
-    {
-      it->max_ascent = max (it->max_ascent, tem_it.max_ascent);
-      it->max_descent = max (it->max_descent, tem_it.max_descent);
-      it->max_phys_ascent = max (it->max_phys_ascent, tem_it.max_phys_ascent);
-      it->max_phys_descent = max (it->max_phys_descent, tem_it.max_phys_descent);
+	  it->max_ascent = max (row->ascent, tem_it.max_ascent);
+	  it->max_descent = max (row->height - row->ascent, tem_it.max_descent);
+	  it->max_phys_ascent = max (row->phys_ascent, tem_it.max_phys_ascent);
+	  it->max_phys_descent = max (row->phys_height - row->phys_ascent,
+				      tem_it.max_phys_descent);
+	}
+      else
+	{
+	  it->max_ascent = max (it->max_ascent, tem_it.max_ascent);
+	  it->max_descent = max (it->max_descent, tem_it.max_descent);
+	  it->max_phys_ascent = max (it->max_phys_ascent,
+				     tem_it.max_phys_ascent);
+	  it->max_phys_descent = max (it->max_phys_descent,
+				      tem_it.max_phys_descent);
+	}
     }
 
   it->line_number_produced_p = true;

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Hi Eli, up till now I found not problem with your patch (everything
included). I did no exhaustive test but found no surprises on everyday
usage (with dlnm enabled) either. I would say you commit them. Thanks
again!

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 10 Oct 2018 15:33:49 -0300
> Cc: rms@gnu.org, 32337@debbugs.gnu.org
> 
> Hi Eli, up till now I found not problem with your patch (everything
> included). I did no exhaustive test but found no surprises on everyday
> usage (with dlnm enabled) either. I would say you commit them. Thanks
> again!

Thanks for testing.  Pushed to the emacs-26 branch, and closing the
bug.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

Resending after unarchiving, sorry.

On Tue, Feb 19, 2019 at 12:22 PM Carlos Pita <carlosjosepita@gmail.com> wrote:
>
> Hi Eli,
>
> recently I run into a strange problem that could be related to this change.
>
> It's not difficult to reproduce, you have to install the treemacs
> package, which installs the hydra package. Then run M-x treemacs and,
> in the tree buffer press ? to open the hydra. Finally look at the
> first character of the last line of the hydra buffer. Try this with
> emacs-26 HEAD and with 26.1 and compare.
>
> You can get an idea of the problem by looking at the attached screenshots.
>
> I've reported it in
> https://github.com/Alexander-Miller/treemacs/issues/360 also, but it
> probably is a bug in core emacs.
>
> Best regards
> --
> Carlos

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

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

The screenshots:

[-- Attachment #2: emacs26-branch.png --]
[-- Type: image/png, Size: 4853 bytes --]

[-- Attachment #3: emacs26-stable.png --]
[-- Type: image/png, Size: 6771 bytes --]

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Tue, 19 Feb 2019 12:22:26 -0300
> Cc: rms@gnu.org, 32337-done@debbugs.gnu.org
> 
> recently I run into a strange problem that could be related to this change.
> 
> It's not difficult to reproduce, you have to install the treemacs
> package, which installs the hydra package. Then run M-x treemacs and,
> in the tree buffer press ? to open the hydra. Finally look at the
> first character of the last line of the hydra buffer. Try this with
> emacs-26 HEAD and with 26.1 and compare.
> 
> You can get an idea of the problem by looking at the attached screenshots.

I have hard time believing that this has anything to do with the fix
for bug#32337.  AFAIU, there are no line-numbers shown in the
situation you describe and show on the screenshots, right?  The
problem is that the first character on the last line is displayed
using a different (incorrect) face, right?

> I've reported it in
> https://github.com/Alexander-Miller/treemacs/issues/360 also, but it
> probably is a bug in core emacs.

Would it be possible for you to bisect the emacs-26 branch and find
the commit which caused this?  That'd be most appreciated.

Thanks.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

> Would it be possible for you to bisect the emacs-26 branch and find
> the commit which caused this?  That'd be most appreciated.

Not the most exciting proposition but ok, I'll get back to you on that
in logarithmic time.

bug#32337: 26.1; display-line-numbers-mode makes cursor ill-aligned when number face is changed

[Carlos Pita]

The problem seems to be caused by commit
71c92d89137b7fdde6c2bd4bed9b8dfda5fa53dd

See also https://github.com/Alexander-Miller/treemacs/issues/360

I'm closing this again since it's indeed not related to patch applied here.