Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work/47display images #48

Merged
merged 12 commits into from
Feb 8, 2024
270 changes: 268 additions & 2 deletions adoc-mode.el
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,33 @@ Also used to delimit the scan for the end delimiter."
:group 'adoc
:package-version '(adoc-mode . "0.8.0"))

(defcustom adoc-max-image-size nil
"Maximum width and height for displayed inline images.
This variable may be nil or a cons cell (MAX-WIDTH . MAX-HEIGHT).
When nil, use the actual size. Otherwise, use ImageMagick to
resize larger images to be of the given maximum dimensions. This
requires Emacs to be built with ImageMagick support."
:group 'adoc
:package-version '(markdown-mode . "0.8.0")
Copy link
Owner

@bbatsov bbatsov Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should say adoc-mode here. :-) And I guess the version will be 0.9.

Copy link
Collaborator Author

@TobiasZawada TobiasZawada Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbatsov I don't get the integration errors locally. Do you know what is happening over there on the server? I already tried to require mouse and image in the case that the server does not have a graphical environment and does not load these automatically. Currently, I can only test with Emacs 29.1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should say adoc-mode here. :-) And I guess the version will be 0.9.

Done.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbatsov I don't get the integration errors locally. Do you know what is happening over there on the server? I already tried to require mouse and image in the case that the server does not have a graphical environment and does not load these automatically. Currently, I can only test with Emacs 29.1.

Perhaps Emacs that use for the CI is not compiled with support for images? I think even in the terminal environment that's some optional functionality that probably doesn't make sense for most Emacs packages.

:type '(choice
(const :tag "Use actual image width" nil)
(cons (choice (sexp :tag "Maximum width in pixels")
(const :tag "No maximum width" nil))
(choice (sexp :tag "Maximum height in pixels")
(const :tag "No maximum height" nil)))))

(defcustom adoc-show-images-at-startup t
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from my remark about the name (e.g. I'd prefer adoc-display-images), this should also have a :package-version property. It can also be used for the toggle function (it can check the value and flip it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:package-version done (not yet commited).
I am not so sure about adoc-display-images. I really meant it as adoc-display-images-at-startup. Afterwards that variable does not make so much sense since the user has the possibility to switch the display state of each single image through the context menu individually. If she wants a clean state she can use the functions adoc-display-images and adoc-toggle-images.

But I am not such a great User-Experience guy. So maybe you can convince me if you give me a strong reason.

"Run `adoc-display-images' in `adoc-mode'."
:group 'adoc
:type 'booleanp)

(defcustom adoc-use-local-context-menu t
"Activate context menu in adoc-mode.
If deactivated, you can still globally activate
the context menu with `context-menu-mode'."
:group 'adoc
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add :package-version here as well.

:type 'booleanp)


;;;; faces / font lock
(define-obsolete-face-alias 'adoc-orig-default 'adoc-align-face "23.3")
Expand Down Expand Up @@ -2878,6 +2905,238 @@ Is influenced by customization variables such as `adoc-title-style'."))))
(adoc-tempo-define "adoc-pass-+++" '("+++" (r "text" text) "+++") nil adoc-help-pass-+++)
(adoc-tempo-define "adoc-pass-$$" '("$$" (r "text" text) "$$") nil adoc-help-pass-$$)
; backticks handled in tempo-template-adoc-monospace-literal

;;;; Images
;; Disclaimer: Most of the following stuff is copied from `markdown-mode' and adapted to `adoc-mode'.
(require 'url-parse)

(defvar adoc-inline-image-overlays nil)
(make-variable-buffer-local 'adoc-inline-image-overlays)

(defcustom adoc-display-remote-images nil
"If non-nil, download and display remote images.
See also `adoc-inline-image-overlays'.

Only image URLs specified with a protocol listed in
`adoc-remote-image-protocols' are displayed."
:group 'markdown
:type 'boolean)

(defcustom adoc-remote-image-protocols '("https")
"List of protocols to use to download remote images.
See also `adoc-display-remote-images'."
:group 'markdown
:type '(repeat string))

(defvar adoc--remote-image-cache
(make-hash-table :test 'equal)
"A map from URLs to image paths.")

(defun adoc--get-remote-image (url)
"Retrieve the image path for a given URL."
(or (gethash url adoc--remote-image-cache)
(let ((dl-path (make-temp-file "adoc-mode--image")))
(require 'url)
(url-copy-file url dl-path t)
(puthash url dl-path adoc--remote-image-cache))))

(defconst adoc-re-image "\\<image::?\\(\\_<[^]]+\\)\\(\\[[^]]*\\]\\)"
"Regexp matching block- and inline-images.")

(defvar adoc-image-overlay-functions nil
"Functions called after the creation of an image overlay.
Each function is called with the created overlay as argument.")

(defun adoc-create-image-overlay (file start end)
"Create image overlay with START and END displaying image FILE."
(when (not (zerop (length file)))
(unless (file-exists-p file)
(when (and adoc-display-remote-images
(member (downcase (url-type (url-generic-parse-url file)))
adoc-remote-image-protocols))
(setq file (adoc--get-remote-image file))))
(when (file-exists-p file)
(let* ((abspath (if (file-name-absolute-p file)
file
(concat default-directory file)))
(image
(if (and adoc-max-image-size
(image-type-available-p 'imagemagick))
(create-image
abspath 'imagemagick nil
:max-width (car adoc-max-image-size)
:max-height (cdr adoc-max-image-size))
(create-image abspath))))
(when image
(let ((ov (make-overlay start end)))
(overlay-put ov 'adoc-image t)
(overlay-put ov 'display image)
(overlay-put ov 'face 'default)
(run-hook-with-args 'adoc-image-overlay-functions ov)))))))

(defun adoc-display-images ()
"Add inline image overlays to image links in the buffer.
This can be toggled with `adoc-toggle-inline-images'
or \\[adoc-toggle-inline-images]."
(interactive)
(unless (display-images-p)
(error "Cannot show images"))
(adoc-remove-images)
(save-excursion
(save-restriction
(widen)
(goto-char (point-min))
(while (re-search-forward adoc-re-image nil t)
(let ((start (match-beginning 0))
(end (match-end 0))
(file (match-string-no-properties 1)))
(adoc-create-image-overlay file start end)
)))))

(defun adoc-image-overlays (&optional begin end)
"Return list of image overlays in region from BEGIN to END.
BEGIN and END default to the buffer boundaries
ignoring any restrictions."
(save-restriction
(widen)
(unless begin (setq begin (point-min)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use let binding here. The usage of setq for introducing local bindings is mostly an antipatern in Elisp.

Copy link
Collaborator Author

@TobiasZawada TobiasZawada Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is no local binding introduced through the setq. The local binding is already introduced through the function argument begin. The setq just modifies its value.

What you get through the additional let-form is an superfluous local variable begin hiding the binding of the other local begin.

This also is reflected by one more statement in the byte-code of the version with let:

Version with setq:

(with-temp-buffer
  (disassemble (byte-compile
		;;;; The function:
		(lambda (&optional my-unin)
		  (unless my-unin (setq my-unin 1))
		  (message "my-unin: %s" my-unin) ;; Do just something.
		  )
		)
	       ;;;;;;;;;;
	       (current-buffer))
  (buffer-string))

Generated Byte-Code of the version with setq:

byte code:
  args: (&optional my-unin)
0	varref	  my-unin
1	goto-if-not-nil 1
4	constant  1
5	varset	  my-unin
6:1	constant  message
7	constant  "my-unin: %s"
8	varref	  my-unin
9	call	  2
10	return	  

Version with let:

(with-temp-buffer
  (disassemble (byte-compile
		;;;; The function:
		(lambda (&optional my-unin)
		  (let ((my-unin (or my-unin 1)))
		    (message "my-unin: %s" my-unin) ;; Do just something.
		    ))
		)
	       ;;;;;;;;;;
	       (current-buffer))
  (buffer-string))

Generated Byte-Code of the version with let:

byte code:
  args: (&optional my-unin)
0	varref	  my-unin
1	goto-if-not-nil-else-pop 1
4	constant  1
5:1	varbind	  my-unin
6	constant  message
7	constant  "my-unin: %s"
8	varref	  my-unin
9	call	  2
10	unbind	  1
11	return	  

As you see it uses varbind instead of varset and therefore needs an additional unbind. Even if it is not visible in the code it also has more stack usage through the additional local variable.

So, now I really need to do my regular work ;-).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad - too much multitasking this morning. :D I'd still write something like (setq begin (or begin (point-min))), as unless feels like an overkill for this usecase, but I'm fine either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(setq begin (or begin (point-min))) has the superfluous assignment (setq begin begin).
(unless begin (setq begin (point-min))) does not have that assignment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads better to me regardless, but it's not something I feel strongly about. Feel free to keep it as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both variants have almost the same frequency in /usr/share/emacs/29.1/lisp. Since I am lazy, I do not change it. Furthermore, the statement (unless begin (setq begin ...)) is more straight-forward than (setq begin (or begin ...)). The first version says explicitly what is done while the second version is more like a trick.

(unless end (setq end (point-max)))
(let (image-overlays)
(dolist (ov (overlays-in begin end))
(when (overlay-get ov 'adoc-image)
(push ov image-overlays)))
image-overlays)))

(defun adoc-remove-images ()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps adoc-remove-inline-images?

"Remove inline image overlays from image links in the buffer.
This can be toggled with `adoc-toggle-images'
or \\[adoc-toggle-images]."
(interactive)
(let ((image-list (adoc-image-overlays)))
(when image-list
(dolist (ov image-list)
(delete-overlay ov))
t)))

(defun adoc-toggle-images ()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be adoc-toggle-inline-images or adoc-toggle-display-images.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This affects block images as well as inline images. Therefore I removed the "inline".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a block image?

Copy link
Collaborator Author

@TobiasZawada TobiasZawada Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See section Block Image Macro in the spec. And there is also the description of the Inline Image Macro.

Copy link
Owner

@bbatsov bbatsov Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you mean the AsciiDoc spec - for me "inline" images are those that we display inline in Emacs. :-) Anyways, I guess we can avoid "inline" in the names, as long as they are consistent across all functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing up this topic. Otherwise clarification would be impossible.

In the source code of adoc-mode.el the word "inline" is consistently used in the sense of the Adoc-specification (as the opposite of "block").

"Toggle the display of images."
(interactive)
(unless (adoc-remove-images)
(adoc-display-images)
))

(defmacro adoc-with-point-at-event (location &rest body)
"Execute BODY like `progn'.
If LOCATION is an event run BODY in the event's buffer
and set LOCATION to the event position.

If LOCATION is number or marker just run BODY with
LOCATION untouched.

Otherwise set LOCATION to current point."
(declare (indent 1) (debug (symbolp form body)))
(let ((posn (make-symbol "posn")))
`(if (mouse-event-p ,location)
(let* ((,posn (event-start ,location)))
(with-current-buffer (window-buffer (posn-window ,posn))
(setq ,location (posn-point ,posn))
,@body))
(unless (number-or-marker-p ,location)
(setq ,location (point)))
,@body)))

(defun adoc-bounds-of-image-link-at (&optional location)
"Get bounds of image link at LOCATION.
Return \\(START . END) giving the start and the end
positions of the image link found.

If an image link is identified `match-data' is set as follows:
0. whole link inclusive \"image::?\" and attributes
1. image path
2. bracketed attribute list."
(adoc-with-point-at-event location
(save-excursion
(when location (goto-char location))
(setq location (point))
(if (looking-at "[[:alpha:]]*::?\\_<")
(skip-chars-backward "[:alpha:]")
(re-search-backward "\\_<image:" (line-beginning-position) t))
(when (looking-at adoc-re-image)
(cons (match-beginning 0) (match-end 0))))))

(cl-defstruct adoc-image-link
"Data from an image link."
begin end uri begin-uri end-uri begin-attributes end-attributes)

(defun adoc-image-link-at (&optional location)
"Return image link at LOCATION if there is one.
LOCATION can be a buffer position or a mouse event.
It defaults to \\(point)."
(adoc-with-point-at-event location
(when (adoc-bounds-of-image-link-at location)
(make-adoc-image-link
:begin (match-beginning 0)
:end (match-end 0)
:begin-uri (match-beginning 1)
:end-uri (match-end 1)
:begin-attributes (match-beginning 2)
:end-attributes (match-end 2)
:uri (match-string 1)))))

(put 'adoc-image 'bounds-of-thing-at-point #'adoc-bounds-of-image-link-at)

(defun adoc-display-image-at (&optional location)
"Create image overlay at LOCATION.
LOCATION can be a buffer position like `point'
or an event. It defaults to \\(point)."
(interactive "d")
(adoc-with-point-at-event location
(when-let ((link (adoc-image-link-at location)))
(adoc-create-image-overlay
(adoc-image-link-uri link)
(adoc-image-link-begin link)
(adoc-image-link-end link)
))))

(defun adoc-image-overlay-at (location)
"Get image overlay at LOCATION."
(adoc-with-point-at-event location
(cl-loop for ov being the overlays from (1- location) to (1+ location)
when (overlay-get ov 'adoc-image)
return ov)))

(defun adoc-remove-image-overlay-at (&optional location flush)
"Delete overlay at LOCATION.
POINT defaults to `point'.
POINT can also be a mouse event.
If FLUSH is non-nil also flush the cache for this image."
(interactive "d")
(adoc-with-point-at-event location
(let ((ov (adoc-image-overlay-at location)))
(when ov
(when-let ((image (and flush (overlay-get ov 'display)))
((imagep image)))
(image-flush image))
(delete-overlay ov)))))

(defvar adoc-image-context-menu (make-sparse-keymap "Image")
"Context menu shown at image links and corresponding overlays.")

(defun adoc-image-context-menu (menu event)
"Add `adoc-image-context-menu' to the context MENU.
See variable `context-menu-functions' for parameter EVENT."
(when (adoc-image-link-at event)
(let ((image-menu (make-sparse-keymap "Image")))
(set-keymap-parent image-menu adoc-image-context-menu)
(if (adoc-image-overlay-at event)
(easy-menu-add-item image-menu nil ["Remove Preview At Point"
(lambda (e) (interactive "e") (adoc-remove-image-overlay-at e))
t])
(easy-menu-add-item image-menu nil ["Generate Preview At Point"
(lambda (e) (interactive "e") (adoc-display-image-at e))
t]))
(easy-menu-add-item menu nil image-menu)))
menu)


;;;; misc
Expand Down Expand Up @@ -3438,7 +3697,9 @@ LOCAL-ATTRIBUTE-FACE-ALIST before it is looked up in
["$$text$$" tempo-template-pass-$$
:help ,adoc-help-pass-$$]
["`text`" tempo-template-monospace-literal ; redundant to the one in the quotes section
:help ,adoc-help-monospace-literal])))))
:help ,adoc-help-monospace-literal])))
"---"
["Toggle display of images" adoc-toggle-images t]))
map)
"Keymap used in adoc mode.")

Expand Down Expand Up @@ -3485,6 +3746,7 @@ Turning on Adoc mode runs the normal hook `adoc-mode-hook'."
;; nil, or even something else. See also similar comment in sgml-mode.
(setq-local imenu-create-index-function 'adoc-imenu-create-index)

(add-hook 'context-menu-functions #'adoc-image-context-menu nil t)
;; compilation
(when (boundp 'compilation-error-regexp-alist-alist)
(add-to-list 'compilation-error-regexp-alist-alist
Expand All @@ -3493,7 +3755,11 @@ Turning on Adoc mode runs the normal hook `adoc-mode-hook'."
2 3 nil (1 . nil))))
(when (boundp 'compilation-error-regexp-alist)
(make-local-variable 'compilation-error-regexp-alist)
(add-to-list 'compilation-error-regexp-alist 'asciidoc)))
(add-to-list 'compilation-error-regexp-alist 'asciidoc))
(when adoc-show-images-at-startup
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably name this adoc-display-images for consistency with the rest of the naming. After all the fact that the the var is used on mode start is an implementation detail. Technically speaking you can also use it for the toggle code and it can make it simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency I changed it to adoc-display-images-at-startup. But this variable cannot be used for toggling. It is really only an option whether adoc-display-images should be called within adoc-mode or not.

The user has the possibility to create or remove images locally through the functions adoc-display-image-at und adoc-remove-image-at.

(adoc-display-images))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you can wrap this in something like ignore-errors to avoid the CI issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing me there. I use now (and (display-graphic-p) adoc-show-images-at-startup) as condition. That does the trick.

(when adoc-use-local-context-menu
(setq-local context-menu-mode t)))

;;;###autoload
(add-to-list 'auto-mode-alist '("\\.a\\(?:scii\\)?doc\\'" . adoc-mode))
Expand Down
Loading