casouri / eldoc-box

childframe doc for eglot and anything that uses eldoc
361 stars 26 forks source link

Add keymap for eldoc-box #73

Closed dalugm closed 1 year ago

dalugm commented 1 year ago

On Windows, eldoc-box's childframe can not be selected by cursor, and I can't scroll the childframe with mouse. So I write a keymap for it.

dalugm commented 1 year ago

Or should we add a customizable option like eldoc-box-use-extra-commands-map, so users needn't to write

(set-keymap-parent eldoc-box-hover-mode-map eldoc-box-extra-commands-map)

in their config.

Just like this:

(defcustom eldoc-box-use-extra-commands-map nil
  "If non-nil, use `eldoc-box-extra-commands-map' on
`eldoc-box-hover-mode' and `eldoc-box-hover-at-point-mode'."
  :type 'boolean)

(when eldoc-box-use-extra-commands-map
  (set-keymap-parent eldoc-box-hover-mode-map eldoc-box-extra-commands-map)
  (set-keymap-parent eldoc-box-hover-at-point-mode-map eldoc-box-extra-commands-map))

Well, I guess this is not a good idea. It might be better to leave eldoc-box-extra-commands-map as a reference.

dalugm commented 1 year ago

I noticed a problem:

For example, my eldoc-box' config is

(use-package eldoc-box
  :when (display-graphic-p)
  :hook (eldoc-mode . eldoc-box-hover-mode)
  :custom
  (eldoc-box-only-multi-line t)
  (eldoc-box-clear-with-C-g t)
  :bind (("C-c h h" . eldoc-box-help-at-point)
         (:map eldoc-box-hover-mode-map
               ("M-n" . eldoc-box-scroll-up)
               ("M-p" . eldoc-box-scroll-down)
               ("M-a" . eldoc-box-beginning)
               ("M-e" . eldoc-box-end))
         (:map eldoc-box-hover-at-point-mode-map
               ("M-n" . eldoc-box-scroll-up)
               ("M-p" . eldoc-box-scroll-down)
               ("M-a" . eldoc-box-beginning)
               ("M-e" . eldoc-box-end))))

When I run eval-expression (press M-:), the read--expression-map will be overwritten by eldoc-box-hover-mode-map:

Press M-p to find previous history command, Emacs runs eldoc-box-scroll-down instead of previous-history-element. Shouldn't eldoc-box-hover-/at-point-mode-map be disabled when eldoc-box's frame is invisible?

Any ideas to fix this? Or is this a problem? thx ;)

casouri commented 1 year ago

Huuuuuuh. Minor mode map like eldoc-box-hover-mode-map should be overridden by whatever map the mini buffer is using. If it's not happening, something's probably wrong. But I can't think of anything on top of my head.

dalugm commented 1 year ago

Huuuuuuh. Minor mode map like eldoc-box-hover-mode-map should be overridden by whatever map the mini buffer is using. If it's not happening, something's probably wrong. But I can't think of anything on top of my head.

I guess that's because eldoc-box's keymap takes over the corresponding key, just like if you enabled yasnippet-mode, when you press tab, it will trigger yas-expand instead insert tab char.

Do you think add a minibuffer's hook is a good idea? I can write like this to avoid eldoc-box's keymap to override minibuffer's keymap:

(defun eldoc-box--minibuffer-setup ()
  "Setup the minibuffer for `eldoc-box'."
  (cond
   (eldoc-box-hover-mode
    (eldoc-box-hover-mode -1))
   (eldoc-box-hover-at-point-mode
    (eldoc-box-hover-at-point-mode -1))))

(add-hook 'minibuffer-setup-hook #'eldoc-box--minibuffer-setup)

But I think the best solution should be eldoc-box's keymap only avaiable when eldoc-box's frame is visible, do you have any ideas to achieve this?

dalugm commented 1 year ago

For now I'm using minor-mode-overriding-map-alist, I think this should be a better solution.

I add a new option eldoc-box-use-extra-commands-map, when it's non-nil, eldoc-box-extra-commands-map will be activated when eldoc-box's doc frame is visible, and deactived when the frame hide.

It's not as aggressive as the previous approach, and I think this should be the solution. Do you have any opinions? I'd like to hear it :)

dalugm commented 1 year ago

ping, it has been two weeks, and it works fine on my machine.

Are there any other work needed?

casouri commented 1 year ago

Thanks for reminding me. I sometimes miss messages :-(

The idiomatic way to use minor-mode-overriding-map-alist is to use a variable to control turning on and off a keymap, rather than adding and removing the entry from the alist. You can just set the variable to t/nil when the frame is visible/invisible.

Ok, I think I know why your old code have conflict with minibuffer: you enables eldoc-box-hover-mode whenever eldoc-mode is on. The minibuffer has eldoc-mode on, which turns on eldoc-box in the minibuffer, so eldoc-box's map overrides minibuffer's.

I like the old approach better, maybe you can conditionally turn on eldoc-box-hover-mode (ie, don't turn on when in minibuffer) in eldoc-mode-hook?

dalugm commented 1 year ago

The idiomatic way to use minor-mode-overriding-map-alist is to use a variable to control turning on and off a keymap, rather than adding and removing the entry from the alist. You can just set the variable to t/nil when the frame is visible/invisible.

I didn't catch it. minor-mode-overriding-map-alist is an alist, when we want to enable keymap locally, we have to change the alist. How can we turn on and off a keymap by setting a variable to it?

And we're using a variable eldoc-box-use-extra-commands-map to control whether to change minor-mode-overriding-map-alist or not.

maybe you can conditionally turn on eldoc-box-hover-mode (ie, don't turn on when in minibuffer) in eldoc-mode-hook?

Did you mean the method I mentioned above?

(defun eldoc-box--minibuffer-setup ()
  "Setup the minibuffer for `eldoc-box'."
  (cond
   (eldoc-box-hover-mode
    (eldoc-box-hover-mode -1))
   (eldoc-box-hover-at-point-mode
    (eldoc-box-hover-at-point-mode -1))))

(add-hook 'minibuffer-setup-hook #'eldoc-box--minibuffer-setup)

Actually, I like current approach more. If using a hook to control whether enable eldoc-box-hover[-at-point]-mode, I'm afraid there will be many cases to deal with, that's why I abandon it. When using minor-mode-overriding-map-alist, we only need to care about whether the childframe is visible.

casouri commented 1 year ago

I didn't catch it. minor-mode-overriding-map-alist is an alist, when we want to enable keymap locally, we have to change the alist. How can we turn on and off a keymap by setting a variable to it?

The key of the alist is a variable, and if that variable is nil, the keymap is not activated, if it's non-nil, the keymap is activated. So you can control enabling the keymap by setting that variable rather than adding/removing the keymap from the alist.

Did you mean the method I mentioned above?

No, I mean changing :hook (eldoc-mode . eldoc-box-hover-mode) to only enable eldoc-box-hover-mode when you are not in a minibuffer. Pair that with the very first version where we simply set the parent of a eldoc-box map to extra-commands-map. So this is what you do: define a variable eldoc-box--visible-frame-map-active-p; in eldoc-box-hover/at-point-mode: add (eldoc-box--visible-frame-map-active-p . keymap) to minor-mode-map-alist; when frame is visible, set eldoc-box--visible-frame-map-active-p to t, and vice versa.

If you really like what you have now, I'm fine with it. Though we should use a more descriptive name for eldoc-box-use-extra-commands-map, maybe eldox-box-visible-frame-map.

dalugm commented 1 year ago

The key of the alist is a variable, and if that variable is nil, the keymap is not activated, if it's non-nil, the keymap is activated. So you can control enabling the keymap by setting that variable rather than adding/removing the keymap from the alist.

Oh, I thought the VARIABLE in alist (VARIABLE . KEYMAP) can be minor mode's name (when enabled t, when disabled nil) only. But I think always keep an alist of eldoc-box's keymap in minor-mode-overriding-map-alist is not a good idea. I see many other packages are changing minor-mode-overriding-map-alist directly.

https://github.com/emacs-mirror/emacs/blob/master/lisp/minibuffer.el#L2698-L2699

No, I mean changing :hook (eldoc-mode . eldoc-box-hover-mode) to only enable eldoc-box-hover-mode when you are not in a minibuffer

I guess this is my config's problem. But when I'm using eldoc-box, I set eldoc-box-only-multi-line to t, I don't want to abandon minibuffer too :(

define a variable eldoc-box--visible-frame-map-active-p; in eldoc-box-hover/at-point-mode: add (eldoc-box--visible-frame-map-active-p . keymap) to minor-mode-map-alist; when frame is visible, set eldoc-box--visible-frame-map-active-p to t, and vice versa.

This might be a better solution. But it wrote more codes and we had to maintain two keymap, if this is the idiomatic way I'd like to change like this.

dalugm commented 1 year ago

Emmm, I think using minor-mode-map-alist is not a good idea.

After added two default keymap for eldoc-box-hover[-at-point]-mode, Emacs will put eldoc-doc-hover-mode in the VARIABLE place in alist (VARIABLE . KEYMAP). And in my config, it will always be t. If I want to set the VARIABLE to eldoc-box-use..., I have to set it explicitly like this:

(push `(eldoc-box-use-extra-commands-map . ,keymap) minor-mode-map-alist)

I don't think it's better than setting minor-mode-overriding-map-alist.

Anyway, thanks for your patience, I still have a lot to learn...