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.

Sign up for GitHub

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

Jump to bottom

Work/47display images #48

Merged
merged 12 commits into from
Feb 8, 2024
Merged

Work/47display images #48

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
69cb4bb
#47 Adapt display-images from markdown-mode to adoc-mode
Jan 31, 2024
7bdf4d9
Toggle display of images
Feb 1, 2024
8abe17c
Try to get working CI-tests by requiring mouse and image
Feb 2, 2024
5ea33a5
Correct package version from markdown 0.8.0 to adoc 0.9.0.
Feb 2, 2024
35f260d
Remove dependency on context-menu-mode.
Feb 3, 2024
acbe11e
Avoid failing CI-tests through attempt to show images at start
Feb 3, 2024
e8e8629
when-let has moved from subr-x to subr
Feb 3, 2024
ca7e637
Tell the byte-compiler that imagep and image-flush are defined.
Feb 3, 2024
950e78c
In defcustom booleanp is not a valid type.
Feb 3, 2024
5c46396
Add display of images to README.adoc.
Feb 5, 2024
71b9680
require subr-x simplification
Feb 5, 2024
38486ab
Rename adoc-display-images-at-startup to adoc-display-images.
Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions adoc-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2913,15 +2913,6 @@ Is influenced by customization variables such as `adoc-title-style'."))))
(defvar adoc-inline-image-overlays nil)
(make-variable-buffer-local 'adoc-inline-image-overlays)

(defun adoc-remove-images ()
"Remove inline image overlays from image links in the buffer.
This can be toggled with `adoc-toggle-inline-images'
or \\[adoc-toggle-inline-images]."
(interactive)
(save-restriction
(widen)
(remove-overlays nil nil 'adoc-image t)))

(defcustom adoc-display-remote-images nil
"If non-nil, download and display remote images.
See also `adoc-inline-image-overlays'.
Expand Down Expand Up @@ -3002,6 +2993,38 @@ or \\[adoc-toggle-inline-images]."
(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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
Expand Down Expand Up @@ -3674,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
Loading