* Duplicated outline-cycle binding, and problems with the new one
@ 2021-12-31 23:32 Yuan Fu
2022-01-02 18:25 ` Juri Linkov
0 siblings, 1 reply; 11+ messages in thread
From: Yuan Fu @ 2021-12-31 23:32 UTC (permalink / raw)
To: Emacs developers
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2021-12-31 23:32 Duplicated outline-cycle binding, and problems with the new one Yuan Fu
@ 2022-01-02 18:25 ` Juri Linkov
2022-01-02 19:07 ` Stefan Monnier
0 siblings, 1 reply; 11+ messages in thread
From: Juri Linkov @ 2022-01-02 18:25 UTC (permalink / raw)
To: Yuan Fu; +Cc: Emacs developers
> 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)))
...
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-02 18:25 ` Juri Linkov
@ 2022-01-02 19:07 ` Stefan Monnier
2022-01-02 19:18 ` Juri Linkov
2022-01-05 18:35 ` Juri Linkov
0 siblings, 2 replies; 11+ messages in thread
From: Stefan Monnier @ 2022-01-02 19:07 UTC (permalink / raw)
To: Juri Linkov; +Cc: Yuan Fu, Emacs developers
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-02 19:07 ` Stefan Monnier
@ 2022-01-02 19:18 ` Juri Linkov
2022-01-04 1:40 ` Yuan Fu
2022-01-05 18:35 ` Juri Linkov
1 sibling, 1 reply; 11+ messages in thread
From: Juri Linkov @ 2022-01-02 19:18 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Yuan Fu, Emacs developers
>>> 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)
--
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-02 19:18 ` Juri Linkov
@ 2022-01-04 1:40 ` Yuan Fu
0 siblings, 0 replies; 11+ messages in thread
From: Yuan Fu @ 2022-01-04 1:40 UTC (permalink / raw)
To: Juri Linkov; +Cc: Stefan Monnier, Emacs developers
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-02 19:07 ` Stefan Monnier
2022-01-02 19:18 ` Juri Linkov
@ 2022-01-05 18:35 ` Juri Linkov
2022-01-05 19:21 ` Stefan Monnier
1 sibling, 1 reply; 11+ messages in thread
From: Juri Linkov @ 2022-01-05 18:35 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Yuan Fu, Emacs developers
>>> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-05 18:35 ` Juri Linkov
@ 2022-01-05 19:21 ` Stefan Monnier
2022-01-06 18:56 ` Juri Linkov
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2022-01-05 19:21 UTC (permalink / raw)
To: Juri Linkov; +Cc: Yuan Fu, Emacs developers
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-05 19:21 ` Stefan Monnier
@ 2022-01-06 18:56 ` Juri Linkov
2022-01-09 22:55 ` Yuan Fu
0 siblings, 1 reply; 11+ messages in thread
From: Juri Linkov @ 2022-01-06 18:56 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Yuan Fu, Emacs developers
[-- 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.
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-06 18:56 ` Juri Linkov
@ 2022-01-09 22:55 ` Yuan Fu
2022-01-10 8:21 ` Juri Linkov
0 siblings, 1 reply; 11+ messages in thread
From: Yuan Fu @ 2022-01-09 22:55 UTC (permalink / raw)
To: Juri Linkov; +Cc: Stefan Monnier, Emacs developers
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Duplicated outline-cycle binding, and problems with the new one
2022-01-09 22:55 ` Yuan Fu
@ 2022-01-10 8:21 ` Juri Linkov
2022-01-10 18:30 ` Juri Linkov
0 siblings, 1 reply; 11+ messages in thread
From: Juri Linkov @ 2022-01-10 8:21 UTC (permalink / raw)
To: Yuan Fu; +Cc: Stefan Monnier, Emacs developers
>>> 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))))
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-01-10 18:30 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-31 23:32 Duplicated outline-cycle binding, and problems with the new one Yuan Fu
2022-01-02 18:25 ` Juri Linkov
2022-01-02 19:07 ` Stefan Monnier
2022-01-02 19:18 ` Juri Linkov
2022-01-04 1:40 ` Yuan Fu
2022-01-05 18:35 ` Juri Linkov
2022-01-05 19:21 ` Stefan Monnier
2022-01-06 18:56 ` Juri Linkov
2022-01-09 22:55 ` Yuan Fu
2022-01-10 8:21 ` Juri Linkov
2022-01-10 18:30 ` Juri Linkov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).