Re: Native line numbers landed on master

[Alex <agrambot@gmail.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Sun, 09 Jul 2017 16:56:23 -0600
>> 
>> I've attached a patch below. I changed the menu bar to toggle the global
>> mode since the other toggles in the Show/Hide menu are also toggled
>> globally. Does it look alright for inclusion?
>
> I have some comments:
>
>> +(defgroup display-line-numbers nil
>> +  "Display line numbers in the buffer."
>> +  :group 'display)
>
> This means the defcustoms here will be separate from those defined in
> cus-start.el.  Is that intended?
>
> More generally, do we want some of the defcustoms to live here and
> some in another file?  And if we want all of them here, does that mean
> this file needs to be preloaded, or at least auto-loaded when
> display-line-numbers is set by the user?

I'm not sure if there's a convention for this (built-in feature with a
Lisp-level mode wrapper) situation, but it might be nice to separate the
variables that are directly linked to display-line-numbers and ones that
only are used in the minor mode wrapper.

What about defining the defgroup in cus-edit.el, making both these
variables and the ones in cus-start belong to it?

I don't know if the file should be pre/auto-loaded. Is there a reason to
do so considering linum.el isn't?

>> +(define-globalized-minor-mode global-display-line-numbers-mode
>> +  display-line-numbers-mode
>> +  (lambda ()
>> +    (unless (or (minibufferp)
>> +                ;; taken from linum.el
>> +                (and (daemonp) (null (frame-parameter nil 'client))))
>> +      (display-line-numbers-mode))))
>
> The daemonp part is only needed when display-line-number-width-start
> is non-nil, right?

I suppose so, but would one want line numbers in that specific buffer
either way? I added that part because you added it to linum.el in
bd3c6eec. Does the problem affect display-line-numbers?

Also, I was wondering if setting display-line-number-width in
pre-command-hook unconditionally is a good idea. I timed it and the
function itself seemed slightly faster than a let/when approach, but
describe-variable states that setting it calls set-buffer-redisplay,
which disables redisplay optimizations. So if I understand this
correctly, adding the current display-line-numbers-update-width to
pre-command-hook would disable redisplay optimizations for every
command.

P.S. I also noticed that the docstring for display-line-numbers doesn't
describe the 'relative value, or state that 'visual also uses relative
line numbers.