Native line numbers landed on master

classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Native line numbers landed on master

Eli Zaretskii
I've merged the line-numbers branch into master.  Please report any
problems you see as result of this via 'report-emacs-bug', as usual.

Thanks to everyone who tested the branch and provided feedback.

I will keep the branch in the repository for a week or so, before
deleting it, to let people who tracked the branch switch to master in
their own free time.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

martin rudalics
 > I've merged the line-numbers branch into master.  Please report any
 > problems you see as result of this via 'report-emacs-bug', as usual.

One small detail: Menu entries are usually capitalized.  Please consider
doing this for "all lines" too.

Many thanks for the great work, martin

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
> Date: Sat, 08 Jul 2017 10:41:20 +0200
> From: martin rudalics <[hidden email]>
>
>  > I've merged the line-numbers branch into master.  Please report any
>  > problems you see as result of this via 'report-emacs-bug', as usual.
>
> One small detail: Menu entries are usually capitalized.  Please consider
> doing this for "all lines" too.

Done, thanks for the suggestion.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Alex-2
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

> I've merged the line-numbers branch into master.  Please report any
> problems you see as result of this via 'report-emacs-bug', as usual.
>
> Thanks to everyone who tested the branch and provided feedback.
>
> I will keep the branch in the repository for a week or so, before
> deleting it, to let people who tracked the branch switch to master in
> their own free time.

Why did you choose the name line-number-display-width over, e.g.,
display-line-number-width? It seems every other related variable has a
prefix of display-. I can see the name being confused with
line-number-display-limit-width and line-number-display-limit.

P.S. I was thinking about how a minor mode for this should be
implemented. I attached a proof of concept, which includes a couple
extra variables to achieve line number width behaviour similar to
linum/nlinum. What do you think?


display-line-numbers.el (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
> From: Alex <[hidden email]>
> Cc: [hidden email]
> Date: Sat, 08 Jul 2017 16:38:44 -0600
>
> Why did you choose the name line-number-display-width over, e.g.,
> display-line-number-width? It seems every other related variable has a
> prefix of display-.

Not every other one: the faces start with line-number.

Is the name that bad?  It sounded right at the time, but if others
think it might confuse, we could rename it.

> P.S. I was thinking about how a minor mode for this should be
> implemented. I attached a proof of concept, which includes a couple
> extra variables to achieve line number width behaviour similar to
> linum/nlinum. What do you think?

Looks okay, although I'd drop the # part in the below:

  (add-hook 'pre-command-hook #'display-line-numbers-update-width nil t))

Thanks.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Alex-2
Eli Zaretskii <[hidden email]> writes:

>> From: Alex <[hidden email]>
>> Cc: [hidden email]
>> Date: Sat, 08 Jul 2017 16:38:44 -0600
>>
>> Why did you choose the name line-number-display-width over, e.g.,
>> display-line-number-width? It seems every other related variable has a
>> prefix of display-.
>
> Not every other one: the faces start with line-number.
>
> Is the name that bad?  It sounded right at the time, but if others
> think it might confuse, we could rename it.
I think it's a little confusing, but it's not a huge deal.

>> P.S. I was thinking about how a minor mode for this should be
>> implemented. I attached a proof of concept, which includes a couple
>> extra variables to achieve line number width behaviour similar to
>> linum/nlinum. What do you think?
>
> Looks okay, although I'd drop the # part in the below:
>
>   (add-hook 'pre-command-hook #'display-line-numbers-update-width nil t))

