bug#6871: Please make linum-mode per buffer, not per major mode

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

Line numbers should work even in multi major mode buffers. Please add
the following lines to linum.el:

(put 'linum-overlays  'permanent-local t)
(put 'linum-available 'permanent-local t)
(put 'linum-mode      'permanent-local t)

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

> Line numbers should work even in multi major mode buffers. Please add
> the following lines to linum.el:

> (put 'linum-overlays  'permanent-local t)
> (put 'linum-available 'permanent-local t)
> (put 'linum-mode      'permanent-local t)

I don't think that's going to be sufficient: you'll probably at least
also want to remove the

        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)

I.e. you can't just make some vars permanent-local, you have to adjust
the major mode from "mode-specific" to "permanent-local".


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Tue, Aug 17, 2010 at 10:59 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> Line numbers should work even in multi major mode buffers. Please add
>> the following lines to linum.el:
>
>> (put 'linum-overlays  'permanent-local t)
>> (put 'linum-available 'permanent-local t)
>> (put 'linum-mode      'permanent-local t)
>
> I don't think that's going to be sufficient: you'll probably at least
> also want to remove the
>
>        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
>
> I.e. you can't just make some vars permanent-local, you have to adjust
> the major mode from "mode-specific" to "permanent-local".

Yes, I know. I have added code to take careof this for linum in
mumamo.el. But we do not yet have code in Emacs for taking care of
this more generally.

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

>>> Line numbers should work even in multi major mode buffers. Please add
>>> the following lines to linum.el:
>> 
>>> (put 'linum-overlays  'permanent-local t)
>>> (put 'linum-available 'permanent-local t)
>>> (put 'linum-mode      'permanent-local t)
>> 
>> I don't think that's going to be sufficient: you'll probably at least
>> also want to remove the
>> 
>>        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
>> 
>> I.e. you can't just make some vars permanent-local, you have to adjust
>> the major mode from "mode-specific" to "permanent-local".

> Yes, I know. I have added code to take careof this for linum in
> mumamo.el. But we do not yet have code in Emacs for taking care of
> this more generally.

But your report is about fixing it in linum.el (where it belongs), which
means to turn linum-mode from a major-mode-specific minor-mode to
a permanent-local major-mode.  I think such a change is acceptable
(although the motivation of multi-major-mode buffers might not be that
compelling, since it tends to lead to the idea that most/all
minor-modes should be made permanent-local), but we need a proper patch
for it.


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Tue, Aug 17, 2010 at 2:11 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>>>> Line numbers should work even in multi major mode buffers. Please add
>>>> the following lines to linum.el:
>>>
>>>> (put 'linum-overlays  'permanent-local t)
>>>> (put 'linum-available 'permanent-local t)
>>>> (put 'linum-mode      'permanent-local t)
>>>
>>> I don't think that's going to be sufficient: you'll probably at least
>>> also want to remove the
>>>
>>>        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
>>>
>>> I.e. you can't just make some vars permanent-local, you have to adjust
>>> the major mode from "mode-specific" to "permanent-local".
>
>> Yes, I know. I have added code to take careof this for linum in
>> mumamo.el. But we do not yet have code in Emacs for taking care of
>> this more generally.
>
> But your report is about fixing it in linum.el (where it belongs), which
> means to turn linum-mode from a major-mode-specific minor-mode to
> a permanent-local major-mode.  I think such a change is acceptable
> (although the motivation of multi-major-mode buffers might not be that
> compelling, since it tends to lead to the idea that most/all
> minor-modes should be made permanent-local), but we need a proper patch
> for it.

How about also removing adding linum to change-major-mode-hook?

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

>>>> I don't think that's going to be sufficient: you'll probably at least
>>>> also want to remove the
>>>>        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
[...]
>> minor-modes should be made permanent-local), but we need a proper patch
>> for it.
> How about also removing adding linum to change-major-mode-hook?

Yes, that's what I suggested, isn't it?
Please write a patch, test it (after removing the workarounds you
currently use), and send it here.


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

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

On Wed, Aug 18, 2010 at 9:19 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>>>>> I don't think that's going to be sufficient: you'll probably at least
>>>>> also want to remove the
>>>>>        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
> [...]
>>> minor-modes should be made permanent-local), but we need a proper patch
>>> for it.
>> How about also removing adding linum to change-major-mode-hook?
>
> Yes, that's what I suggested, isn't it?

I was not quite sure.

> Please write a patch, test it (after removing the workarounds you
> currently use), and send it here.


Attached the patch. There are some other changes to linum.el too.

The handling in after-change-functions was not optimal. Maybe the doc
string for these hook should explain a bit more about how to use it?

