-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
adoc-mode.el
Outdated
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andimage
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.
@bbatsov I already wrote a package |
ignoring any restrictions." | ||
(save-restriction | ||
(widen) | ||
(unless begin (setq begin (point-min))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;-).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
(push ov image-overlays))) | ||
image-overlays))) | ||
|
||
(defun adoc-remove-images () |
There was a problem hiding this comment.
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
?
(delete-overlay ov)) | ||
t))) | ||
|
||
(defun adoc-toggle-images () |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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").
adoc-mode.el
Outdated
(add-to-list 'compilation-error-regexp-alist 'asciidoc))) | ||
(add-to-list 'compilation-error-regexp-alist 'asciidoc)) | ||
(when adoc-show-images-at-startup | ||
(adoc-display-images)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
adoc-mode.el
Outdated
@@ -3493,7 +3757,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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-mode.el
Outdated
(choice (sexp :tag "Maximum height in pixels") | ||
(const :tag "No maximum height" nil))))) | ||
|
||
(defcustom adoc-show-images-at-startup t |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
"Activate context menu in adoc-mode. | ||
If deactivated, you can still globally activate | ||
the context menu with `context-menu-mode'." | ||
:group 'adoc |
There was a problem hiding this comment.
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.
Please, also mention the new functionality in the changelog and the readme. |
Yes I am aware of that. That is one of the reasons for marking this pull-request as draft. Thanks for the review. |
@bbatsov I have removed I very much like the context sensitive I think the User Experience stuff is very important. Maybe, we even could ask the community about the path which |
(require 'subr-x) | ||
)) | ||
|
||
(declare-function imagep "image.c") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using these declares I guess you don't need to require image
. I'm not sure if requiring mouse
is needed as well, if you did it just to see if it would affected the CI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(require 'image)
just didn't do the trick and mouse-event-p
is contained in subr.el
.
Thanks for the remainder...
- remove more "markdown" and "inline" - add context menu to image links - fontification of images before fontification of general links, since image links can contain urls
@bbatsov I have no keyboard shortcuts defined yet. Maybe, we can postpone this whole key-sequence-stuff and change this draft to a real pull request. |
Yeah, that'd be fine by me. In general I agree we'll need 2 keybindings only:
|
adoc-mode.el
Outdated
@@ -46,6 +46,10 @@ | |||
(require 'tempo) | |||
(require 'mouse) | |||
(require 'image) | |||
(eval-when-compile | |||
(when (version< emacs-version "29.0") ;; when-let has moved from subr-x to subr | |||
(require 'subr-x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subr-x is full of useful functions, so it's fine to just require it unconditionally.
adoc-mode.el
Outdated
(choice (sexp :tag "Maximum height in pixels") | ||
(const :tag "No maximum height" nil))))) | ||
|
||
(defcustom adoc-display-images-at-startup t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems my feedback about this one disappeared somewhere, but I think it should be named just adoc-display-images
and it should be also used in the toggle function to simplify it a bit. There's nothing really special about displaying the images at startup, so let's avoid such a specific name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the display state of the images are solely controlled by the image overlays in the buffer. So there is no variable needed for that.
The sense of adoc-display-images-at-startup
is to give the user the possibility to avoid the preview of images when an adoc-file is opened.
You are right, that at-startup
is a bad suffix, since the "Emacs startup sequence" is a reserved term. Maybe it is better to call it adoc-display-images-at-file-open
or adoc-display-images-at-find-file
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbatsov I've got the feeling that I miss the point of what you want with adoc-display-images
. I intended it as a user option. But, I have the feeling that you actually want it to be a buffer-local variable.
The user has the possibility to remove the preview for a single image link with adoc-remove-image-overlay-at
. I know from my writing experience with Orgmode that this is an important feature.
Furthermore, the user can insert a new image link having no overlay yet.
Both actions lead to a state where some image links in the buffer have overlays and some have none. This is not a clear state that could be described easily by a buffer-local variable adoc-display-images
.
This is also the reason for adoc-toggle-images
for first removing all remaining overlays and returning t if overlays were removed. If no overlays were removed it re-generates all overlays.
So, if there is a mixed state adoc-toggle-images
first establishes the clean state that there are no overlays at all. And if this state is reached the next call of adoc-toggle-images
will regenerate all overlays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking of making it buffer-local
, which is not in direct conflict with it being an user option.
Both actions lead to a state where some image links in the buffer have overlays and some have none. This is not a clear state that could be described easily by a buffer-local variable adoc-display-images.
You can always reset everything at toggle time. My point is mostly that this variable in its current form seems too specific to be of much usefulness. If you're concerned about changing it, I'd rather just remove it or leave it as-is, but with the name that I suggested, so we have more flexibility to change its behavior down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also the reason for adoc-toggle-images for first removing all remaining overlays and returning t if overlays were removed. If no overlays were removed it re-generates all overlays.
Btw, it's good to also mention such more granular use-cases in the README, as I'm normally an "all-in" user, so the thought of showing/hiding particular images didn't even cross my mind and I'm pretty sure I wouldn't be the only person thinking like this.
Add Usage section to README.adoc with a description of the not so obvious features.
@bbatsov If anything remains unsolved here we treat it as a new issue. |
Just a bit of feedback for the future - when squashing you should manually edit the commit message to remove parts that are not longer relevant:
Now the message is almost useless as it's just the list of the commits that were squashed together (and after the squashing some of them have observable effects, so it's pointless to mention those). |
Display images natively in adoc-mode (without the need of
texfrag-mode
).The implementation is adopted from
markdown-mode
.The display of images can be turned on via the corresponding AsciiDoc menu item.
The sub-menu
Image
is added to the context menu fromcontext-menu-mode
.Currently, there is only the menu item "Generate preview at point" or "Remove preview at point" in there.
But, a menu item for an image editor can be added there.