Is there a reason to drop it? I heard that the byte compiler uses #' to
check for existence of the function (and warns if it's not found), so it
seems like a nice default in these situations.

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?


0001-Add-minor-mode-interface-for-display-line-numbers.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Filipe Silva
In reply to this post by Eli Zaretskii
Thank you for your quality work on this feature Eli. It will be a huge success for the emacs community.

Filipe

On Jul 8, 2017 04:59, "Eli Zaretskii" <[hidden email]> wrote:
I've merged the line-numbers branch into master.  Please report any
problems you see as result of this via 'report-emacs-bug', as usual.

Thanks to everyone who tested the branch and provided feedback.

I will keep the branch in the repository for a week or so, before
deleting it, to let people who tracked the branch switch to master in
their own free time.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
In reply to this post by Alex-2
> From: Alex <[hidden email]>
> Cc: [hidden email]
> 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?

> +(defcustom display-line-number-inhibit-shrink nil

Instead of "inhibit-shrink" (which is a kind-of double negation), I
would suggest to use "grow-only", which AFAIR was your original
suggestion.

> +(defcustom display-line-number-width-start nil
> +  "If non-nil, set initial line number width based on buffer contents.

"Based on buffer contents" is a euphemism.  I would suggest to tell
explicitly that this will cause the lines counted in the buffer when
it's created.

> +To change the type of line numbers displayed by default,
> +customize `display-line-number-type'. To change the type while
                                       ^^
Two spaces, please.

> +(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?

Thanks.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Alex-2
Eli Zaretskii <[hidden email]> writes:

>> From: Alex <[hidden email]>
>> Cc: [hidden email]
>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

John Wiegley
In reply to this post by Eli Zaretskii
>>>>> "EZ" == Eli Zaretskii <[hidden email]> writes:

>> Why did you choose the name line-number-display-width over, e.g.,
>> display-line-number-width? It seems every other related variable has a
>> prefix of display-.

EZ> Not every other one: the faces start with line-number.

If the defgroup is display-line-numbers, shouldn't all customization options
use that prefix?

--
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
> From: John Wiegley <[hidden email]>
> Cc: Alex <[hidden email]>,  [hidden email]
> Date: Mon, 10 Jul 2017 15:19:18 -0700
>
> >>>>> "EZ" == Eli Zaretskii <[hidden email]> writes:
>
> >> Why did you choose the name line-number-display-width over, e.g.,
> >> display-line-number-width? It seems every other related variable has a
> >> prefix of display-.
>
> EZ> Not every other one: the faces start with line-number.
>
> If the defgroup is display-line-numbers, shouldn't all customization options
> use that prefix?

They should, and they do, AFAICT.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

> I've merged the line-numbers branch into master.  Please report any
> problems you see as result of this via 'report-emacs-bug', as usual.
>

Hi Eli,

I'm not sure if this is a problem or intended design:

If I customize line-number-current-line to have a background colour,
then when the current line is wrapped, the current line number and the
empty space(s) below it have that backround applied. I was hoping to
have that apply just to the line number itself.

In any case, it's a minor issue, I've removed nlinum from my
setup. Thanks for your work on this Eli.

Regards

Robert


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Jean-Christophe Helary
> Eli Zaretskii <[hidden email]> writes:
>
>> I've merged the line-numbers branch into master.  Please report any
>> problems you see as result of this via 'report-emacs-bug', as usual.

I'm pretty sure I'm building master but I don't see how to make the line numbers appear.

Jean-Christophe
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Robert Pluim
Jean-Christophe Helary <[hidden email]> writes:

>> Eli Zaretskii <[hidden email]> writes:
>>
>>> I've merged the line-numbers branch into master.  Please report any
>>> problems you see as result of this via 'report-emacs-bug', as usual.
>
> I'm pretty sure I'm building master but I don't see how to make the line numbers appear.

From NEWS:

** Emacs now supports optional display of line numbers in the buffer.
This is similar to what linum-mode provides, but much faster and
doesn't usurp the display margin for the line numbers.  Customize the
buffer-local variable 'display-line-numbers' to activate this optional
display.

If you don't have that, you don't have master :-)

Regards

Robert


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Jean-Christophe Helary

> On Jul 11, 2017, at 22:47, Robert Pluim <[hidden email]> wrote:
>
>> I'm pretty sure I'm building master but I don't see how to make the line numbers appear.
>
> From NEWS:
>
> ** Emacs now supports optional display of line numbers in the buffer.
> This is similar to what linum-mode provides, but much faster and
> doesn't usurp the display margin for the line numbers.  Customize the
> buffer-local variable 'display-line-numbers' to activate this optional
> display.
>
> If you don't have that, you don't have master :-)

I was not looking at the right place :) Thank you.
I'll have NEWS popup each time I build master from now on... :)

And, wow, that's amazing.

Jean-Christophe
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
In reply to this post by Alex-2
> From: Alex <[hidden email]>
> Cc: [hidden email]
> Date: Mon, 10 Jul 2017 14:31:46 -0600
>
> >> +(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?

Sounds OK to me, thanks.  I think we do want all the variables to
appear in the same customization group, even if they are defined
separately.

Maybe also add a comment in display-line-numbers.el that some
additional defcustoms are defined in cus-start.el.

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

linum.el _is_ autoloaded if you invoke linum-mode, no?

The scenario I had in mind was a user doing this:

  M-x set-variable RET display-line-numbers RET t RET

This is a legitimate way of activating the feature in a buffer.  Do we
want then the user to automatically have access to all the
customizations and features in display-line-numbers.el?

> >> +(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 don't know.  Similarity to interactive invocation, maybe?

> I added that part because you added it to linum.el in bd3c6eec.

That was to fix a bug that I think shouldn't happen with the native
implementation, because it doesn't count lines.

> Does the problem affect display-line-numbers?

I don't think so, but it should be easy to test.  I'll take a look.

> 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.

Yes, it shouldn't be unconditional.

> 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.

Thanks, I fixed this.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Date: Tue, 11 Jul 2017 15:00:55 +0200
>
> I'm not sure if this is a problem or intended design:

Not consciously intended, just the "natural" thing.

> If I customize line-number-current-line to have a background colour,
> then when the current line is wrapped, the current line number and the
> empty space(s) below it have that backround applied. I was hoping to
> have that apply just to the line number itself.

Are you talking about display-line-numbers set to t, and then about
the empty field displayed for continuation lines, which show no
numbers?  Otherwise I don't understand what you mean by "empty
space(s) below it".

If you indeed mean continuation lines, I'd like to hear from others
what they think about this use case.  Displaying the empty fields in a
face different from the number itself is some work, but I will do it
if that's the "popular demand".

> In any case, it's a minor issue, I've removed nlinum from my
> setup. Thanks for your work on this Eli.

You are welcome, and thanks for the feedback.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Stefan Monnier
In reply to this post by Eli Zaretskii
> Looks okay, although I'd drop the # part in the below:
>   (add-hook 'pre-command-hook #'display-line-numbers-update-width nil t))

Recent Elisp changes have rather gone the other way (adding a # rather
than removing them).  Part of the reason is that lexical-binding gives
different semantics to '(lambda ...) and #'(lambda ...), other part is
that cl-flet and cl-labels also give different semantics to 'foo and
#'foo.

Finally, if you keep the # part, the byte-compiler will be able to check
that you spelled the function correctly (and warn you if that function
is not known to exist).


        Stefan


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Date: Tue, 11 Jul 2017 15:00:55 +0200
>>
>> If I customize line-number-current-line to have a background colour,
>> then when the current line is wrapped, the current line number and the
>> empty space(s) below it have that backround applied. I was hoping to
>> have that apply just to the line number itself.
>
> Are you talking about display-line-numbers set to t, and then about
> the empty field displayed for continuation lines, which show no
> numbers?  Otherwise I don't understand what you mean by "empty
> space(s) below it".

Yes, I mean the empty field.

> If you indeed mean continuation lines, I'd like to hear from others
> what they think about this use case.  Displaying the empty fields in a
> face different from the number itself is some work, but I will do it
> if that's the "popular demand".

I can live with it, it just looks a little jarring when there are
continuation lines.

Regards

Robert


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Native line numbers landed on master

Eli Zaretskii
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Date: Tue, 11 Jul 2017 11:27:18 -0400
>
> > Looks okay, although I'd drop the # part in the below:
> >   (add-hook 'pre-command-hook #'display-line-numbers-update-width nil t))
>
> Recent Elisp changes have rather gone the other way (adding a # rather
> than removing them).  Part of the reason is that lexical-binding gives
> different semantics to '(lambda ...) and #'(lambda ...), other part is
> that cl-flet and cl-labels also give different semantics to 'foo and
> #'foo.
>
> Finally, if you keep the # part, the byte-compiler will be able to check
> that you spelled the function correctly (and warn you if that function
> is not known to exist).

Without this being documented anywhere (AFAICS), how can we ever hope
to educate our users (starting with myself) about this?

123
Loading...