[-- Attachment #2: linum-multi-major.diff --]
[-- Type: application/octet-stream, Size: 9007 bytes --]

;;; linum.el --- display line numbers in the left margin

;; Copyright (C) 2008, 2009, 2010 Free Software Foundation, Inc.

;; Author: Markus Triska <markus.triska@gmx.at>
;; Maintainer: FSF
;; 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:

;; Display line numbers for the current buffer.
;;
;; Toggle display of line numbers with M-x linum-mode.  To enable
;; line numbering in all buffers, use M-x global-linum-mode.

;;; Code:

(defconst linum-version "0.9x")

(defvar linum-overlays nil "Overlays used in this buffer.")
(defvar linum-available nil "Overlays available for reuse.")
(defvar linum-before-numbering-hook nil
  "Functions run in each buffer before line numbering starts.")

(mapc #'make-variable-buffer-local '(linum-overlays linum-available))
(put 'linum-overlays  'permanent-local t)
(put 'linum-available 'permanent-local t)

(defgroup linum nil
  "Show line numbers in the left margin."
  :group 'convenience)

;;;###autoload
(defcustom linum-format 'dynamic
  "Format used to display line numbers.
Either a format string like \"%7d\", `dynamic' to adapt the width
as needed, or a function that is called with a line number as its
argument and should evaluate to a string to be shown on that line.
See also `linum-before-numbering-hook'."
  :group 'linum
  :type 'sexp)

(defface linum
  '((t :inherit (shadow default)))
  "Face for displaying line numbers in the display margin."
  :group 'linum)

(defcustom linum-delay nil
  "Delay updates to give Emacs a chance for other changes."
  :group 'linum
  :type 'boolean)

;;;###autoload
(define-minor-mode linum-mode
  "Toggle display of line numbers in the left margin."
  :lighter ""                           ; for desktop.el
  (if linum-mode
      (progn
        (add-hook 'post-command-hook (if linum-delay
                                         'linum-schedule
                                       'linum-update-current-buffer) nil t)
        (add-hook 'before-change-functions 'linum-before-change nil t)
        (add-hook 'after-change-functions 'linum-after-change nil t)
        (add-hook 'window-scroll-functions 'linum-after-scroll nil t)
        ;; Using both window-size-change-functions and
        ;; window-configuration-change-hook seems redundant. --Stef
        ;; (add-hook 'window-size-change-functions 'linum-after-size nil t)
        (add-hook 'window-configuration-change-hook
                  ;; Update just the selected window
                  'linum-update-selected-window nil t)
        (setq linum-change-beg 1)
        (linum-update-current-buffer))
    (remove-hook 'post-command-hook 'linum-update-current-buffer t)
    (remove-hook 'post-command-hook 'linum-schedule t)
    ;; (remove-hook 'window-size-change-functions 'linum-after-size t)
    (remove-hook 'window-scroll-functions 'linum-after-scroll t)
    (remove-hook 'before-change-functions 'linum-before-change t)
    (remove-hook 'after-change-functions 'linum-after-change t)
    (remove-hook 'window-configuration-change-hook 'linum-update-selected-window t)
    (linum-delete-overlays)))
(put 'linum-mode      'permanent-local t)

;;;###autoload
(define-globalized-minor-mode global-linum-mode linum-mode linum-on)

(defun linum-on ()
  (unless (minibufferp)
    (linum-mode 1)))

(defun linum-delete-overlays ()
  "Delete all overlays displaying line numbers for this buffer."
  (mapc #'delete-overlay linum-overlays)
  (setq linum-overlays nil)
  (dolist (w (get-buffer-window-list (current-buffer) nil t))
    (set-window-margins w 0 (cdr (window-margins w)))))

(defun linum-update-current-buffer ()
  "Update line numbers for the current buffer."
  (linum-update (current-buffer)))
(put 'linum-update-current-buffer 'permanent-local-hook t)

(defun linum-update (buffer)
  "Update line numbers for all windows displaying BUFFER."
  (with-current-buffer buffer
    (when (and linum-mode
               linum-change-beg)
      (setq linum-available linum-overlays)
      (setq linum-overlays nil)
      (save-excursion
        (mapc #'linum-update-window
              (get-buffer-window-list buffer nil 'visible)))
      (mapc #'delete-overlay linum-available)
      (setq linum-change-beg nil)
      (setq linum-available nil))))

(defun linum-update-selected-window ()
  "Update line numbers for the selected window."
  (let ((here (window-point))
        ;; We might have scrolled or changed win config
        (linum-change-beg 1))
    (linum-update-window (selected-window))
    (goto-char here)))
(put 'linum-update-selected-window 'permanent-local-hook t)

(defun linum-update-window (win)
  "Update line numbers for the portion visible in window WIN."
  (goto-char (window-start win))
  (let ((line (line-number-at-pos))
        (limit (window-end win t))
        (fmt (cond ((stringp linum-format) linum-format)
                   ((eq linum-format 'dynamic)
                    (let ((w (length (number-to-string
                                      (count-lines (point-min) (point-max))))))
                      (concat "%" (number-to-string w) "d")))))
        (width 0))
    (when (<= linum-change-beg limit)
      (run-hooks 'linum-before-numbering-hook)
      ;; Create an overlay (or reuse an existing one) for each
      ;; line visible in this window, if necessary.
      (while (and (not (eobp)) (<= (point) limit))
        (let* ((str (if fmt
                        (propertize (format fmt line) 'face 'linum)
                      (funcall linum-format line)))
               (visited (catch 'visited
                          (dolist (o (overlays-in (point) (point)))
                            (when (equal-including-properties
                                   (overlay-get o 'linum-str) str)
                              (unless (memq o linum-overlays)
                                (push o linum-overlays))
                              (setq linum-available (delq o linum-available))
                              (throw 'visited t))))))
          (setq width (max width (length str)))
          (unless visited
            (let ((ov (if (null linum-available)
                          (make-overlay (point) (point))
                        (move-overlay (pop linum-available) (point) (point)))))
              (push ov linum-overlays)
              (overlay-put ov 'before-string
                           (propertize " " 'display `((margin left-margin) ,str)))
              (overlay-put ov 'linum-str str))))
        ;; Text may contain those nasty intangible properties, but that
        ;; shouldn't prevent us from counting those lines.
        (let ((inhibit-point-motion-hooks t))
          (forward-line))
        (setq line (1+ line)))
      (set-window-margins win width (cdr (window-margins win))))))

(defvar linum-change-beg nil
  "Position of change beginning, recorded after change.")
