Re: Native line numbers landed on master

[Alex <agrambot@gmail.com>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Mon, 10 Jul 2017 14:31:46 -0600
>> 
>> 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.

Well, it seems this cus-edit change doesn't fix it by itself. When
executing customize-group without display-line-numbers loaded, then the
lisp-level defcustoms aren't shown. It might be because there's a
missing entry in cus-load.el, but I don't know how to add a group to it.
linum, for instance, appears to automatically be added to it.

Would it be better to skip creating a new defgroup and have all
variables just be in the display group, or are you in favour of a new
group just for line numbers?

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

Right, I was referring to the defcustoms.

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

As is stands, the contents of display-line-numbers.el only applies to
the minor modes and not display-line-numbers itself. So the user would
have to call one of the minor modes to use the features, and at that
point everything will be loaded.

I thought about autoloading the defcustoms, but it seems as if that's a
contentious decision[1].

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

So even a single count-lines on the whole buffer is too much?

>> Does the problem affect display-line-numbers?
>
> I don't think so, but it should be easy to test.  I'll take a look.

If so, then I'll leave it in.

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

Thanks, though it seems like your copyedit commit introduced a couple
typos. It also doesn't mention that any other non-nil value is treated
as t.

I've attached an updated patch. I didn't update the commit message yet.


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

From d0e76628503adc7fd6a4a41c61954f305d6f2ac5 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sun, 9 Jul 2017 16:40:52 -0600
Subject: [PATCH] Add minor mode interface for display-line-numbers

* lisp/display-line-numbers.el: New file.
* lisp/menu-bar (menu-bar-showhide-menu): Use global minor mode.
(toggle-display-line-numbers): Remove.
---
 lisp/cus-edit.el             |  4 ++
 lisp/cus-start.el            |  8 ++--
 lisp/display-line-numbers.el | 99 ++++++++++++++++++++++++++++++++++++++++++++
 lisp/menu-bar.el             | 14 ++-----
 4 files changed, 110 insertions(+), 15 deletions(-)
 create mode 100644 lisp/display-line-numbers.el

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 6dbb45ec6b..6dde2faf92 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -380,6 +380,10 @@ display
   "How characters are displayed in buffers."
   :group 'environment)
 
+(defgroup display-line-numbers nil
+  "Display line numbers in the buffer."
+  :group 'display)
+
 (defgroup execute nil
   "Executing external commands."
   :group 'processes)
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index e0290395ad..117b336a65 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -584,7 +584,7 @@ minibuffer-prompt-properties--setter
 		       (const :tag "Grow only" :value grow-only))
 	      "25.1")
 	     (display-raw-bytes-as-hex display boolean "26.1")
-             (display-line-numbers display
+             (display-line-numbers display-line-numbers
                                    (choice
                                     (const :tag "Off (nil)" :value nil)
                                     (const :tag "Absolute line numbers"
@@ -594,7 +594,7 @@ minibuffer-prompt-properties--setter
                                     (const :tag "Visually relative line numbers"
                                            :value visual))
                                    "26.1")
-             (display-line-number-width display
+             (display-line-number-width display-line-numbers
                                  (choice
                                   (const :tag "Dynamically computed"
                                          :value nil)
@@ -602,14 +602,14 @@ minibuffer-prompt-properties--setter
                                            :value 2
                                            :format "%v"))
                                  "26.1")
-             (display-line-numbers-current-absolute display
+             (display-line-numbers-current-absolute display-line-numbers
                                  (choice
                                   (const :tag "Display actual number of current line"
                                          :value t)
                                   (const :tag "Display zero as number of current line"
                                          :value nil))
                                  "26.1")
-             (display-line-numbers-widen display
+             (display-line-numbers-widen display-line-numbers
                                  (choice
                                   (const :tag "Disregard narrowing when calculating line numbers"
                                          :value t)
diff --git a/lisp/display-line-numbers.el b/lisp/display-line-numbers.el
new file mode 100644
index 0000000000..cca537a17b
--- /dev/null
+++ b/lisp/display-line-numbers.el
@@ -0,0 +1,99 @@
+;;; display-line-numbers.el --- interface for display-line-numbers -*- lexical-binding: t -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; Maintainer: emacs-devel@gnu.org
+;; Keywords: convenience
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Provides a minor mode interface for `display-line-numbers'.
+;;
+;; Toggle display of line numbers with M-x display-line-numbers-mode.
+;; To enable line numbering in all buffers, use M-x
+;; global-display-line-numbers-mode.  To change the default type of
+;; line numbers displayed, customize display-line-number-type.
+
+;; NOTE: Customization variables for `display-line-numbers' itself are
+;; defined in cus-start.el.
+
+;;; Code:
+
+(defcustom display-line-number-type t
+  "Sets the type of line numbers to use in `display-line-number-mode'.
+See `display-line-numbers' for value options."
+  :group 'display-line-numbers
+  :type '(choice (const :tag "Relative line numbers" relative)
+                 (const :tag "Relative visual line numbers" visual)
+                 (other :tag "Absolute line numbers" t))
+  :version "26.1")
+
+(defcustom display-line-numbers-grow-only nil
+  "If non-nil, do not shrink line number width."
+  :group 'display-line-numbers
+  :type 'boolean
+  :version "26.1")
+
+(defcustom display-line-number-width-start nil
+  "If non-nil, count number of lines to use for line number width.
+When `display-line-number-mode' is turned on,
+`display-line-number-width' is set to the minimum width necessary
+to display all line numbers in the buffer."
+  :group 'display-line-numbers
+  :type 'boolean
+  :version "26.1")
+
+(defun display-line-numbers-update-width ()
+  "Prevent the line number width from shrinking."
+  (let ((width (line-number-display-width)))
+    (when (> width (or display-line-number-width 1))
+      (setq display-line-number-width width))))
+
+;;;###autoload
+(define-minor-mode display-line-numbers-mode
+  "Toggle display of line numbers in the buffer.
+This uses `display-line-numbers' internally.
+
+To change the type of line numbers displayed by default,
+customize `display-line-number-type'.  To change the type while
+the mode is on, set `display-line-numbers' directly."
+  :lighter nil
+  (if display-line-numbers-mode
+      (progn
+        (when display-line-number-width-start
+          (setq display-line-number-width
+                (length (number-to-string
+                         (count-lines (point-min) (point-max))))))
+        (when display-line-numbers-grow-only
+          (add-hook 'pre-command-hook #'display-line-numbers-update-width nil t))
+        (setq display-line-numbers display-line-number-type))
+    (remove-hook 'pre-command-hook #'display-line-numbers-update-width t)
+    (setq display-line-numbers nil)))
+
+;;;###autoload
+(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))))
+
+(provide 'display-line-numbers)
+
+;;; display-line-numbers.el ends here
diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index 3ca7d1b5b3..434dd9b91f 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -1101,23 +1101,15 @@ menu-bar-showhide-tool-bar-menu-customize-enable-bottom
                     :button (:radio . (eq tool-bar-mode nil))))
       menu)))
 
-(defun toggle-display-line-numbers ()
-  (interactive)
-  (if display-line-numbers
-      (setq display-line-numbers nil)
-    (setq display-line-numbers t))
-  (force-mode-line-update))
-
 (defvar menu-bar-showhide-menu
   (let ((menu (make-sparse-keymap "Show/Hide")))
 
     (bindings--define-key menu [display-line-numbers]
       `(menu-item "Line Numbers for All Lines"
-                  ,(lambda ()
-                     (interactive)
-                     (toggle-display-line-numbers))
+                  global-display-line-numbers-mode
                   :help "Show the line number alongside each line"
-                  :button (:toggle . display-line-numbers)))
+                  :button (:toggle . (bound-and-true-p
+                                      global-display-line-numbers-mode))))
 
     (bindings--define-key menu [column-number-mode]
       (menu-bar-make-mm-toggle column-number-mode
-- 
2.13.2


[-- Attachment #3: Type: text/plain, Size: 59 bytes --]


Footnotes: 
[1]  https://emacs.stackexchange.com/a/32860