astoff / comint-mime

Display graphics and other MIME attachments in Emacs shells
66 stars 7 forks source link

comint-mime-setup-shell interferes with OSC 7 and mimecat fails to load #22

Closed shipmints closed 1 month ago

shipmints commented 1 month ago

Greetings and thank you for this package and your other work. code-cells is on deck for me also. Trying comint-mime out to see if I can get python-based plots into shell buffers and avoid web-based outputs. (Perhaps some day, a decent web component might live natively in Emacs and we could have a more natural symbiotic Emacs/web ecosystem. I've looked at https://github.com/emacs-eaf but it requires X11 and I'm on a Mac nearly 100% of the time.)

While testing comint-mime on Emacs 29, which now defaults to OSC 7 ANSI escape code directory tracking enabled in comint-mode and using bash with PROMPT_COMMAND appropriately set, I've found that comint-mime-setup-shell is broken in two ways:

While attempting to narrow down what's going on, and failing to completely understand what is actually happening, I was able to force it to work by overriding comint-mime-setup-shell with my own that has only this change which addresses both issues:

(defun comint-mime-setup-shell (&rest _)
...
    (comint-redirect-send-command
     ;; (format " . %s\n" (shell-quote-argument ; BUG: using "." is broken?
     (format "source %s\n" (shell-quote-argument
...

This change will screw up shells that support only .. I don't understand why this works. I tried several attempts to see if there was a race condition or something else that would cause this, to no avail.

Also, there might be a secondary bug. Should there be a progn else clause?

(defun comint-mime-setup-shell (&rest _)
  "Setup code specific to `shell-mode'."
  (if (save-excursion
        (goto-char (field-beginning (point-max) t))
        (not (re-search-forward comint-prompt-regexp nil t)))
      (add-hook 'comint-output-filter-functions 'comint-mime-setup-shell nil t)
    (progn ; *** missing?
      (remove-hook 'comint-output-filter-functions 'comint-mime-setup-shell t)
      (comint-redirect-send-command
       (format " . %s\n" (shell-quote-argument
                          (expand-file-name "comint-mime.sh"
                                            comint-mime-setup-script-dir)))
       nil nil t))))
astoff commented 1 month ago

Sorry for the delay to reply. What does the entire (format ...) for evaluates to? (You can try with C-x C-e.) Whatever the result is, it should be a command your shell (I suppose that's zsh) understands. I'm suspecting it might be related to relative versus absolute file names.

Should there be a progn else clause?

No, see C-h f if RET for more info on the if syntax.

shipmints commented 1 month ago

Using bash over here across all platforms. Old fashioned, perhaps, but reliably ubiquitous. The expansions are nearly identical and the "." should work.

" . /Users/shipmints/.emacs.d/elpa/comint-mime-0.4/comint-mime.sh
"
"source /Users/shipmints/.emacs.d/elpa/comint-mime-0.4/comint-mime.sh
"

While I was typing this and thinking it through, I realized what the issue is, and it is one that will bite other people. I have a bash alias alias .='cd ..' and this is what gets executed by the command. Adding a single backslash avoids invoking the alias. I would happily delete the alias BUT I'd prefer to keep it as an edge case, should this come up again with other programs.

       (format "\\. %s\n" (shell-quote-argument

I tested this just now and it works. I believe the backslash approach is portable and innocuous.

astoff commented 1 month ago

Ah, makes sense! But what does the backslash mean? The trick I know to avoid these problems would be to use

     (format " command . %s\n" (shell-quote-argument

(If you wonder what the space at the beginning means, check out the HISTCONTROL=ignorespace option.)

astoff commented 1 month ago

Or maybe, now that I found this explanation, I think I'm going to pick this one as a less weird alternative:

     (format " '.' %s\n" (shell-quote-argument

If you could do a quick test it would be great. Thanks for figuring it out!

shipmints commented 1 month ago

That works. The backslash syntax is well documented for avoiding alias expansion, too.

shipmints commented 1 month ago

For future readers, it might pay to put in a comment about both the leading space and the quotes.

shipmints commented 1 month ago

Let me know when comint-mime is updated in elpa and I'll remove my workaround. Cheers.

shipmints commented 6 days ago

Quick ping on publishing to [m]elpa when you get a chance. Thank you.

astoff commented 8 hours ago

Done, thanks for the ping.