[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#60426: 29.0.60; Binding to outline-minor-mode-cycle-map correctly is
From: |
Rah Guzar |
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 |
User-agent: |
mu4e 1.8.13; emacs 29.0.60 |
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@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