(make-variable-buffer-local 'linum-change-beg)
(put linum-change-beg 'permanent-local t)

(defvar linum-changed-region-has-newline nil)
(defun linum-before-change (beg end)
  ;; Record new lines in changed region for check in after change function.
  (when (string-match-p "\n" (buffer-substring-no-properties beg end))
    (setq linum-changed-region-has-newline t)))
(put 'linum-before-change 'permanent-local-hook t)

(defun linum-after-change (beg end len)
  ;; update overlays after newlines are delete or inserted
  (when (or linum-changed-region-has-newline
            (string-match-p "\n" (buffer-substring-no-properties beg end)))
    (setq linum-changed-region-has-newline nil)
    (setq linum-change-beg beg)))
(put 'linum-after-change 'permanent-local-hook t)

(defun linum-after-scroll (win start)
  (with-selected-window win
    (linum-update-selected-window)))
(put 'linum-after-scroll 'permanent-local-hook t)

;; (defun linum-after-size (frame)
;;   (linum-after-config))

(defun linum-schedule ()
  ;; schedule an update; the delay gives Emacs a chance for display changes
  (run-with-idle-timer 0 nil #'linum-update-current-buffer))
(put 'linum-schedule 'permanent-local-hook t)

;; (defun linum-after-config ()
;;   (walk-windows (lambda (w) (linum-update (window-buffer w))) nil 'visible))

(defun linum-unload-function ()
  "Unload the Linum library."
  (global-linum-mode -1)
  ;; continue standard unloading
  nil)

(provide 'linum)

;; arch-tag: dea45631-ed3c-4867-8b49-1c41c80aec6a
;;; linum.el ends here

bug#6871: Please make linum-mode per buffer, not per major mode

[Juanma Barranquero]

On Thu, Aug 19, 2010 at 06:29, Lennart Borgman
<lennart.borgman@gmail.com> wrote:

> Attached the patch.

That is not a patch, it's a full linum.el. And there is no ChangeLog.

> There are some other changes to linum.el too.

Why? If they are not related, that should be sent as another patch.

    Juanma

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

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

On Thu, Aug 19, 2010 at 2:13 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 06:29, Lennart Borgman
> <lennart.borgman@gmail.com> wrote:
>
>> Attached the patch.
>
> That is not a patch, it's a full linum.el. And there is no ChangeLog.

Ah, shit. Thanks. I must learn to switch buffer before I save the diff....

I think I have attached the diff now ;-)


>> There are some other changes to linum.el too.
>
> Why? If they are not related, that should be sent as another patch.

Maybe. Linum is rather small and it is quicker to do it this way.

[-- Attachment #2: linum-multi-major.diff --]
[-- Type: application/octet-stream, Size: 7119 bytes --]

=== modified file 'lisp/linum.el'
--- trunk/lisp/linum.el	2010-01-13 08:35:10 +0000
+++ patched/lisp/linum.el	2010-08-19 04:22:04 +0000
@@ -38,6 +38,8 @@
   "Functions run in each buffer before line numbering starts.")
 
 (mapc #'make-variable-buffer-local '(linum-overlays linum-available))
+(put 'linum-overlays  'permanent-local t)
+(put 'linum-available 'permanent-local t)
 
 (defgroup linum nil
   "Show line numbers in the left margin."
@@ -58,13 +60,6 @@
   "Face for displaying line numbers in the display margin."
   :group 'linum)
 
-(defcustom linum-eager t
-  "Whether line numbers should be updated after each command.
-The conservative setting `nil' might miss some buffer changes,
-and you have to scroll or press \\[recenter-top-bottom] to update the numbers."
-  :group 'linum
-  :type 'boolean)
-
 (defcustom linum-delay nil
   "Delay updates to give Emacs a chance for other changes."
   :group 'linum
@@ -76,30 +71,29 @@
   :lighter ""                           ; for desktop.el
   (if linum-mode
       (progn
-        (if linum-eager
             (add-hook 'post-command-hook (if linum-delay
                                              'linum-schedule
-                                           'linum-update-current) nil t)
-          (add-hook 'after-change-functions 'linum-after-change nil t))
+                                       'linum-update-current-buffer) nil t)
+        (add-hook 'before-change-functions 'linum-before-change nil t)
+        (add-hook 'after-change-functions 'linum-after-change nil t)
         (add-hook 'window-scroll-functions 'linum-after-scroll nil t)
         ;; Using both window-size-change-functions and
         ;; window-configuration-change-hook seems redundant. --Stef
         ;; (add-hook 'window-size-change-functions 'linum-after-size nil t)
-        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
         (add-hook 'window-configuration-change-hook
-                  ;; FIXME: If the buffer is shown in N windows, this
-                  ;; will be called N times rather than once.  We should use
-                  ;; something like linum-update-window instead.
-                  'linum-update-current nil t)
-        (linum-update-current))
-    (remove-hook 'post-command-hook 'linum-update-current t)
+                  ;; Update just the selected window
+                  'linum-update-selected-window nil t)
+        (setq linum-change-beg 1)
+        (linum-update-current-buffer))
+    (remove-hook 'post-command-hook 'linum-update-current-buffer t)
     (remove-hook 'post-command-hook 'linum-schedule t)
     ;; (remove-hook 'window-size-change-functions 'linum-after-size t)
     (remove-hook 'window-scroll-functions 'linum-after-scroll t)
+    (remove-hook 'before-change-functions 'linum-before-change t)
     (remove-hook 'after-change-functions 'linum-after-change t)
-    (remove-hook 'window-configuration-change-hook 'linum-update-current t)
-    (remove-hook 'change-major-mode-hook 'linum-delete-overlays t)
+    (remove-hook 'window-configuration-change-hook 'linum-update-selected-window t)
     (linum-delete-overlays)))
+(put 'linum-mode      'permanent-local t)
 
 ;;;###autoload
 (define-globalized-minor-mode global-linum-mode linum-mode linum-on)
@@ -115,22 +109,34 @@
   (dolist (w (get-buffer-window-list (current-buffer) nil t))
     (set-window-margins w 0 (cdr (window-margins w)))))
 
-(defun linum-update-current ()
+(defun linum-update-current-buffer ()
   "Update line numbers for the current buffer."
   (linum-update (current-buffer)))
+(put 'linum-update-current-buffer 'permanent-local-hook t)
 
 (defun linum-update (buffer)
   "Update line numbers for all windows displaying BUFFER."
   (with-current-buffer buffer
-    (when linum-mode
+    (when (and linum-mode
+               linum-change-beg)
       (setq linum-available linum-overlays)
       (setq linum-overlays nil)
       (save-excursion
         (mapc #'linum-update-window
               (get-buffer-window-list buffer nil 'visible)))
       (mapc #'delete-overlay linum-available)
+      (setq linum-change-beg nil)
       (setq linum-available nil))))
 
+(defun linum-update-selected-window ()
+  "Update line numbers for the selected window."
+  (let ((here (window-point))
+        ;; We might have scrolled or changed win config
+        (linum-change-beg 1))
+    (linum-update-window (selected-window))
+    (goto-char here)))
+(put 'linum-update-selected-window 'permanent-local-hook t)
+
 (defun linum-update-window (win)
   "Update line numbers for the portion visible in window WIN."
   (goto-char (window-start win))
@@ -142,6 +148,7 @@
                                       (count-lines (point-min) (point-max))))))
                       (concat "%" (number-to-string w) "d")))))
         (width 0))
+    (when (<= linum-change-beg limit)
     (run-hooks 'linum-before-numbering-hook)
     ;; Create an overlay (or reuse an existing one) for each
     ;; line visible in this window, if necessary.
@@ -171,24 +178,40 @@
       (let ((inhibit-point-motion-hooks t))
         (forward-line))
       (setq line (1+ line)))
-    (set-window-margins win width (cdr (window-margins win)))))
+      (set-window-margins win width (cdr (window-margins win))))))
+
+(defvar linum-change-beg nil
+  "Position of change beginning, recorded after change.")
+(make-variable-buffer-local 'linum-change-beg)
+(put linum-change-beg 'permanent-local t)
+
+(defvar linum-changed-region-has-newline nil)
+(defun linum-before-change (beg end)
+  ;; Record new lines in changed region for check in after change function.
+  (when (string-match-p "\n" (buffer-substring-no-properties beg end))
+    (setq linum-changed-region-has-newline t)))
+(put 'linum-before-change 'permanent-local-hook t)
 
 (defun linum-after-change (beg end len)
-  ;; update overlays on deletions, and after newlines are inserted
-  (when (or (= beg end)
-            (= end (point-max))
+  ;; update overlays after newlines are delete or inserted
+  (when (or linum-changed-region-has-newline
             (string-match-p "\n" (buffer-substring-no-properties beg end)))
-    (linum-update-current)))
+    (setq linum-changed-region-has-newline nil)
+    (setq linum-change-beg beg)))
+(put 'linum-after-change 'permanent-local-hook t)
 
 (defun linum-after-scroll (win start)
-  (linum-update (window-buffer win)))
+  (with-selected-window win
+    (linum-update-selected-window)))
+(put 'linum-after-scroll 'permanent-local-hook t)
 
 ;; (defun linum-after-size (frame)
 ;;   (linum-after-config))
 
 (defun linum-schedule ()
   ;; schedule an update; the delay gives Emacs a chance for display changes
-  (run-with-idle-timer 0 nil #'linum-update-current))
+  (run-with-idle-timer 0 nil #'linum-update-current-buffer))
+(put 'linum-schedule 'permanent-local-hook t)
 
 ;; (defun linum-after-config ()
 ;;   (walk-windows (lambda (w) (linum-update (window-buffer w))) nil 'visible))

bug#6871: Please make linum-mode per buffer, not per major mode

[Juanma Barranquero]

On Thu, Aug 19, 2010 at 15:25, Lennart Borgman
<lennart.borgman@gmail.com> wrote:

> Ah, shit. Thanks. I must learn to switch buffer before I save the diff....
>
> I think I have attached the diff now ;-)

That's better, thanks. Still no ChangeLog to explain the changes, though.

> Maybe. Linum is rather small and it is quicker to do it this way.

Quicker for you, perhaps, but if the changes are unrelated should be
committed separately, so harder for someone at some point.

    Juanma

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Thu, Aug 19, 2010 at 3:38 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 15:25, Lennart Borgman
> <lennart.borgman@gmail.com> wrote:
>
>> Ah, shit. Thanks. I must learn to switch buffer before I save the diff....
>>
>> I think I have attached the diff now ;-)
>
> That's better, thanks. Still no ChangeLog to explain the changes, though.

It is still just for communication.

>> Maybe. Linum is rather small and it is quicker to do it this way.
>
> Quicker for you, perhaps, but if the changes are unrelated should be
> committed separately, so harder for someone at some point.

Do they have to be committed separately? It is just small (but
important) changes.

bug#6871: Please make linum-mode per buffer, not per major mode

[Juanma Barranquero]

On Thu, Aug 19, 2010 at 16:00, Lennart Borgman
<lennart.borgman@gmail.com> wrote:

> It is still just for communication.

ChangeLog entries help communicate intent.

> Do they have to be committed separately?

At the risk of repeating me thrice: if they are unrelated, yes, they
should be committed separately.

    Juanma

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Thu, Aug 19, 2010 at 4:40 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 16:00, Lennart Borgman
> <lennart.borgman@gmail.com> wrote:
>
>> It is still just for communication.
>
> ChangeLog entries help communicate intent.

I thought the intent would be obvious in this small amount of code,
but it looks like I am wrong.

>> Do they have to be committed separately?
>
> At the risk of repeating me thrice: if they are unrelated, yes, they
> should be committed separately.

I see. How do you yourself manage a situation like this? What happened is this:

- I found a problem for multi major mode use of linum.
- Looking at the code I found some other easy to see problems.

Normally in my own code I just fix all the issues I see on the fly so
to say. That is faster since I have already looked at the code then
and knows what to do. Later (which may be much later) I might forget
about exactly how to do it.

Do you try to keep multiple copies of the code with different patches
in each? Or what do you do?

bug#6871: Please make linum-mode per buffer, not per major mode

[Juanma Barranquero]

On Thu, Aug 19, 2010 at 23:23, Lennart Borgman
<lennart.borgman@gmail.com> wrote:

> I thought the intent would be obvious in this small amount of code,
> but it looks like I am wrong.

Usually, changes are much clearer to the writer than to the reader :-)

> Do you try to keep multiple copies of the code with different patches
> in each? Or what do you do?

That depends. I don't develop on the trunk, but a branch, so if some
of the changes are trivial fixes or whatever, I move them to the trunk
and commit them right now (so one less thing to think about) and keep
the other changes in the branch while I refine them. Else, I commit
each change to my branch as separate commits, and then, when the time
comes to merge back to the trunk, I decide if they are related or not,
so I push them as a merge or separate commits.

    Juanma

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Thu, Aug 19, 2010 at 11:57 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 23:23, Lennart Borgman
> <lennart.borgman@gmail.com> wrote:
>
>> I thought the intent would be obvious in this small amount of code,
>> but it looks like I am wrong.
>
> Usually, changes are much clearer to the writer than to the reader :-)

Depends on who is writing and who is reading. I hoped they were
clearer to the reader ;-)

>> Do you try to keep multiple copies of the code with different patches
>> in each? Or what do you do?
>
> That depends. I don't develop on the trunk, but a branch, so if some
> of the changes are trivial fixes or whatever, I move them to the trunk
> and commit them right now (so one less thing to think about) and keep
> the other changes in the branch while I refine them. Else, I commit
> each change to my branch as separate commits, and then, when the time
> comes to merge back to the trunk, I decide if they are related or not,
> so I push them as a merge or separate commits.

Thansk, I think I understand the workflow, but I do not know how to do
this practically.

Let us say I first in this case write the parts for multi major modes.
I guess I then submit this to my local rep? How?

And how do I make a diff after that? Can I make the diff later on to
in a simple way?

After this I want to do the other changes to linum immediately so I
will not forget them. So I do that and submit them to my local rep. Or
should I do something else at this point?

And what if Stefan (or someone else) now says that the first part
needs some refinements and I should send a new patch? How can I handle
that now after I have submitted my first part to my local rep?

bug#6871: Please make linum-mode per buffer, not per major mode

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Fri, 20 Aug 2010 00:21:40 +0200
> Cc: 6871@debbugs.gnu.org
> 
> > That depends. I don't develop on the trunk, but a branch, so if some
> > of the changes are trivial fixes or whatever, I move them to the trunk
> > and commit them right now (so one less thing to think about) and keep
> > the other changes in the branch while I refine them. Else, I commit
> > each change to my branch as separate commits, and then, when the time
> > comes to merge back to the trunk, I decide if they are related or not,
> > so I push them as a merge or separate commits.
> 
> Thansk, I think I understand the workflow, but I do not know how to do
> this practically.
> 
> Let us say I first in this case write the parts for multi major modes.
> I guess I then submit this to my local rep? How?

You need to create a separate branch, with the "bzr branch" command.
Then you can commit there with "bzr ci".

Alternatively, you can use "bzr ci --local" in the bound branch, but
this is less recommended, since it requires you to be aware of local
commits that you didn't push to the remote repository.

> And how do I make a diff after that? Can I make the diff later on to
> in a simple way?

Ye, "bzr diff" supports versions.  E.g.,

  bzr diff -r101100..101101

will produce the diffs between the two named versions.

> After this I want to do the other changes to linum immediately so I
> will not forget them. So I do that and submit them to my local rep.

Yes.

> Or should I do something else at this point?

Nothing else is required.

> And what if Stefan (or someone else) now says that the first part
> needs some refinements and I should send a new patch? How can I handle
> that now after I have submitted my first part to my local rep?

You commit another change and use "bzr diff" as above, with a
different couple of revision numbers.

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

> Normally in my own code I just fix all the issues I see on the fly so
> to say. That is faster since I have already looked at the code then
> and knows what to do. Later (which may be much later) I might forget
> about exactly how to do it.

I have my own branch with a hodge-podge of changes in random order.
When I want to install a change from there to the trunk, I do a diff of
the relevant files and then pick the relevant parts and add
a ChangeLog message.

I usually take advantage of this opportunity to polish the change a bit
(which might mean to rewrite it completely in a different way, or just
add a comment, ...).  When I update my branch from trunk, I then get
"spurious conflicts", but they're easy to fix and usually having
a chance to revisit that code before committing it lets me improve
it significantly, so it's definitely worth the trouble.
Especially since it lets me (on the other side) make random changes to
my heart's content.


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

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

On Fri, Aug 20, 2010 at 12:43 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> Normally in my own code I just fix all the issues I see on the fly so
>> to say. That is faster since I have already looked at the code then
>> and knows what to do. Later (which may be much later) I might forget
>> about exactly how to do it.
>
> I have my own branch with a hodge-podge of changes in random order.
> When I want to install a change from there to the trunk, I do a diff of
> the relevant files and then pick the relevant parts and add
> a ChangeLog message.

Seems easiest, but I am not sure how to make the diff file. I just
edited the diff file I had. Does this work, i.e. how does the program
patch handle this?

I have attached a patch done this way for just making linum-mode per
buffer instead of per major mode.

[-- Attachment #2: linum-multi-major-1.diff --]
[-- Type: application/octet-stream, Size: 1570 bytes --]

=== modified file 'lisp/linum.el'
--- trunk/lisp/linum.el	2010-01-13 08:35:10 +0000
+++ patched/lisp/linum.el	2010-08-19 04:22:04 +0000
@@ -38,6 +38,8 @@
   "Functions run in each buffer before line numbering starts.")
 
 (mapc #'make-variable-buffer-local '(linum-overlays linum-available))
+(put 'linum-overlays  'permanent-local t)
+(put 'linum-available 'permanent-local t)
 
 (defgroup linum nil
   "Show line numbers in the left margin."
@@ -115,3 +109,4 @@
   "Update line numbers for the current buffer."
   (linum-update (current-buffer)))
+(put 'linum-update-current 'permanent-local-hook t)
 
@@ -171,19 +178,22 @@
 (defun linum-after-change (beg end len)
  ;; update overlays on deletions, and after newlines are inserted
  (when (or (= beg end)
            (= end (point-max))
            (string-match-p "\n" (buffer-substring-no-properties beg end)))
    (linum-update-current)))
+(put 'linum-after-change 'permanent-local-hook t)
 
 (defun linum-after-scroll (win start)
  (linum-update (window-buffer win)))
+(put 'linum-after-scroll 'permanent-local-hook t)
 
 ;; (defun linum-after-size (frame)
 ;;   (linum-after-config))
 
 (defun linum-schedule ()
   ;; schedule an update; the delay gives Emacs a chance for display changes
-  (run-with-idle-timer 0 nil #'linum-update-current))
+  (run-with-idle-timer 0 nil #'linum-update-current-buffer))
+(put 'linum-schedule 'permanent-local-hook t)
 
 ;; (defun linum-after-config ()
 ;;   (walk-windows (lambda (w) (linum-update (window-buffer w))) nil 'visible))


[-- Attachment #3: linum-multi-major-1.chlog --]
[-- Type: application/octet-stream, Size: 131 bytes --]

2010-08-20  Lennart Borgman  <lennart.borgman@gmail.com>

	* linum.el: Made linum-mode per buffer rather than per major mode.

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

>>> Normally in my own code I just fix all the issues I see on the fly so
>>> to say. That is faster since I have already looked at the code then
>>> and knows what to do. Later (which may be much later) I might forget
>>> about exactly how to do it.
>> I have my own branch with a hodge-podge of changes in random order.
>> When I want to install a change from there to the trunk, I do a diff of
>> the relevant files and then pick the relevant parts and add
>> a ChangeLog message.
> Seems easiest, but I am not sure how to make the diff file.

By "I do a diff of the relevant files", I meant "bzr diff -rsubmit:
<files>", or rather C-u C-x v = s[TAB] RET RET.
By "pick the relevant parts", I meant:
- go to the *vc-diff* buffer
- M-x cd RET /my/checkout/of/the/trunk RET
- apply C-c C-a to the hunks I want to pick
- edit to clean up, improve, ...
- save the modified files.
- do C-x v = on the modified files, to see the patch I'm about to commit
  to trunk (or to send it via email).

> I just edited the diff file I had. Does this work,

Depends if you edited it well or not.

> i.e. how does the program patch handle this?

Again, depends if you edited it correctly or not.  Have you tried it?

> I have attached a patch done this way for just making linum-mode per
> buffer instead of per major mode.

Have you tried the patch and confirmed that it works and fixes
your problem?  I see it lacks the removal of change-major-mode-hook.


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

> I think I have attached the diff now ;-)

Yes, this time it looks right, thanks.

>>> There are some other changes to linum.el too.
>> Why? If they are not related, that should be sent as another patch.
> Maybe. Linum is rather small and it is quicker to do it this way.

I don't mind a few cosmetic changes as long as they don't drown the main
change, but in this case, the changes aren't cosmetic at all.
They don't sound bad, tho.  So please post them separately, with an
explanation for why they're right.

And please resend a new patch that only fixes the permanent-local part
of linum-mode, because I'm having a hard time figuring out which part
belongs to what.


        Stefan


> === modified file 'lisp/linum.el'
> --- trunk/lisp/linum.el	2010-01-13 08:35:10 +0000
> +++ patched/lisp/linum.el	2010-08-19 04:22:04 +0000
> @@ -38,6 +38,8 @@
>    "Functions run in each buffer before line numbering starts.")
 
>  (mapc #'make-variable-buffer-local '(linum-overlays linum-available))
> +(put 'linum-overlays  'permanent-local t)
> +(put 'linum-available 'permanent-local t)
 
>  (defgroup linum nil
>    "Show line numbers in the left margin."
> @@ -58,13 +60,6 @@
>    "Face for displaying line numbers in the display margin."
>    :group 'linum)
 
> -(defcustom linum-eager t
> -  "Whether line numbers should be updated after each command.
> -The conservative setting `nil' might miss some buffer changes,
> -and you have to scroll or press \\[recenter-top-bottom] to update the numbers."
> -  :group 'linum
> -  :type 'boolean)
> -
>  (defcustom linum-delay nil
>    "Delay updates to give Emacs a chance for other changes."
>    :group 'linum
> @@ -76,30 +71,29 @@
>    :lighter ""                           ; for desktop.el
>    (if linum-mode
>        (progn
> -        (if linum-eager
>              (add-hook 'post-command-hook (if linum-delay
>                                               'linum-schedule
> -                                           'linum-update-current) nil t)
> -          (add-hook 'after-change-functions 'linum-after-change nil t))
> +                                       'linum-update-current-buffer) nil t)
> +        (add-hook 'before-change-functions 'linum-before-change nil t)
> +        (add-hook 'after-change-functions 'linum-after-change nil t)
>          (add-hook 'window-scroll-functions 'linum-after-scroll nil t)
>          ;; Using both window-size-change-functions and
>          ;; window-configuration-change-hook seems redundant. --Stef
>          ;; (add-hook 'window-size-change-functions 'linum-after-size nil t)
> -        (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
>          (add-hook 'window-configuration-change-hook
> -                  ;; FIXME: If the buffer is shown in N windows, this
> -                  ;; will be called N times rather than once.  We should use
> -                  ;; something like linum-update-window instead.
> -                  'linum-update-current nil t)
> -        (linum-update-current))
> -    (remove-hook 'post-command-hook 'linum-update-current t)
> +                  ;; Update just the selected window
> +                  'linum-update-selected-window nil t)
> +        (setq linum-change-beg 1)
> +        (linum-update-current-buffer))
> +    (remove-hook 'post-command-hook 'linum-update-current-buffer t)
>      (remove-hook 'post-command-hook 'linum-schedule t)
>      ;; (remove-hook 'window-size-change-functions 'linum-after-size t)
>      (remove-hook 'window-scroll-functions 'linum-after-scroll t)
> +    (remove-hook 'before-change-functions 'linum-before-change t)
>      (remove-hook 'after-change-functions 'linum-after-change t)
> -    (remove-hook 'window-configuration-change-hook 'linum-update-current t)
> -    (remove-hook 'change-major-mode-hook 'linum-delete-overlays t)
> +    (remove-hook 'window-configuration-change-hook 'linum-update-selected-window t)
>      (linum-delete-overlays)))
> +(put 'linum-mode      'permanent-local t)
 
>  ;;;###autoload
>  (define-globalized-minor-mode global-linum-mode linum-mode linum-on)
> @@ -115,22 +109,34 @@
>    (dolist (w (get-buffer-window-list (current-buffer) nil t))
>      (set-window-margins w 0 (cdr (window-margins w)))))
 
> -(defun linum-update-current ()
> +(defun linum-update-current-buffer ()
>    "Update line numbers for the current buffer."
>    (linum-update (current-buffer)))
> +(put 'linum-update-current-buffer 'permanent-local-hook t)
 
>  (defun linum-update (buffer)
>    "Update line numbers for all windows displaying BUFFER."
>    (with-current-buffer buffer
> -    (when linum-mode
> +    (when (and linum-mode
> +               linum-change-beg)
>        (setq linum-available linum-overlays)
>        (setq linum-overlays nil)
>        (save-excursion
>          (mapc #'linum-update-window
>                (get-buffer-window-list buffer nil 'visible)))
>        (mapc #'delete-overlay linum-available)
> +      (setq linum-change-beg nil)
>        (setq linum-available nil))))
 
> +(defun linum-update-selected-window ()
> +  "Update line numbers for the selected window."
> +  (let ((here (window-point))
> +        ;; We might have scrolled or changed win config
> +        (linum-change-beg 1))
> +    (linum-update-window (selected-window))
> +    (goto-char here)))
> +(put 'linum-update-selected-window 'permanent-local-hook t)
> +
>  (defun linum-update-window (win)
>    "Update line numbers for the portion visible in window WIN."
>    (goto-char (window-start win))
> @@ -142,6 +148,7 @@
>                                        (count-lines (point-min) (point-max))))))
>                        (concat "%" (number-to-string w) "d")))))
>          (width 0))
> +    (when (<= linum-change-beg limit)
>      (run-hooks 'linum-before-numbering-hook)
>      ;; Create an overlay (or reuse an existing one) for each
>      ;; line visible in this window, if necessary.
> @@ -171,24 +178,40 @@
>        (let ((inhibit-point-motion-hooks t))
>          (forward-line))
>        (setq line (1+ line)))
> -    (set-window-margins win width (cdr (window-margins win)))))
> +      (set-window-margins win width (cdr (window-margins win))))))
> +
> +(defvar linum-change-beg nil
> +  "Position of change beginning, recorded after change.")
> +(make-variable-buffer-local 'linum-change-beg)
> +(put linum-change-beg 'permanent-local t)
> +
> +(defvar linum-changed-region-has-newline nil)
> +(defun linum-before-change (beg end)
> +  ;; Record new lines in changed region for check in after change function.
> +  (when (string-match-p "\n" (buffer-substring-no-properties beg end))
> +    (setq linum-changed-region-has-newline t)))
> +(put 'linum-before-change 'permanent-local-hook t)
 
>  (defun linum-after-change (beg end len)
> -  ;; update overlays on deletions, and after newlines are inserted
> -  (when (or (= beg end)
> -            (= end (point-max))
> +  ;; update overlays after newlines are delete or inserted
> +  (when (or linum-changed-region-has-newline
>              (string-match-p "\n" (buffer-substring-no-properties beg end)))
> -    (linum-update-current)))
> +    (setq linum-changed-region-has-newline nil)
> +    (setq linum-change-beg beg)))
> +(put 'linum-after-change 'permanent-local-hook t)
 
>  (defun linum-after-scroll (win start)
> -  (linum-update (window-buffer win)))
> +  (with-selected-window win
> +    (linum-update-selected-window)))
> +(put 'linum-after-scroll 'permanent-local-hook t)
 
>  ;; (defun linum-after-size (frame)
>  ;;   (linum-after-config))
 
>  (defun linum-schedule ()
>    ;; schedule an update; the delay gives Emacs a chance for display changes
> -  (run-with-idle-timer 0 nil #'linum-update-current))
> +  (run-with-idle-timer 0 nil #'linum-update-current-buffer))
> +(put 'linum-schedule 'permanent-local-hook t)
 
>  ;; (defun linum-after-config ()
>  ;;   (walk-windows (lambda (w) (linum-update (window-buffer w))) nil 'visible))

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Thu, Aug 19, 2010 at 4:45 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> I don't mind a few cosmetic changes as long as they don't drown the main
> change, but in this case, the changes aren't cosmetic at all.
> They don't sound bad, tho.  So please post them separately, with an
> explanation for why they're right.
>
> And please resend a new patch that only fixes the permanent-local part
> of linum-mode, because I'm having a hard time figuring out which part
> belongs to what.

Ah, sorry. I thought it would be easy for you to do that. I will try
to find time for it.

bug#6871: Please make linum-mode per buffer, not per major mode

[MON KEY]

> I think such a change is acceptable (although the motivation of
> multi-major-mode buffers might not be that compelling, since it
> tends to lead to the idea that most/all minor-modes should be made
> permanent-local), but we need a proper patch for it.

Thats potentially a lot of overlays to persist in buffers which:

- may not always be visible
- may be visible but don't need the linum minor-mode behaviour
  automatically persisted

Will such a change negatively impact redisplay elsewhere?
I ask because of this comment here from linum.el:

,----
|
| (add-hook 'window-configuration-change-hook
| 	  ;; FIXME: If the buffer is shown in N windows, this
| 	  ;; will be called N times rather than once.  We should use
| 	  ;; something like linum-update-window instead.
|  'linum-update-current nil t)
|
`----

Also, linum-mode activation already sets timers when linum-eager is
non-nil... will these timers now persist as well on major-mode
changes? If so how will they impact redisplay?

More concretely, how many (more) permanent-locals does Lennart need in
order to attain mumamo.el bliss?

And, while your giving permanent-locals away so freely can maybe you
spare a few for the slime guys?  Or the geiser/quack/scheme guys I'm
sure they could use some too... :-P

--
/s_P\

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Wed, Aug 18, 2010 at 1:47 AM, MON KEY <monkey@sandpframing.com> wrote:
>> I think such a change is acceptable (although the motivation of
>> multi-major-mode buffers might not be that compelling, since it
>> tends to lead to the idea that most/all minor-modes should be made
>> permanent-local), but we need a proper patch for it.
>
> Thats potentially a lot of overlays to persist in buffers which:
>
> - may not always be visible
> - may be visible but don't need the linum minor-mode behaviour
>  automatically persisted
>
> Will such a change negatively impact redisplay elsewhere?
> I ask because of this comment here from linum.el:
>
> ,----
> |
> | (add-hook 'window-configuration-change-hook
> |         ;; FIXME: If the buffer is shown in N windows, this
> |         ;; will be called N times rather than once.  We should use
> |         ;; something like linum-update-window instead.
> |  'linum-update-current nil t)
> |
> `----
>
> Also, linum-mode activation already sets timers when linum-eager is
> non-nil... will these timers now persist as well on major-mode
> changes? If so how will they impact redisplay?
>
> More concretely, how many (more) permanent-locals does Lennart need in
> order to attain mumamo.el bliss?

Those things are not related.

bug#6871: Please make linum-mode per buffer, not per major mode

[Stefan Monnier]

>> I think such a change is acceptable (although the motivation of
>> multi-major-mode buffers might not be that compelling, since it
>> tends to lead to the idea that most/all minor-modes should be made
>> permanent-local), but we need a proper patch for it.

> Thats potentially a lot of overlays to persist in buffers which:

> - may not always be visible
> - may be visible but don't need the linum minor-mode behaviour
>   automatically persisted

It's relatively rare to use linum-mode and then to change major-mode.
So (until proven otherwise) it's a non-issue.


        Stefan

bug#6871: Please make linum-mode per buffer, not per major mode

[MON KEY]

On Wed, Aug 18, 2010 at 3:20 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> It's relatively rare to use linum-mode and then to change major-mode.

Isn't that precisely the reason Lennart made the request?
E.g. that mumamo buffers _do_ change modes repeatedly?

Whatever, this doesn't address the stated concern that the overlays
will now have potential for persistence where there was none before.

> So (until proven otherwise) it's a non-issue.

OK.
FWIW I think this will come back to bite w/re redisplay once mumamo
related code starts exploiting the feature.
Witness the aggressive leeway Lennart has taken w/ his linum "patch"...

>        Stefan
>

--
/s_P\

bug#6871: Please make linum-mode per buffer, not per major mode

[Lennart Borgman]

On Thu, Aug 19, 2010 at 7:49 PM, MON KEY <monkey@sandpframing.com> wrote:
>
> Whatever, this doesn't address the stated concern that the overlays
> will now have potential for persistence where there was none before.

I am sorry, but you have misunderstood this. And that is all I have
time to say about it. Try to figure out yourself.

> Witness the aggressive leeway Lennart has taken w/ his linum "patch"...

What is "leeway"?

bug#6871: Please make linum-mode per buffer, not per major mode

[Lars Ingebrigtsen]

Lennart Borgman <lennart.borgman@gmail.com> writes:

> Line numbers should work even in multi major mode buffers. Please add
> the following lines to linum.el:
>
> (put 'linum-overlays  'permanent-local t)
> (put 'linum-available 'permanent-local t)
> (put 'linum-mode      'permanent-local t)

This was ten years ago.  These days, `display-line-numbers-mode' does
pretty much everything linum-mode does, but better, so I think there's
probably no reason to implement this feature request, and I'm closing
this bug report.  (I've just skimmed the thread, though, so if there is
something here that should definitely be fixed, please respond to the
debbugs address and we'll reopen.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no