Duplicated outline-cycle binding, and problems with the new one

Duplicated outline-cycle binding, and problems with the new one

[Yuan Fu]

When outline-cycle was originally added, I think we only added this

(defvar outline-mode-map
  (let ((map (make-sparse-keymap)))
    (define-key map "\C-c" outline-mode-prefix-map)
    (define-key map [menu-bar] outline-mode-menu-bar-map)
    ;; Only takes effect if point is on a heading.
    (define-key map (kbd "TAB")
      `(menu-item "" outline-cycle
                  :filter ,(lambda (cmd)
                             (when (outline-on-heading-p) cmd))))
    (define-key map (kbd "<backtab>") #'outline-cycle-buffer)
    map))

So ‘outline-cycle’ takes effect when point is on a heading. Now in addition to this, I also see

(defvar outline-minor-mode-cycle-map
  (let ((map (make-sparse-keymap)))
    (outline-minor-mode-cycle--bind map (kbd "TAB") #'outline-cycle)
    (outline-minor-mode-cycle--bind map (kbd "<backtab>") #'outline-cycle-buffer)
    map)
  "Keymap used by `outline-minor-mode-cycle'.”)

Which presumably are applied as keymap text properties to headings in a buffer. I’m having problems with this: some text that are not headings in my buffer are incorrectly propertied with this keymap, and when I try to indent with TAB, outline-cycle is invoked.

Overall, from my limited knowledge, I think the old approach is more reliable: I wouldn’t have this problem with the old approach. And the new functionality added by the new approach and outline-minor-mode-cycle-filter can be easily implemented in the old approach. We don’t need to fiddle with font-lock-keywords with the old approach, either. How about we go back to the old approach? Juri, WDYT?

Yuan

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

> Which presumably are applied as keymap text properties to headings in
> a buffer.  I’m having problems with this: some text that are not
> headings in my buffer are incorrectly propertied with this keymap, and
> when I try to indent with TAB, outline-cycle is invoked.

This looks like a bug, maybe you have a wrong outline-regexp.

> Overall, from my limited knowledge, I think the old approach is more
> reliable: I wouldn’t have this problem with the old approach.  And the
> new functionality added by the new approach and
> outline-minor-mode-cycle-filter can be easily implemented in the old
> approach.  We don’t need to fiddle with font-lock-keywords with the
> old approach, either.  How about we go back to the old approach?

Some time ago we discussed this possibility, but it will require writing
too many wrappers for different modes, for example, for diff-mode:

  (defvar-keymap diff-mode-shared-map
    "TAB" (lambda () (interactive)
            (if (and outline-minor-mode (outline-on-heading-p))
                (outline-cycle)
              (diff-hunk-next)))
  ...

Re: Duplicated outline-cycle binding, and problems with the new one

[Stefan Monnier]

>> Overall, from my limited knowledge, I think the old approach is more
>> reliable: I wouldn’t have this problem with the old approach.  And the
>> new functionality added by the new approach and
>> outline-minor-mode-cycle-filter can be easily implemented in the old
>> approach.  We don’t need to fiddle with font-lock-keywords with the
>> old approach, either.  How about we go back to the old approach?
>
> Some time ago we discussed this possibility, but it will require writing
> too many wrappers for different modes, for example, for diff-mode:
>
>   (defvar-keymap diff-mode-shared-map
>     "TAB" (lambda () (interactive)
>             (if (and outline-minor-mode (outline-on-heading-p))
>                 (outline-cycle)
>               (diff-hunk-next)))
>   ...

Why?  The old approach used a conditional binding, so it should "just
work" without the major modes knowing about it.


        Stefan

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

>>> Overall, from my limited knowledge, I think the old approach is more
>>> reliable: I wouldn’t have this problem with the old approach.  And the
>>> new functionality added by the new approach and
>>> outline-minor-mode-cycle-filter can be easily implemented in the old
>>> approach.  We don’t need to fiddle with font-lock-keywords with the
>>> old approach, either.  How about we go back to the old approach?
>>
>> Some time ago we discussed this possibility, but it will require writing
>> too many wrappers for different modes, for example, for diff-mode:
>>
>>   (defvar-keymap diff-mode-shared-map
>>     "TAB" (lambda () (interactive)
>>             (if (and outline-minor-mode (outline-on-heading-p))
>>                 (outline-cycle)
>>               (diff-hunk-next)))
>>   ...
>
> Why?  The old approach used a conditional binding, so it should "just
> work" without the major modes knowing about it.

Then maybe something like this (not tested):

diff --git a/lisp/outline.el b/lisp/outline.el
index 0304d2334c..52b9883950 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -195,8 +195,10 @@ outline-minor-mode-cycle--bind
       :filter
       ,(or filter
            (lambda (cmd)
-             (when (or (not (functionp outline-minor-mode-cycle-filter))
-                       (funcall outline-minor-mode-cycle-filter))
+             (when (and outline-minor-mode-cycle
+                        (outline-on-heading-p)
+                        (or (not (functionp outline-minor-mode-cycle-filter))
+                            (funcall outline-minor-mode-cycle-filter)))
                cmd))))))
 
 (defvar outline-minor-mode-cycle-map
@@ -225,10 +227,8 @@ outline-font-lock-keywords
                   0 '(if outline-minor-mode
                          (if outline-minor-mode-cycle
                              (if outline-minor-mode-highlight
-                                 (list 'face (outline-font-lock-face)
-                                       'keymap outline-minor-mode-cycle-map)
-                               (list 'face nil
-                                     'keymap outline-minor-mode-cycle-map))
+                                 (list 'face (outline-font-lock-face))
+                               (list 'face nil))
                            (if outline-minor-mode-highlight
                                (list 'face (outline-font-lock-face))))
                        (outline-font-lock-face))
@@ -421,8 +422,10 @@ outline-minor-mode
 
 See the command `outline-mode' for more information on this mode."
   :lighter " Outl"
-  :keymap (list (cons [menu-bar] outline-minor-mode-menu-bar-map)
-		(cons outline-minor-mode-prefix outline-mode-prefix-map))
+  :keymap (append
+           (list (cons [menu-bar] outline-minor-mode-menu-bar-map)
+                 (cons outline-minor-mode-prefix outline-mode-prefix-map))
+           (cdr outline-minor-mode-cycle-map))
   (if outline-minor-mode
       (progn
         (when (or outline-minor-mode-cycle outline-minor-mode-highlight)
-- 

Re: Duplicated outline-cycle binding, and problems with the new one

[Yuan Fu]

> On Jan 2, 2022, at 11:18 AM, Juri Linkov <juri@linkov.net> wrote:
> 
>>>> Overall, from my limited knowledge, I think the old approach is more
>>>> reliable: I wouldn’t have this problem with the old approach.  And the
>>>> new functionality added by the new approach and
>>>> outline-minor-mode-cycle-filter can be easily implemented in the old
>>>> approach.  We don’t need to fiddle with font-lock-keywords with the
>>>> old approach, either.  How about we go back to the old approach?
>>> 
>>> Some time ago we discussed this possibility, but it will require writing
>>> too many wrappers for different modes, for example, for diff-mode:
>>> 
>>>  (defvar-keymap diff-mode-shared-map
>>>    "TAB" (lambda () (interactive)
>>>            (if (and outline-minor-mode (outline-on-heading-p))
>>>                (outline-cycle)
>>>              (diff-hunk-next)))
>>>  ...
>> 
>> Why?  The old approach used a conditional binding, so it should "just
>> work" without the major modes knowing about it.
> 
> Then maybe something like this (not tested):

[…]

This looks really good to me, for what it’s worth.

Yuan

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

>>> Overall, from my limited knowledge, I think the old approach is more
>>> reliable: I wouldn’t have this problem with the old approach.  And the
>>> new functionality added by the new approach and
>>> outline-minor-mode-cycle-filter can be easily implemented in the old
>>> approach.  We don’t need to fiddle with font-lock-keywords with the
>>> old approach, either.  How about we go back to the old approach?
>>
>> Some time ago we discussed this possibility, but it will require writing
>> too many wrappers for different modes, for example, for diff-mode:
>
> Why?  The old approach used a conditional binding, so it should "just
> work" without the major modes knowing about it.

Unfortunately, this is impossible to do.  When using

  (define-minor-mode outline-minor-mode
    :keymap (easy-mmode-define-keymap
             `(([menu-bar] . ,outline-minor-mode-menu-bar-map)
               (,outline-minor-mode-prefix . ,outline-mode-prefix-map))
             :inherit outline-minor-mode-cycle-map)

it updates minor-mode-map-alist with the keymap where the TAB key
is bound to outline-cycle in outline-minor-mode-cycle-map.

But diff-mode overrides this binding with diff-mode-shared-map
where the TAB key is bound to diff-hunk-next, since
minor-mode-overriding-map-alist takes priority over
minor-mode-map-alist when diff-mode does this:

  (let ((ro-bind (cons 'buffer-read-only diff-mode-shared-map)))
    (add-to-list 'minor-mode-overriding-map-alist ro-bind)

so TAB bound to outline-cycle can't be used on outline headings
in read-only diff buffers.

Re: Duplicated outline-cycle binding, and problems with the new one

[Stefan Monnier]

> But diff-mode overrides this binding with diff-mode-shared-map
> where the TAB key is bound to diff-hunk-next, since
> minor-mode-overriding-map-alist takes priority over
> minor-mode-map-alist when diff-mode does this:

I see.  I knew using `minor-mode-overriding-map-alist` in `diff-mode.el`
this way was going to bite us sooner or later.

Maybe we should use a hook on `read-only-mode` to set/unset
a `diff-mode-read-only` variable so we can add the keymap
(conditionalized on this new `diff-mode-read-only`) to
`minor-mode-map-alist` instead of `minor-mode-overriding-map-alist`.


        Stefan

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

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

>> But diff-mode overrides this binding with diff-mode-shared-map
>> where the TAB key is bound to diff-hunk-next, since
>> minor-mode-overriding-map-alist takes priority over
>> minor-mode-map-alist when diff-mode does this:
>
> I see.  I knew using `minor-mode-overriding-map-alist` in `diff-mode.el`
> this way was going to bite us sooner or later.
>
> Maybe we should use a hook on `read-only-mode` to set/unset
> a `diff-mode-read-only` variable so we can add the keymap
> (conditionalized on this new `diff-mode-read-only`) to
> `minor-mode-map-alist` instead of `minor-mode-overriding-map-alist`.

So this is because `minor-mode-map-alist` is not buffer-local.
Then this requires changing `(setq buffer-read-only t)` to
`(read-only-mode 1)` in diff-related places.  Since `read-only-mode`
always activates `view-mode` when `view-read-only` is t,
it needs let-binding: (let ((view-read-only nil)) (read-only-mode 1)).
This will keep the current behavior.  Then special-handling of
`view-mode` in `diff-mode` is not needed because `view-mode`
is higher than `diff-mode-read-only` in `minor-mode-map-alist`,
where `diff-mode-read-only` is at the end to not take precedence
over `outline-minor-mode`.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-mode-read-only.patch --]
[-- Type: text/x-diff, Size: 2797 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index ca8df5d380..63b9549b54 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1479,6 +1479,12 @@ diff--font-lock-cleanup
 (defvar whitespace-style)
 (defvar whitespace-trailing-regexp)
 
+(defvar-local diff-mode-read-only nil)
+
+(or (assq 'diff-mode-read-only minor-mode-map-alist)
+    (nconc minor-mode-map-alist
+           (list (cons 'diff-mode-read-only diff-mode-shared-map))))
+
 ;;;###autoload
 (define-derived-mode diff-mode fundamental-mode "Diff"
   "Major mode for viewing/editing context diffs.
@@ -1516,23 +1522,19 @@ diff-mode
 
   (diff-setup-whitespace)
 
+  (add-hook 'read-only-mode-hook
+            (lambda ()
+              (setq diff-mode-read-only buffer-read-only))
+            nil t)
+
   (if diff-default-read-only
-      (setq buffer-read-only t))
+      (let ((view-read-only nil)) (read-only-mode 1)))
   ;; setup change hooks
   (if (not diff-update-on-the-fly)
       (add-hook 'write-contents-functions #'diff-write-contents-hooks nil t)
     (make-local-variable 'diff-unhandled-changes)
     (add-hook 'after-change-functions #'diff-after-change-function nil t)
     (add-hook 'post-command-hook #'diff-post-command-hook nil t))
-  ;; Neat trick from Dave Love to add more bindings in read-only mode:
-  (let ((ro-bind (cons 'buffer-read-only diff-mode-shared-map)))
-    (add-to-list 'minor-mode-overriding-map-alist ro-bind)
-    ;; Turn off this little trick in case the buffer is put in view-mode.
-    (add-hook 'view-mode-hook
-	      (lambda ()
-		(setq minor-mode-overriding-map-alist
-		      (delq ro-bind minor-mode-overriding-map-alist)))
-	      nil t))
   ;; add-log support
   (setq-local add-log-current-defun-function #'diff-current-defun)
   (setq-local add-log-buffer-file-name-function
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index 4abcf6c15a..425155be40 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -182,7 +182,7 @@ diff-no-select
 		     " "))
 	 (thisdir default-directory))
     (with-current-buffer buf
-      (setq buffer-read-only t)
+      (let ((view-read-only nil)) (read-only-mode 1))
       (buffer-disable-undo (current-buffer))
       (let ((inhibit-read-only t))
 	(erase-buffer))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 6041c79efc..600c4561ac 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1817,7 +1826,7 @@ vc-diff-internal
     ;; Make the *vc-diff* buffer read only, the diff-mode key
     ;; bindings are nicer for read only buffers. pcl-cvs does the
     ;; same thing.
-    (setq buffer-read-only t)
+    (let ((view-read-only nil)) (read-only-mode 1))
     (if (and (zerop (buffer-size))
              (not (get-buffer-process (current-buffer))))
         ;; Treat this case specially so as not to pop the buffer.

Re: Duplicated outline-cycle binding, and problems with the new one

[Yuan Fu]

> On Jan 6, 2022, at 10:56 AM, Juri Linkov <juri@linkov.net> wrote:
> 
>>> But diff-mode overrides this binding with diff-mode-shared-map
>>> where the TAB key is bound to diff-hunk-next, since
>>> minor-mode-overriding-map-alist takes priority over
>>> minor-mode-map-alist when diff-mode does this:
>> 
>> I see.  I knew using `minor-mode-overriding-map-alist` in `diff-mode.el`
>> this way was going to bite us sooner or later.
>> 
>> Maybe we should use a hook on `read-only-mode` to set/unset
>> a `diff-mode-read-only` variable so we can add the keymap
>> (conditionalized on this new `diff-mode-read-only`) to
>> `minor-mode-map-alist` instead of `minor-mode-overriding-map-alist`.
> 
> So this is because `minor-mode-map-alist` is not buffer-local.
> Then this requires changing `(setq buffer-read-only t)` to
> `(read-only-mode 1)` in diff-related places.  Since `read-only-mode`
> always activates `view-mode` when `view-read-only` is t,
> it needs let-binding: (let ((view-read-only nil)) (read-only-mode 1)).
> This will keep the current behavior.  Then special-handling of
> `view-mode` in `diff-mode` is not needed because `view-mode`
> is higher than `diff-mode-read-only` in `minor-mode-map-alist`,
> where `diff-mode-read-only` is at the end to not take precedence
> over `outline-minor-mode`.

Once again, LGTM :-)

Yuan

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

>>> Maybe we should use a hook on `read-only-mode` to set/unset
>>> a `diff-mode-read-only` variable so we can add the keymap
>>> (conditionalized on this new `diff-mode-read-only`) to
>>> `minor-mode-map-alist` instead of `minor-mode-overriding-map-alist`.
>> 
>> So this is because `minor-mode-map-alist` is not buffer-local.
>> Then this requires changing `(setq buffer-read-only t)` to
>> `(read-only-mode 1)` in diff-related places.  Since `read-only-mode`
>> always activates `view-mode` when `view-read-only` is t,
>> it needs let-binding: (let ((view-read-only nil)) (read-only-mode 1)).
>> This will keep the current behavior.  Then special-handling of
>> `view-mode` in `diff-mode` is not needed because `view-mode`
>> is higher than `diff-mode-read-only` in `minor-mode-map-alist`,
>> where `diff-mode-read-only` is at the end to not take precedence
>> over `outline-minor-mode`.
>
> Once again, LGTM :-)

Testing shows that it requires more changes in other modes,
e.g. a change is needed in diff.el to make read-only
only after calling diff-mode that adds read-only-mode-hook:

diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index 4abcf6c15a..a8423237cf 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -182,12 +182,12 @@ diff-no-select
 		     " "))
 	 (thisdir default-directory))
     (with-current-buffer buf
-      (setq buffer-read-only t)
       (buffer-disable-undo (current-buffer))
       (let ((inhibit-read-only t))
 	(erase-buffer))
       (buffer-enable-undo (current-buffer))
       (diff-mode)
+      (let ((view-read-only nil)) (read-only-mode 1))
       (setq-local revert-buffer-function
                   (lambda (_ignore-auto _noconfirm)
                     (diff-no-select old new switches no-async (current-buffer))))

Re: Duplicated outline-cycle binding, and problems with the new one

[Juri Linkov]

>> Once again, LGTM :-)
>
> Testing shows that it requires more changes in other modes,
> e.g. a change is needed in diff.el to make read-only
> only after calling diff-mode that adds read-only-mode-hook:

I pushed a simpler fix.  Thanks for the suggestion.