Add support for displaying various images #2248

arrdem · 2018-03-24T05:07:14Z

This is my second-pass changeset to implement support for embedding images in the REPL. It's probably got some sharp edges - my elisp isn't that good - but it definitely functions as advertised. I'd appreocate criticism on how the handlers should be made more configurable both on the nREPL side of the changeset (clojure-emacs/cider-nrepl#517) and on the emacs side.

Demo

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
All code passes the linter (make lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

xiongtx · 2018-03-24T05:39:53Z

cider-repl.el

                                 (lambda (buffer pprint-out)
                                   (cider-repl-emit-result buffer pprint-out (not after-first-result-chunk))
+                                   (setq after-first-result-chunk t))
+                                 (lambda (buffer value content-type)
+                                   (if-let ((handler (cdr (assoc content-type cider-repl-content-type-handler-alist))))


if-let is obsolete. Emacs ≥26.1 uses if-let*, as does CIDER (via cider-compat.el).

xiongtx · 2018-03-24T05:41:19Z

nrepl-client.el

@@ -764,7 +764,8 @@ to the REPL."
 (defun nrepl-make-response-handler (buffer value-handler stdout-handler
                                           stderr-handler done-handler
                                           &optional eval-error-handler
-                                           pprint-out-handler)
+                                           pprint-out-handler
+                                           content-type-handler)


Docstring should mention this and say what it does.

I love it how ridiculous this function is at that point. :D It's certainly one of the prime candidates for some massive refactorings.

xiongtx · 2018-03-24T05:42:40Z

cider-repl.el

+  (create-image (if datap (base64-decode-string image) image) type datap
+                :margin cider-repl-image-margin))
+
+(setq cider-repl-content-type-handler-alist


defvar/defcustom?

xiongtx · 2018-03-24T05:44:47Z

nrepl-client.el

      (when (buffer-live-p buffer)
        (with-current-buffer buffer
          (when (and ns (not (derived-mode-p 'clojure-mode)))
            (cider-set-buffer-ns ns))))
-      (cond (value
+      (cond ((and content content-type content-type-handler)
+             (funcall content-type-handler buffer content content-type))


content-type-handler is called w/ only 3 args, while the functions in cider-repl-content-type-handler-alist can take 4.

Right. What happens here is that the content-type-handler function is called as (buff content content-type), it does its own internal dispatch, chooses a handler implementatiion (if any) and defers to that handler.

Because of the current mechanics of CIDER's various insertion functions, the arguments &optional show-prefix bol seem to be required in order to ensure that the user's prefix configuration is respected. The precise behavior of the bol flag is ... not obvious so I just retained it blindly.

I'd like to see the way that the REPL handles marks cleaned up to maintain one set of marks per eval requests which would I think simplify some of this logic and fix some interesting behavior where output / errors from threads forked by previous evals can become interleaved with the latest output and its value(s).

Yeah, that'd be nice. Unfortunately there's a ton of legacy in the REPL code - much of it came straight from SLIME and was barely updated afterwards.

xiongtx · 2018-03-24T05:45:39Z

cider-repl.el

+       (cider-repl-emit-result buffer pprint-out (not after-first-result-chunk))
+       (setq after-first-result-chunk t))
+     (lambda (buffer value content-type)
+       (if-let ((handler (cdr (assoc content-type


if-let -> if-let*

xiongtx · 2018-03-24T06:23:43Z

cider-repl.el

+  (create-image (if datap (base64-decode-string image) image) type datap
+                :margin cider-repl-image-margin))
+
+(defun cider-repl-handle-jpeg (buffer image &optional show-prefix bol)


There's a lot of repetition w/ these functions. Seems like a cider-repl-handle-image that dispatches on image type could eliminate that. There's apply-partially if you need it.

xiongtx · 2018-03-24T06:26:18Z

cider-repl.el

+    ("image/jpeg;base64" .  #'cider-repl-handle-jpeg64)
+    ("image/png". #'cider-repl-handle-png)
+    ("image/png;base64" . #'cider-repl-handle-png64))
+  "Association list from content-types to handlers for inserting nREPL


Just alist is fine.

stardiviner · 2018-03-25T09:00:04Z

@arrdem in the screenshot, seems it is gnuplot to convert image. I'm watching this nREPL support image content-type topic. Because I want to add this feature to ob-clojure and ob-clojure-literate
But after reading you two post:

Display images of various format generated by clojure code in dedicated buffers or inline in the REPL #1510 (comment)
Add support for displaying various images #2248
Can't fully understand (not good on Clojure still)
Can you check out ob-clojure.el and ob-clojure-literate.el, and point me out how can I use this new feature? And I want to understand the theory how does it works.
This function should be able to save image object to image file locally into buffer working directory. That's what Org-mode babel graphics result works.

bbatsov · 2018-03-27T12:57:12Z

cider-repl.el

+          (set-marker cider-repl-prompt-start-mark (point) buffer))))
+    (cider-repl--show-maximum-output)))
+
+(defcustom cider-repl-image-margin 10


Probably we should also add some defcustom to turn this functionality off in case someone doesn't like it, or the initial implementation causes problems in some cases.

I agree with this. Should have an option to disable it to avoid early stage problems.

bbatsov · 2018-03-27T13:06:14Z

Overall things look good to me (apart from the failing build).

I'd appreocate criticism on how the handlers should be made more configurable both on the nREPL side of the changeset (clojure-emacs/cider-nrepl#517) and on the emacs side.

I guess one way to make things configurable would be set the configuration on the Emacs side and send it to the middleware as part of some "init" message when the connection is being established.

I dislike in the Emacs handlers that even though they don't have to support text, they current support params that make sense only for text output.

I've got two questions:

What happens now if some expression produces some output and its result is an image.
What happens if the resulting image can't fit in the REPL buffer (e.g. it's too wide)?

I'd be really nice to add support for this for interactive evaluation as well, which probably means some of the image handling code, shouldn't live in the REPL source code. E.g. you can show resulting images in an overlay or a dedicated result buffer, but obviously we can't show them in the minibuffer. :-)

Btw, I'm wondering about the special way you've treated value - is this really necessary? I can image we can both print the result and the image, although I guess that's a matter of personal preferences.

stardiviner · 2018-03-27T13:11:27Z

About image too wide problem, I guess Emacs handle this. Org-mode has similar function to specify image with. I don't know how it is implemented. Even more, the image width can be smarter based on current Emacs window width.
Use code like this to get window width (window-width (selected-window) t)

bbatsov · 2018-03-28T02:18:53Z

One more thing - does clearing output in the REPL work properly for images? This certainly needs to be tested.

Not great but this prevents the pretty-print middleware from interacting with the image / content-type middleware.

…list

zcaudate · 2018-06-24T07:56:26Z

This is really cool. Is is possible to get it working with cider-eval-last-sexp/cider-eval-last-sexp-to-repl?

xiongtx reviewed Mar 24, 2018

View reviewed changes

stardiviner mentioned this pull request Mar 25, 2018

Add support for various image content-types clojure-emacs/cider-nrepl#517

Merged

arrdem force-pushed the feature/images branch from 0b49c6b to 86f5803 Compare March 26, 2018 02:33

bbatsov reviewed Mar 27, 2018

View reviewed changes

arrdem force-pushed the feature/images branch from d070c50 to 7b6809a Compare April 12, 2018 05:17

arrdem added 15 commits April 27, 2018 22:08

Initial cut at image support

0ed1b73

Use content to disambiguate from value

265e6e0

Not great but this prevents the pretty-print middleware from interacting with the image / content-type middleware.

Don't need to substring anymore

1508d28

Get base64 decoding of images working

f7fecad

Format

80038b3

Factor image insertion a bit better, add a customizable margin

eb6f27e

Re-indent for the linter

0242edd

Update CHANGELOG

41efe47

Add to Author list

9e209ef

Add to Author list

467192f

if-let -> if-let*

3cb624f

Factor into defcustoms and some fns

58bbea5

Document the changes to nrepl-make-response-handler

bce72dc

Love to spend my time fixing format issues

0d7db17

Add :type for the linter

a6c0993

arrdem added 3 commits April 27, 2018 22:08

Add a content-type REPL option akin to the pprint option

fb707f5

Update the handler signature to reflect that content-type is now a 2-…

0864d6d

…list

And working again atop 7a838de380f10acf813b1c22175a3f89a5a94956

6baabf7

arrdem force-pushed the feature/images branch from f39ab3d to 6baabf7 Compare April 28, 2018 08:04

arrdem added 7 commits April 28, 2018 01:08

Docstring

964965e

Add another docstring

62bb7f0

Handle content-transfer-encoding magically

82c8f3b

Don't manually manage base64 decoding anymore

f37f51b

Whitespace

e243028

Whitespace

6ca461e

More whitespace

e56ce60

bbatsov merged commit dd83634 into clojure-emacs:master Apr 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for displaying various images #2248

Add support for displaying various images #2248

arrdem commented Mar 24, 2018

xiongtx Mar 24, 2018 •

edited

Loading

xiongtx Mar 24, 2018

arrdem Mar 24, 2018

bbatsov Mar 27, 2018

xiongtx Mar 24, 2018 •

edited

Loading

xiongtx Mar 24, 2018

arrdem Mar 24, 2018

bbatsov Mar 27, 2018

xiongtx Mar 24, 2018

xiongtx Mar 24, 2018 •

edited

Loading

xiongtx Mar 24, 2018

stardiviner commented Mar 25, 2018 •

edited

Loading

bbatsov Mar 27, 2018

stardiviner Mar 27, 2018

bbatsov commented Mar 27, 2018

stardiviner commented Mar 27, 2018

bbatsov commented Mar 28, 2018

zcaudate commented Jun 24, 2018

Add support for displaying various images #2248

Add support for displaying various images #2248

Conversation

arrdem commented Mar 24, 2018

Demo

xiongtx Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiongtx Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiongtx Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stardiviner commented Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 27, 2018

stardiviner commented Mar 27, 2018

bbatsov commented Mar 28, 2018

zcaudate commented Jun 24, 2018

xiongtx Mar 24, 2018 •

edited

Loading

xiongtx Mar 24, 2018 •

edited

Loading

xiongtx Mar 24, 2018 •

edited

Loading

stardiviner commented Mar 25, 2018 •

edited

Loading