emacs-evil / evil-magit

Black magic or evil keys for magit
https://github.com/justbur/evil-magit
GNU General Public License v3.0
272 stars 16 forks source link

Can't require evil-magit since Magit switched from magit-popup to Transient #61

Closed bmag closed 5 years ago

bmag commented 5 years ago

Magit has moved from magit-popup to Transient in https://github.com/magit/magit/pull/3728. As part of it the popup commands were renamed to remove the -popup suffix, resulting in the following error when trying to (require 'evil-magit):

Debugger entered--Lisp error: (void-variable magit-dispatch-popup)
  (copy-sequence magit-dispatch-popup)
  (defvar evil-magit-dispatch-popup-backup (copy-sequence magit-dispatch-popup))
  #<subr require>(evil-magit)
  apply(#<subr require> evil-magit)
  require(evil-magit)
  eval((require 'evil-magit) nil)
  eval-expression((require 'evil-magit) nil nil 127)
  funcall-interactively(eval-expression (require 'evil-magit) nil nil 127)
  call-interactively(eval-expression nil nil)
  command-execute(eval-expression)

I assume there's more to fix than just renaming functions, though :(

Silex commented 5 years ago

@tarsius: maybe you could give hints about what needs to be done.

So far I see obvious things like removing -popup everywhere relevant (e.g https://github.com/emacs-evil/evil-magit/blob/master/evil-magit.el#L532-L549).

bmag commented 5 years ago

Found modifying-existing-transients in Transient's manual (also available via info after installing Transient).

tarsius commented 5 years ago

A quick draft is in #62.

justbur commented 5 years ago

I'll have to fix the popups later, but at least the current version of evil-magit will work now.

bmag commented 5 years ago

https://github.com/emacs-evil/evil-magit/pull/63 should fix most of the remapping, but not anything forge-related (i.e. "push" and "pull" remapping). Mostly untested, but seems to work for me.

bmag commented 5 years ago

Most of the changes are of the sort:

;; old
(magit-change-popup-key 'magit-tag-popup :actions (string-to-char "k") (string-to-char "x"))
;; new
(transient-suffix-put 'magit-tag "k" :key "x")

But I'm not sure about the "revert" remapping. It contains "V" twice, and it isn't clear how to distinguish them anymore, so I just call transient-suffix-put twice (1st time catches the 1st "V", 2nd time catches the 2nd "V" because 1st isn't "V" anymore at this point):

;; old
(magit-change-popup-key 'magit-revert-popup :actions (string-to-char "V") (string-to-char "O"))
(magit-change-popup-key 'magit-revert-popup :sequence-actions (string-to-char "V") (string-to-char "O"))
;; new
(transient-suffix-put 'magit-revert "V" :key "O")
(transient-suffix-put 'magit-revert "V" :key "O")

@tarsius is there a better way?

For reference, magit-revert definition:

;;;###autoload (autoload 'magit-revert "magit-sequence" nil t)
(define-transient-command magit-revert ()
  "Revert existing commits, with or without creating new commits."
  :man-page "git-revert"
  :value '("--edit")
  ["Arguments"
   :if-not magit-sequencer-in-progress-p
   (magit-cherry-pick:--mainline)
   ("-e" "Edit commit message"       ("-e" "--edit"))
   ("-E" "Don't edit commit message" "--no-edit")
   ("=s" magit-merge:--strategy)
   ("-s" "Add Signed-off-by lines"   ("-s" "--signoff"))
   (5 magit:--gpg-sign)]
  ["Actions"
   :if-not magit-sequencer-in-progress-p
   ("V" "Revert commit(s)" magit-revert-and-commit)
   ("v" "Revert changes"   magit-revert-no-commit)]
  ["Actions"
   :if magit-sequencer-in-progress-p
   ("V" "Continue" magit-sequencer-continue)
   ("s" "Skip"     magit-sequencer-skip)
   ("a" "Abort"    magit-sequencer-abort)])
tarsius commented 5 years ago

is there a better way?

Not yet. The plan is to support "paths" (a list of integers, i.e. a position in the tree) as LOC (which is why that argument isn't called just KEY).

tarsius commented 5 years ago

Oh, actually there is. LOC can also be a command.