C-<up> binded to merlin-type-enclosing-go-up #129

Closed
bobot opened this Issue Nov 2, 2013 · 7 comments

Projects

None yet

4 participants

@bobot
bobot commented Nov 2, 2013

Hi,

Just after an update I fall into this bug:

Actions to reproduce the bug:

  • load an .ml with the merlin mode activated (opam version 1.4.1)
  • go somewhere in the buffer
  • type C-<up>

Current result:

  • the cursor doesn't move

Expected result:

  • the cursor move "fastly" up

The commit 6610145 binded C-<up> to merlin-type-enclosing-go-up, which seems to do nothing before C-t ('merlin-type-expr) is run, instead of merlin-prev-phrase-or-go-up. Normally C-<up> is binded to backward-paragraph.

Possible solution: keep the default binding for C-<up>

Best solution: Just after a C-t make C-<up> and C-<down> control the typed-region, after any other keystrokes exit this behavior. Outside the "C-t-mode" C-<up> is backward-paragraph or something better.

Thx

@trefis
Contributor
trefis commented Nov 2, 2013

As you noticed we removed the merlin-phrase-next/prev bindings from C-down/up.

This is not a bug but a deliberate choice, the reason for it are :
1/ we do not like functions (or in that case bindings) whose behaviour depends on the context
2/ there already are bindings to these functions on C-c C-n/p (which feels - to the non-emacs user that I am - more emacsy)
3/ You can always redefine the bindings to your liking.

I would not be for reintroducing them. Also, handling the context in the way you presented it adds a lot of complexity for (IMHO) not much in the end.

@trefis trefis closed this Nov 2, 2013
@bobot
bobot commented Nov 3, 2013

Ok its your choice, but:

  1. auto-complete which is used for completion uses bindings that depend on the context
  2. C-<up> is present in numerous tutorials. Less typing for an usual command is rational. And backward-paragraph is more often used than merlin-type-enclosing-go-up.
  3. For reference here what can be used in a .emacs to recover the default:
(eval-after-load 'merlin
  '(progn (define-key merlin-mode-map (kbd "C-<up>") nil)
          (define-key merlin-mode-map (kbd "C-<down>") nil)))
@trefis
Contributor
trefis commented Nov 3, 2013

Ok its your choice, but:

  1. auto-complete which is used for completion uses bindings that depend on the context

Right, but auto-complete is the typical example we don't want to follow.

  1. C- is present in numerous tutorials.

I didn't know that, as I said I am not an emacs user. I guess it would make sense to reintroduce the binding then. I'll let @asmanur or @def-lkb decide.

Thank you.

@trefis trefis reopened this Nov 3, 2013
@bobot
bobot commented Nov 3, 2013

Thanks for taking my remarks into account.

I didn't know how to make the contextual command, so I learned from popup.el. It can be useful for someone else even if the result is not very robust. After mymerlin-type-enclosing C-<up>, C-<down> move to bigger or smaller expression and C-<left>, C-<right> move to the expression on the left or right (not robust, ie syntaxic not semantic) and each time print the type. Funny but not robust.

(defun my-read-key-sequence (keymap &optional prompt timeout)
  (catch 'timeout
    (let ((timer (and timeout
                      (run-with-timer timeout nil
                                      (lambda ()
                                        (if (zerop (length (this-command-keys)))
                                            (throw 'timeout nil))))))
          (old-global-map (current-global-map))
          (temp-global-map (make-sparse-keymap))
          (overriding-terminal-local-map (make-sparse-keymap)))
      (substitute-key-definition 'keyboard-quit 'keyboard-quit
                                 temp-global-map old-global-map)
      (define-key temp-global-map [menu-bar] (lookup-key old-global-map [menu-bar]))
      (define-key temp-global-map [tool-bar] (lookup-key old-global-map [tool-bar]))
      (set-keymap-parent overriding-terminal-local-map keymap)
      (if (current-local-map)
          (define-key overriding-terminal-local-map [menu-bar]
            (lookup-key (current-local-map) [menu-bar])))
      (unwind-protect
          (progn
            (use-global-map temp-global-map)
            (clear-this-command-keys)
            (with-temp-message prompt
              (read-key-sequence nil)))
        (use-global-map old-global-map)
        (if timer (cancel-timer timer))))))


(defun* my-menu-event-loop (keymap)
  (block nil
    (while t
      (setq key (my-read-key-sequence keymap "type:"))
      (if (null key)
          (keyboard-quit)
        (if (eq (lookup-key (current-global-map) key) 'keyboard-quit)
            (keyboard-quit))
        (setq binding (lookup-key keymap key))
        (cond
         ((eq binding 'up)
          (merlin-type-enclosing-go-up))
         ((eq binding 'down)
          (merlin-type-enclosing-go-down))
         ((eq binding 'left)
            (let ((data (elt merlin-enclosing-types merlin-enclosing-offset)))
              (if (cddr data)
                  (progn
                    (goto-char (car (cdr data)))
                    (backward-word)
                    (forward-word)
                    (merlin-type-enclosing))
                )))
         ((eq binding 'right)
            (let ((data (elt merlin-enclosing-types merlin-enclosing-offset)))
              (if (cddr data)
                  (progn
                    (goto-char (cdr (cdr data)))
                    (forward-word)
                    (backward-word)
                    (merlin-type-enclosing))
                    )))
         ((commandp binding)
          (call-interactively binding))
         (t
          (keyboard-quit)))))))

(defvar my-type-enclosing-map
  (let ((test-map (make-sparse-keymap)))
    (define-key test-map (kbd "C-<up>") 'up)
    (define-key test-map (kbd "C-<down>") 'down)
    (define-key test-map (kbd "C-<left>") 'left)
    (define-key test-map (kbd "C-<right>") 'right)
    test-map))


(defun mymerlin-type-enclosing ()
  "Print the type of the expression under point (or of the region, if it exists)."
  (interactive)
  (save-excursion
    (merlin-sync-to-point)
    (if (region-active-p)
        (merlin-type-region)
      (if (merlin-type-enclosing-query)
          (progn (merlin-type-enclosing-go-up)
                 (my-menu-event-loop my-type-enclosing-map)
                 )))))
@smagill
smagill commented Feb 7, 2014

Just to follow up on this, I was also surprised that merlin-mode overrides the C-up and C-down behavior. I rely on the default bindings for those key combinations quite frequently for navigation in a variety of source languages (and they seem to be behave consistently regardless of file type). That they "broke" almost made me give up on using merlin until I found this bug report with the suggested fix.

@trefis
Contributor
trefis commented Feb 7, 2014

Forgot about this one. Apparently #170 doesn't list every keybinding we stole. I'll be sure to remember it when I fix the ones of #170.

To answer François (which we should have done months ago) I personally don't like the idea of contextual keybindings (but it may be the natural way to do things in emacs? Idk), and it makes the code more "complex" ; so I'd rather avoid it.
Someone disagrees?

@trefis trefis added the emacs mode label Feb 7, 2014
@let-def
Contributor
let-def commented Jul 20, 2014

@gsg introduced an overlay map in 1a4e2dc fixing this problem.

@let-def let-def closed this Jul 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment