astoff / code-cells.el

Emacs utilities for code split into cells, including Jupyter notebooks
GNU General Public License v3.0
187 stars 12 forks source link

Evil Mode: code-cells-convert-ipynb not always called #24

Open tropf opened 10 months ago

tropf commented 10 months ago

Dear Maintainer,

I stumbled across the following strange behavior when using code-cells with evil mode:

Description

When invoking :e! (equivalent to revert-buffer) from evil mode in an .ipynb file, the raw JSON is opened.

Steps to Reproduce

  1. Open any .ipynb file in emacs with code-cells installed and evil mode enabled
  2. The notebook will be opened and converted with jupytext
  3. (here one would do some work)
  4. revert the buffer by typing :e!

Expected Behavior

The buffer is reverted and the converted (readable) notebook is displayed.

Actual Behavior

The buffer is reverted, and the raw .ipynb file is opened, in my case in JSON-mode. It is not human-readable.

Notes

Workaround is to manually call code-cells-convert-ipynb or use revert-buffer instead of evil :e!.

Apparently, calling :e! is not equivalent to calling emacs native revert-buffer. With :e! the hook installed in auto-mode-alist here is not invoked, whereas with revert-buffer it is. (wtf)

I am not sure if this is not the right hook, or if evil is weird. (I would not expect such a bug in a large package as evil.) Unfortunately, I don't know which other hook might be correct. Maybe some emacs wizard can do some digging?

Either way, thanks for the package :)

tropf commented 10 months ago

I did some further digging, evil :e! calls the following code section (upstream here):

(evil-define-command evil-edit (file &optional bang)
  "Open FILE.
If no FILE is specified, reload the current buffer from disk."
  :repeat nil
  (interactive "<f><!>")
  (if file
      (find-file file)
    (revert-buffer bang (or bang (not (buffer-modified-p))) t)))

In particular, it results in a call to revert-buffer t t t. The signature of revert-buffer is:

revert-buffer &optional ignore-auto noconfirm preserve-modes

In particular, preserve-modes is set to t, and the mode is not reinitialized. From emacs docs:

Normally, this command reinitializes the buffer’s major and minor modes using normal-mode. But if preserve-modes is non-nil, the modes remain unchanged.

Hence auto-mode-alist is not invoked, because the mode is left unchanged. Now, a quick-and-dirty workaround is to call:

(add-hook 'after-revert-hook 'code-cells-convert-ipynb)

Now, evil :e! works flawlessly, but emacs vanilla revert-buffer calls code-cells-convert-ipynb twice (once for after-revert-hook, and once for mode-reinitialization), which then gives an error. This error has no effect.

To make both revert-buffer and :e! work, i think one would have to make code-cells-convert-ipynb idempotent: If called multiple times in the same buffer, it should only convert once (and not throw an error on the second call.)

I don't really have experience in that, but will possibly get around to tinkerting.

astoff commented 10 months ago

Okay, thanks for the extra info. I'm not quite sure what do say about this, and at first sight it seems like a problem in evil. Why would people expect :e! to be different from M-x revert-buffer RET yes RET?

Anyway, how about (add-hook 'after-revert-hook 'code-cells-convert-ipynb nil 'local)?

tropf commented 9 months ago

Hi, thanks for the suggestion.

Unfortunately, (add-hook 'after-revert-hook 'code-cells-convert-ipynb nil 'local) does not seem to be sufficient. (Executed with a jupyter notebook opened in Emacs using M-:)

The first :e! works as expected, the second invocation of :e! then shows the JSON again. (WTF?) ...I'm not sure why this is happening.