GNU bug report logs - #60426
29.0.60; Binding to outline-minor-mode-cycle-map correctly is unintuitive and hard

Previous Next

Full log

View this message in rfc822 format

From: Rah Guzar <rahguzar <at> zohomail.eu>
To: Juri Linkov <juri <at> linkov.net>
Cc: 60426 <at> debbugs.gnu.org
Subject: bug#60426: 29.0.60; Binding to outline-minor-mode-cycle-map correctly is unintuitive and hard
Date: Wed, 11 Jan 2023 17:29:22 +0100

Thanks a lot! The patch look good to me. Minor quibbles:
1. I think  in the NEWS entry, "font-lock text property 'keymap'" should
   probably just be "text property 'keymap'" i.e. why mention font-lock?

2. Since the implementation of `outline-minor-mode-cycle--bind` can change,
   it is better to change the order of arguments and make `map` an actual
   third argument. It is awkward to pass `nil` as the first argument and I
   think that way the actual map key is bound to can be treated as an
   implementation detail with `outline-minor-mode-cycle--bind` the official
   way of binding keys on headings.

Thanks again!

Juri Linkov <juri <at> linkov.net> writes:

>>>   (outline-minor-mode-cycle--bind map (kbd "TAB") #'outline-cycle)
>>>
>>> Maybe this requirement should be documented in the docstring
>>> and mentioned in NEWS?
>>
>> I think this is a good idea since people who have already keys bound
>> on outline-minor-mode-cycle-map are likely to be as surprised as I was.
>>
>> But I also think that it is more important to mention this in the
>> docstring for outline-minor-mode-cycle-map. Since its current
>> docstring "Keymap used by outline-minor-mode-cycle." is misleading in
>> my opinion, especially since any key there is going to be active
>> if outline-minor-mode is on, regardless of the value of
>> `outline-minor-mode-cycle`.
>
> Thanks for suggestions.  The following patch addresses all the points
> that have been raised, except obsoleting `outline-minor-mode-cycle-map`.
> It's more convenient to bind keys in this map since `outline-minor-mode-map`
> is not explicitly defined unlike `outline-mode-map`.
>
>> Also I was confused by the two hyphens in the name
>> outline-minor-mode-cycle--bind and thought that it was a private
>> function even though in retrospect they are in the wrong place
>> for it to be private.
>
> Two hyphens in the name rather indicated that the implementation
> is subject to change without notice - an assumption that is true
> for internal functions.  So now I'm not sure if adding an alias
> without two hyphens is worth to do.
>
> [2. text/x-diff; outline-minor-mode-cycle--bind.patch]
> diff --git a/etc/NEWS b/etc/NEWS
> index a28f5c9a65a..6ae79e6cebc 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -350,6 +350,15 @@ next button, even if the mode has bound it to something else.  This
>  also means that 'TAB' on a button in an 'outline-minor-mode' heading
>  will move point instead of collapsing the outline.
>
> +---
> +** 'outline-minor-mode-cycle-map' is now parent of 'outline-minor-mode'.
> +Instead of adding font-lock text property 'keymap' with
> +'outline-minor-mode-cycle' on outline headings in 'outline-minor-mode',
> +the keymap 'outline-minor-mode-cycle' is now active in the whole buffer.
> +But keybindings in 'outline-minor-mode-cycle' are still take effect
> +only on outline headings because they are bound with the help of
> +'outline-minor-mode-cycle--bind' that checks if point is on a heading.
> +
>  ---
>  ** 'Info-default-directory-list' is no longer populated at Emacs startup.
>  If you have code in your init file that removes directories from
> diff --git a/lisp/outline.el b/lisp/outline.el
> index 91f6040687b..26e41735a01 100644
> --- a/lisp/outline.el
> +++ b/lisp/outline.el
> @@ -210,7 +210,12 @@ outline-minor-mode-cycle-filter
>
>  (defvar outline-minor-mode-cycle)
>  (defun outline-minor-mode-cycle--bind (map key binding &optional filter)
> -  (define-key map key
> +  "Define KEY as BINDING in MAP using FILTER.
> +The key takes effect only on the following conditions:
> +`outline-minor-mode-cycle' is non-nil, point is located on the heading line,
> +FILTER or `outline-minor-mode-cycle-filter' is nil or returns non-nil.
> +The argument MAP is optional and defaults to `outline-minor-mode-cycle-map'."
> +  (define-key (or map outline-minor-mode-cycle-map) key
>      `(menu-item
>        "" ,binding
>        ;; Filter out specific positions on the heading.
> @@ -227,8 +232,16 @@ 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)
> +    (keymap-set map "<left-margin> <mouse-1>" 'outline-cycle)
> +    (keymap-set map "<right-margin> <mouse-1>" 'outline-cycle)
> +    (keymap-set map "<left-margin> S-<mouse-1>" 'outline-cycle-buffer)
> +    (keymap-set map "<right-margin> S-<mouse-1>" 'outline-cycle-buffer)
>      map)
> -  "Keymap used by `outline-minor-mode-cycle'.")
> +  "Keymap used as a parent of the `outline-minor-mode' keymap.
> +It contains key bindings that can be used to cycle visibility.
> +The recommended way to bind keys is with `outline-minor-mode-cycle--bind'
> +when the key should be enabled only when `outline-minor-mode-cycle' is
> +non-nil and point is located on the heading line.")
>
>  (defvar outline-mode-map
>    (let ((map (make-sparse-keymap)))
> @@ -518,10 +532,6 @@ outline-minor-mode
>    :keymap (define-keymap
>              :parent outline-minor-mode-cycle-map
>              "<menu-bar>" outline-minor-mode-menu-bar-map
> -            "<left-margin> <mouse-1>" 'outline-cycle
> -            "<right-margin> <mouse-1>" 'outline-cycle
> -            "<left-margin> S-<mouse-1>" 'outline-cycle-buffer
> -            "<right-margin> S-<mouse-1>" 'outline-cycle-buffer
>              (key-description outline-minor-mode-prefix) outline-mode-prefix-map)
>    (if outline-minor-mode
>        (progn

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.