[PATCH] Fix regex for determining image width from attribute

[PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 259 bytes --]

Hi,

A recent patch started computing the inline image width from any attr_
line. This is incorrect, as it matches settings like attr_latex, or
attr_html. We only want to look for settings specifically for the org
buffer. This patch fixes that.

Thanks
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org.el-Fix-regex-for-determining-image-width-fr.patch --]
[-- Type: text/x-patch, Size: 1121 bytes --]

From b9b8fb9b31dbb9145c2fe73af04eccd59f7a9973 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Sun, 21 Nov 2021 11:02:37 -0800
Subject: [PATCH] lisp/org.el: Fix regex for determining image width from
 attribute

	* lisp/org.el (org-display-inline-image--width): The regex
	should only match attr_org, not any attr_.  This would match
	attributes set for all export backends, such as latex and
	HTML, which is incorrect.
---
 lisp/org.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index eeefb4af3..92e765373 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16853,7 +16853,7 @@ buffer boundaries with possible narrowing."
    ((listp org-image-actual-width)
     (let* ((case-fold-search t)
            (par (org-element-lineage link '(paragraph)))
-           (attr-re "^[ \t]*#\\+attr_.*?: +.*?:width +\\(\\S-+\\)")
+           (attr-re "^[ \t]*#\\+attr_org: +.*?:width +\\(\\S-+\\)")
            (par-end (org-element-property :post-affiliated par))
            ;; Try to find an attribute providing a :width.
            (attr-width
-- 
2.31.1

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

Hi Matt,

> A recent patch started computing the inline image width from any attr_
> line. This is incorrect, as it matches settings like attr_latex, or
> attr_html. We only want to look for settings specifically for the org
> buffer. This patch fixes that.

Once again, thank you for the patch. The fact that the current regexp matches
`#+attr_latex' and `#+attr_html' is in fact by design though*. This is because I
consider it safe to assume that a `#+attr_*' which gives non-integer width between
0 and 2 can be safely assumed to be that proportion of the page width. e.g.
`#+attr_latex: :width 0.7\linewidth' or `#+attr_html: :width 70%'.
This way, a good guess can be made without having do have both an
`#+attr_latex/html' /and/ an `#+attr_org' line for the width. Should this assumption
be incorrect, a subsequent `#+attr_org' line will override the other `#+attr_*'.

Should there be edge-cases where this assumption doesn’t hold, I’d be interested
in patches that improves the logic here. As long as this holds more often than
not though, this should be a net positive for user experience as I see it.

Do please let me know if there are any good examples of unintended /
counter-intuitive behaviour you’re aware of.

All the best,
Timothy

* Well, it’s worked this way for a while, and I made a deliberate choice to keep
  this behaviour when expanding the width to recognise proportional values.

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Timothy <tecosaur@gmail.com> writes:

> Once again, thank you for the patch. The fact that the current regexp matches
> `#+attr_latex' and `#+attr_html' is in fact by design though*. This is because I
> consider it safe to assume that a `#+attr_*' which gives non-integer width between
> 0 and 2 can be safely assumed to be that proportion of the page width. e.g.
> `#+attr_latex: :width 0.7\linewidth' or `#+attr_html: :width 70%'.
> This way, a good guess can be made without having do have both an
> `#+attr_latex/html' /and/ an `#+attr_org' line for the width. Should this assumption
> be incorrect, a subsequent `#+attr_org' line will override the other `#+attr_*'.
>
> Should there be edge-cases where this assumption doesn’t hold, I’d be interested
> in patches that improves the logic here. As long as this holds more often than
> not though, this should be a net positive for user experience as I see it.
>
> Do please let me know if there are any good examples of unintended /
> counter-intuitive behaviour you’re aware of.

Thanks for explaining the logic behind the current implementation.

Unfortunately, I think this makes a valid use case
impossible. Specifically, I like to be able to set an image width
explicitly with #+attr_org (or some other attr_ for the corresponding
export) and fall back to the actual image width when this isn't
specified. This includes the ability to use the actual image width in an
org buffer, but an explicitly-set image width for export. For example,
for my blog I often have attr_html set, but I want the image to use its
actual width when displayed in org. I don't see how this is possible
with the current implementation. But, it works naturally with the
implementation I have in mind (IIRC this is how it previously worked,
but I could be mistaken).

I do understand the desire to avoid specifying a whole bunch of
redundant attr_ settings, but I don't think it should make fine-tuned
use cases like the one above impossible. I also find the current
implementation a bit counterintuitive, if less verbose. Maybe a solution
to accomplish all goals would be to add an #+attr_fallback (or
attr_default, attr_any, attr_all, etc.) that is used for any backend
unless a specific setting is made for that backend.

Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

Hi Matt,

> Unfortunately, I think this makes a valid use case
> impossible. Specifically, I like to be able to set an image width
> explicitly with #+attr_org (or some other attr_ for the corresponding
> export) and fall back to the actual image width when this isn’t
> specified. This includes the ability to use the actual image width in an
> org buffer, but an explicitly-set image width for export. For example,
> for my blog I often have attr_html set, but I want the image to use its
> actual width when displayed in org.

Thanks for explaining a use case! That’s most helpful.

> I don’t see how this is possible with the current implementation.But, it
> works naturally with the implementation I have in mind

Actually, it’s almost possible with the current implementation. Consider this
example:
┌────
│ #+attr_org: :width t
│ #+attr_html: :width 20%
│ [[file:image.png]]
└────

At the moment Org tries to interpret `t' as a number (and obviously fails),
however with a small tweak that I think would be very reasonable to make, this
would cause the behaviour you describe.

What do you think?

> (IIRC this is how it previously worked, but I could be mistaken).

You are mistaken. The previous implementation looked for `#+attr_*' too, but
didn’t recognise proportional values.

> Maybe a solution to accomplish all goals would be to add an #+attr_fallback
> (or attr_default, attr_any, attr_all, etc.) that is used for any backend
> unless a specific setting is made for that backend.

Hmmm, I’m not sure this is called for.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 22/11/2021 02:51, Matt Huszagh wrote:
> 
> Maybe a solution
> to accomplish all goals would be to add an #+attr_fallback (or
> attr_default, attr_any, attr_all, etc.) that is used for any backend
> unless a specific setting is made for that backend.

Then it is necessary make all backends aware of such attributes and 
length interpretation.

For a while attr_org may be tried at first with fallback to any other 
attr_ that has :width parameter.

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Timothy <tecosaur@gmail.com> writes:

> Actually, it’s almost possible with the current implementation. Consider this
> example:
> ┌────
> │ #+attr_org: :width t
> │ #+attr_html: :width 20%
> │ [[file:image.png]]
> └────
>
> At the moment Org tries to interpret `t' as a number (and obviously fails),
> however with a small tweak that I think would be very reasonable to make, this
> would cause the behaviour you describe.
>
> What do you think?

Yeah, I think it's ok. To be perfectly honest, I still don't love it,
but I understand now that my disagreement was with a decision made a
while ago (from a quick look at the commit history, 2012 or before), not
with the one you made recently.

In my opinion the most logical solution would be for the width to fall
back on specifically attr_org, not just any attr_ when
`org-image-actual-width' is nil. To clarify further, my main
disagreement is with the idea that setting attr_html (for example)
implies that one wants the same setting in the org buffer. But, it seems
that ship sailed a while ago and now this would be a breaking change.

So, given that, I concede and I think attr_org: :width t is an
acceptable compromise. Thanks for coming up with that!

>> (IIRC this is how it previously worked, but I could be mistaken).
>
> You are mistaken. The previous implementation looked for `#+attr_*' too, but
> didn’t recognise proportional values.

Ok, thanks for clarifying.

>> Maybe a solution to accomplish all goals would be to add an #+attr_fallback
>> (or attr_default, attr_any, attr_all, etc.) that is used for any backend
>> unless a specific setting is made for that backend.
>
> Hmmm, I’m not sure this is called for.

Yeah, I agree this is wrong. I'd misunderstood the current behavior.

Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

Hi Matt,

I’ve just pushed the change I described in 4514a32. This improves the
interpretation of :width attributes and makes a value of “t” work as discussed.
I have not prioritised #+attr_org for now, but that sounds like a change we
could make in the future.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Timothy <tecosaur@gmail.com> writes:

> I’ve just pushed the change I described in 4514a32. This improves the
> interpretation of :width attributes and makes a value of “t” work as discussed.
> I have not prioritised #+attr_org for now, but that sounds like a change we
> could make in the future.

Thanks Timothy. However, I think this change may have some issues. It
seems that it unbalances parentheses. This also no longer sets width
(so, e.g., (floatp width) won't work). Maybe attr-width was intended to
be renamed to width? Are you seeing the same?

I've got an implementation for prioritizing #+attr_org, but I want to
make sure your commit goes in the right way before I submit that.

Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Kyle Meyer]

Matt Huszagh writes:

> Timothy <tecosaur@gmail.com> writes:
>
>> I’ve just pushed the change I described in 4514a32. This improves the
>> interpretation of :width attributes and makes a value of “t” work as discussed.
>> I have not prioritised #+attr_org for now, but that sounds like a change we
>> could make in the future.
>
> Thanks Timothy. However, I think this change may have some issues. It
> seems that it unbalances parentheses. This also no longer sets width
> (so, e.g., (floatp width) won't work). Maybe attr-width was intended to
> be renamed to width? Are you seeing the same?

I'm not sure either, but this syntax error brings down the whole tree,
so I've pushed 27f26f782 to resolve it.  Timothy, please check my guess
at what the intended code was.

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 158 bytes --]

Hi Matt,

This issue and Kyle’s change were resolved in another thread, just FYI this is
fixed now. Thanks for mentioning it.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

Timothy <tecosaur@gmail.com> writes:

> This issue and Kyle’s change were resolved in another thread, just FYI this is
> fixed now. Thanks for mentioning it.

There is just one small residual error I could find. This patch fixes
it.

Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org.el-Fix-string-match-p-function-arguments.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]

From 3724b5bcadab6900367848dadcf470494b5b0d79 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 21:36:03 -0800
Subject: [PATCH] org.el: Fix string-match-p function arguments

	* lisp/org.el (org-display-inline-image--width):
	string-match-p requires 2 arguments, but only one was given.
---
 lisp/org.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 736b743c7..308bb7d51 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16867,7 +16867,7 @@ buffer boundaries with possible narrowing."
              ((string= attr-width "t") nil)
              ;; Fallback to `org-image-actual-width' if no interprable width is given.
              ((or (null attr-width)
-                  (string-match-p "\\`[^0-9]"))
+                  (string-match-p "\\`[^0-9]" attr-width))
               (car org-image-actual-width))
              ;; Convert numeric widths to numbers, converting percentages.
              ((string-match-p "\\`[0-9.]+%" attr-width)
-- 
2.31.1

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]

Hi Matt,

(sigh) well that’s silly. Thanks, I’ve just pushed that.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Here are two patches that prioritize attr_org over other attr_
keywords. I believe this is what you had in mind Timothy. But let me
know if not.

The second patch (intended to be applied after the first) improves the
documentation. It describes behavior that wasn't previously documented
and removes redundant documentation between org-image-actual-width and
org-display-inline-image--width (now only in
org-image-actual-width). Please double check I got everything correct,
as I haven't used all this behavior myself.

Thanks
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 1st patch --]
[-- Type: text/x-patch, Size: 1870 bytes --]

From 22bbe7d651e1f27398597297c7c35ae4f3253773 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:28:48 -0800
Subject: [PATCH 1/2] org.el: Prioritize attr_org when computing image width

	* lisp/org.el (org-display-inline-image--width): First look
	for attr_org: :width and then look for another attr_.* :width
	when that isn't specified.
---
 lisp/org.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 308bb7d51..bf5d08e09 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16853,13 +16853,20 @@ buffer boundaries with possible narrowing."
    ((listp org-image-actual-width)
     (let* ((case-fold-search t)
            (par (org-element-lineage link '(paragraph)))
-           (attr-re "^[ \t]*#\\+attr_.*?: +.*?:width +\\(\\S-+\\)")
+           (attr-re (lambda (backend)
+                      (concat "^[ \t]*#\\+attr_"
+                              backend
+                              ": +.*?:width +\\(\\S-+\\)")))
            (par-end (org-element-property :post-affiliated par))
-           ;; Try to find an attribute providing a :width.
+           ;; If an attr_org provides a :width, use that. Otherwise,
+           ;; use the first attribute that provides it, if any.
            (attr-width
             (when (and par (org-with-point-at
                                (org-element-property :begin par)
-                             (re-search-forward attr-re par-end t)))
+                             (or (re-search-forward (funcall attr-re "org")
+                                                    par-end t)
+                                 (re-search-forward (funcall attr-re ".*?")
+                                                    par-end t))))
               (match-string 1)))
            (width
             (cond
-- 
2.31.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 2nd patch --]
[-- Type: text/x-patch, Size: 3260 bytes --]

From aff581922e8d8bf10f813cdb9bc8adf9c8632683 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:30:11 -0800
Subject: [PATCH] org.el: Clarify documentation for computing image width

	* lisp/org.el (org-display-inline-image--width)
	(org-image-actual-width): Specify documentation for computing
	an inline image width in org-image-actual-width and remove the
	redundant documentation from org-display-inline-image--width.
---
 lisp/org.el | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index bf5d08e09..c8dc1815f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15529,10 +15529,29 @@ When set to a number in a list, try to get the width from any
 
 and fall back on that number if none is found.
 
-When set to nil, try to get the width from an #+ATTR.* keyword
-and fall back on the original width if none is found.
-
-When set to any other non-nil value, always use the image width.
+When set to nil, first try to get the width from #+ATTR_ORG.  If
+that is not found, use the first #+ATTR_.*:width specification.
+If that is also not found, fall back on the original image width.
+
+Finally, org is quite flexible in the width specifications it
+supports and intelligently interprets width specifications for
+other backends when rendering an image in an org buffer.  This
+behavior is described presently.
+
+1. A floating point value is interpreted as the percentage of the text
+   area that should be taken up by the image.
+2. A number followed by a percent sign is divided by 100 and then
+   interpreted as a floating point value.
+3. If a number is followed by other text, extract the number and
+   discard the remaining text.  That number is then interpreted as a
+   floating-point value.  For example,
+
+   #+ATTR_LATEX: :width 0.7\\linewidth
+
+   would be interpreted as 70% of the text width.
+4. If t is provided the original image width is used.  This is useful
+   when you want to specify a width for a backend, but still want to
+   use the original image width in the org buffer.
 
 This requires Emacs >= 24.1, built with imagemagick support."
   :group 'org-appearance
@@ -16838,16 +16857,7 @@ buffer boundaries with possible narrowing."
 (defvar visual-fill-column-width) ; Silence compiler warning
 (defun org-display-inline-image--width (link)
   "Determine the display width of the image LINK, in pixels.
-- When `org-image-actual-width' is t, the image's pixel width is used.
-- When `org-image-actual-width' is a number, that value will is used.
-- When `org-image-actual-width' is nil or a list, the first :width attribute
-  set (if it exists) is used to set the image width.  A width of X% is
-  divided by 100.
-  If no :width attribute is given and `org-image-actual-width' is a list with
-  a number as the car, then that number is used as the default value.
-  If the value is a float between 0 and 2, it interpreted as that proportion
-  of the text width in the buffer."
-  ;; Apply `org-image-actual-width' specifications.
+See `org-image-actual-width' for how the image width is computed."
   (cond
    ((eq org-image-actual-width t) nil)
    ((listp org-image-actual-width)
-- 
2.31.1

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 23/11/2021 14:46, Matt Huszagh wrote:
> Here are two patches that prioritize attr_org over other attr_
> keywords. I believe this is what you had in mind Timothy. But let me
> know if not.

> diff --git a/lisp/org.el b/lisp/org.el
> index 308bb7d51..bf5d08e09 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -16853,13 +16853,20 @@ buffer boundaries with possible narrowing."
>     ((listp org-image-actual-width)
>      (let* ((case-fold-search t)
>             (par (org-element-lineage link '(paragraph)))
> -           (attr-re "^[ \t]*#\\+attr_.*?: +.*?:width +\\(\\S-+\\)")
> +           (attr-re (lambda (backend)
> +                      (concat "^[ \t]*#\\+attr_"
> +                              backend
> +                              ": +.*?:width +\\(\\S-+\\)")))
>             (par-end (org-element-property :post-affiliated par))
> -           ;; Try to find an attribute providing a :width.
> +           ;; If an attr_org provides a :width, use that. Otherwise,
> +           ;; use the first attribute that provides it, if any.
>             (attr-width
>              (when (and par (org-with-point-at
>                                 (org-element-property :begin par)
> -                             (re-search-forward attr-re par-end t)))
> +                             (or (re-search-forward (funcall attr-re "org")
> +                                                    par-end t)
> +                                 (re-search-forward (funcall attr-re ".*?")
> +                                                    par-end t))))

I may be wrong, but it seems both the old and the new regexps match

     #+attr_html : :width 50%

that is not a keyword due to a space before ":". The dot in the regexp 
is too permissive.

> The second patch (intended to be applied after the first) improves the
> documentation. It describes behavior that wasn't previously documented
> and removes redundant documentation between org-image-actual-width and
> org-display-inline-image--width (now only in
> org-image-actual-width). Please double check I got everything correct,
> as I haven't used all this behavior myself.

> +that is not found, use the first #+ATTR_.*:width specification.

Despite ".*" includes ": " before ":width", I would prefer explicit 
space before ":width".

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

> I may be wrong, but it seems both the old and the new regexps match
>
>      #+attr_html : :width 50%
>
> that is not a keyword due to a space before ":". The dot in the regexp 
> is too permissive.

I agree.

> Despite ".*" includes ": " before ":width", I would prefer explicit 
> space before ":width".

Currently we have a space before .*. Would you prefer it after? Anyway,
I've also implemented this change. Let me know what you think.

Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: attr_org.patch --]
[-- Type: text/x-patch, Size: 5139 bytes --]

From 76a0c05cec8e449efd5cbffd8123338912815f17 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:28:48 -0800
Subject: [PATCH 1/2] org.el: Prioritize attr_org when computing image width

	* lisp/org.el (org-display-inline-image--width): First look
	for attr_org: :width and then look for another attr_.* :width
	when that isn't specified.
---
 lisp/org.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 308bb7d51..5f9d120a2 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16853,13 +16853,20 @@ buffer boundaries with possible narrowing."
    ((listp org-image-actual-width)
     (let* ((case-fold-search t)
            (par (org-element-lineage link '(paragraph)))
-           (attr-re "^[ \t]*#\\+attr_.*?: +.*?:width +\\(\\S-+\\)")
+           (attr-re (lambda (backend)
+                      (concat "^[ \t]*#\\+attr_"
+                              backend
+                              ":+.*? :width +\\(\\S-+\\)")))
            (par-end (org-element-property :post-affiliated par))
-           ;; Try to find an attribute providing a :width.
+           ;; If an attr_org provides a :width, use that. Otherwise,
+           ;; use the first attribute that provides it, if any.
            (attr-width
             (when (and par (org-with-point-at
                                (org-element-property :begin par)
-                             (re-search-forward attr-re par-end t)))
+                             (or (re-search-forward (funcall attr-re "org")
+                                                    par-end t)
+                                 (re-search-forward (funcall attr-re "[a-z]*?")
+                                                    par-end t))))
               (match-string 1)))
            (width
             (cond
-- 
2.31.1


From 0bc320b895ffb80a2a3ca8fb494e0aabe76180a3 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:30:11 -0800
Subject: [PATCH 2/2] org.el: Clarify documentation for computing image width

	* lisp/org.el (org-display-inline-image--width)
	(org-image-actual-width): Specify documentation for computing
	an inline image width in org-image-actual-width and remove the
	redundant documentation from org-display-inline-image--width.
---
 lisp/org.el | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 5f9d120a2..37369cdb6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15529,10 +15529,29 @@ When set to a number in a list, try to get the width from any
 
 and fall back on that number if none is found.
 
-When set to nil, try to get the width from an #+ATTR.* keyword
-and fall back on the original width if none is found.
-
-When set to any other non-nil value, always use the image width.
+When set to nil, first try to get the width from #+ATTR_ORG.  If
+that is not found, use the first #+ATTR_.*:width specification.
+If that is also not found, fall back on the original image width.
+
+Finally, org is quite flexible in the width specifications it
+supports and intelligently interprets width specifications for
+other backends when rendering an image in an org buffer.  This
+behavior is described presently.
+
+1. A floating point value is interpreted as the percentage of the text
+   area that should be taken up by the image.
+2. A number followed by a percent sign is divided by 100 and then
+   interpreted as a floating point value.
+3. If a number is followed by other text, extract the number and
+   discard the remaining text.  That number is then interpreted as a
+   floating-point value.  For example,
+
+   #+ATTR_LATEX: :width 0.7\\linewidth
+
+   would be interpreted as 70% of the text width.
+4. If t is provided the original image width is used.  This is useful
+   when you want to specify a width for a backend, but still want to
+   use the original image width in the org buffer.
 
 This requires Emacs >= 24.1, built with imagemagick support."
   :group 'org-appearance
@@ -16838,16 +16857,7 @@ buffer boundaries with possible narrowing."
 (defvar visual-fill-column-width) ; Silence compiler warning
 (defun org-display-inline-image--width (link)
   "Determine the display width of the image LINK, in pixels.
-- When `org-image-actual-width' is t, the image's pixel width is used.
-- When `org-image-actual-width' is a number, that value will is used.
-- When `org-image-actual-width' is nil or a list, the first :width attribute
-  set (if it exists) is used to set the image width.  A width of X% is
-  divided by 100.
-  If no :width attribute is given and `org-image-actual-width' is a list with
-  a number as the car, then that number is used as the default value.
-  If the value is a float between 0 and 2, it interpreted as that proportion
-  of the text width in the buffer."
-  ;; Apply `org-image-actual-width' specifications.
+See `org-image-actual-width' for how the image width is computed."
   (cond
    ((eq org-image-actual-width t) nil)
    ((listp org-image-actual-width)
-- 
2.31.1

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 24/11/2021 08:57, Matt Huszagh wrote:
> Max Nikulin writes:
> 
>> I may be wrong, but it seems both the old and the new regexps match
>>
>>       #+attr_html : :width 50%
>>
>> that is not a keyword due to a space before ":". The dot in the regexp
>> is too permissive.
> 
> I agree.
> 
>> Despite ".*" includes ": " before ":width", I would prefer explicit
>> space before ":width".
> 
> Currently we have a space before .*. Would you prefer it after? Anyway,
> I've also implemented this change. Let me know what you think.

This is related solely to docscring.

> +that is not found, use the first #+ATTR_.*:width specification.

I am unsure how to make this phrase more clear, maybe something like
"use :width value from the first #+ATTR_,*" or even "#+ATTR_xxx" to 
avoid ".*".

> +                                 (re-search-forward (funcall attr-re "[a-z]*?")
> +                                                    par-end t))))

https://orgmode.org/worg/dev/org-syntax.html#Keywords
has no requirement that it may be letter only. I expect it to be more 
coherent with
http://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/org-element.el#n2387
that uses "\\S-" non-space character.

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

> This is related solely to docscring.
>
>> +that is not found, use the first #+ATTR_.*:width specification.
>
> I am unsure how to make this phrase more clear, maybe something like
> "use :width value from the first #+ATTR_,*" or even "#+ATTR_xxx" to 
> avoid ".*".
>
>> +                                 (re-search-forward (funcall attr-re "[a-z]*?")
>> +                                                    par-end t))))
>
> https://orgmode.org/worg/dev/org-syntax.html#Keywords
> has no requirement that it may be letter only. I expect it to be more 
> coherent with
> http://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/org-element.el#n2387
> that uses "\\S-" non-space character.

Better?

Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: attr_org.patch --]
[-- Type: text/x-patch, Size: 5140 bytes --]

From 73f6d6d0c16d9b3312737463361cefe08b01d35c Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:28:48 -0800
Subject: [PATCH 1/2] org.el: Prioritize attr_org when computing image width

	* lisp/org.el (org-display-inline-image--width): First look
	for attr_org: :width and then look for another attr_.* :width
	when that isn't specified.
---
 lisp/org.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 308bb7d51..3718d3118 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16853,13 +16853,20 @@ buffer boundaries with possible narrowing."
    ((listp org-image-actual-width)
     (let* ((case-fold-search t)
            (par (org-element-lineage link '(paragraph)))
-           (attr-re "^[ \t]*#\\+attr_.*?: +.*?:width +\\(\\S-+\\)")
+           (attr-re (lambda (backend)
+                      (concat "^[ \t]*#\\+attr_"
+                              backend
+                              ":+.*? :width +\\(\\S-+\\)")))
            (par-end (org-element-property :post-affiliated par))
-           ;; Try to find an attribute providing a :width.
+           ;; If an attr_org provides a :width, use that. Otherwise,
+           ;; use the first attribute that provides it, if any.
            (attr-width
             (when (and par (org-with-point-at
                                (org-element-property :begin par)
-                             (re-search-forward attr-re par-end t)))
+                             (or (re-search-forward (funcall attr-re "org")
+                                                    par-end t)
+                                 (re-search-forward (funcall attr-re "\\S-+?")
+                                                    par-end t))))
               (match-string 1)))
            (width
             (cond
-- 
2.31.1


From 76e92428716f2dcde0fbd281f71739c44a9be9d3 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Mon, 22 Nov 2021 23:30:11 -0800
Subject: [PATCH 2/2] org.el: Clarify documentation for computing image width

	* lisp/org.el (org-display-inline-image--width)
	(org-image-actual-width): Specify documentation for computing
	an inline image width in org-image-actual-width and remove the
	redundant documentation from org-display-inline-image--width.
---
 lisp/org.el | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 3718d3118..b050cb0dd 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15529,10 +15529,29 @@ When set to a number in a list, try to get the width from any
 
 and fall back on that number if none is found.
 
-When set to nil, try to get the width from an #+ATTR.* keyword
-and fall back on the original width if none is found.
-
-When set to any other non-nil value, always use the image width.
+When set to nil, first try to get the width from #+ATTR_ORG.  If
+that is not found, use the first #+ATTR_xxx :width specification.
+If that is also not found, fall back on the original image width.
+
+Finally, org is quite flexible in the width specifications it
+supports and intelligently interprets width specifications for
+other backends when rendering an image in an org buffer.  This
+behavior is described presently.
+
+1. A floating point value is interpreted as the percentage of the text
+   area that should be taken up by the image.
+2. A number followed by a percent sign is divided by 100 and then
+   interpreted as a floating point value.
+3. If a number is followed by other text, extract the number and
+   discard the remaining text.  That number is then interpreted as a
+   floating-point value.  For example,
+
+   #+ATTR_LATEX: :width 0.7\\linewidth
+
+   would be interpreted as 70% of the text width.
+4. If t is provided the original image width is used.  This is useful
+   when you want to specify a width for a backend, but still want to
+   use the original image width in the org buffer.
 
 This requires Emacs >= 24.1, built with imagemagick support."
   :group 'org-appearance
@@ -16838,16 +16857,7 @@ buffer boundaries with possible narrowing."
 (defvar visual-fill-column-width) ; Silence compiler warning
 (defun org-display-inline-image--width (link)
   "Determine the display width of the image LINK, in pixels.
-- When `org-image-actual-width' is t, the image's pixel width is used.
-- When `org-image-actual-width' is a number, that value will is used.
-- When `org-image-actual-width' is nil or a list, the first :width attribute
-  set (if it exists) is used to set the image width.  A width of X% is
-  divided by 100.
-  If no :width attribute is given and `org-image-actual-width' is a list with
-  a number as the car, then that number is used as the default value.
-  If the value is a float between 0 and 2, it interpreted as that proportion
-  of the text width in the buffer."
-  ;; Apply `org-image-actual-width' specifications.
+See `org-image-actual-width' for how the image width is computed."
   (cond
    ((eq org-image-actual-width t) nil)
    ((listp org-image-actual-width)
-- 
2.31.1

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 24/11/2021 22:59, Matt Huszagh wrote:
> 
> Better?

Certainly. I have not tested the patch though.

I am sorry that I confused you by my note concerning space before 
:width. I am afraid, current variant means repeated ":"

> +                      (concat "^[ \t]*#\\+attr_"
> +                              backend
> +                              ":+.*? :width +\\(\\S-+\\)")))
                                    ^
-----------------------------------'

Is space after "#+attr_XXX:" is required at all? Is something besides 
spaces allowed here?

Untested:
":\\s-*:width\\s-+\\(\\S-+\\)"
I am unsure concerning newlines as space characters, so the following, 
perhaps, is more correct:
":[^\n\\S-]*:width[^\n\\S-]+\\(\\S-+\\)"

Actually value is everything till line end besides trailing spaces, so 
precise regexp should be a bit longer. Are there backends that allows 
spaces between number and units?

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 25/11/2021 00:00, Max Nikulin wrote:
> On 24/11/2021 22:59, Matt Huszagh wrote:
> 
> I am sorry that I confused you by my note concerning space before 
> :width. I am afraid, current variant means repeated ":"
> 
>> +                      (concat "^[ \t]*#\\+attr_"
>> +                              backend
>> +                              ":+.*? :width +\\(\\S-+\\)")))
>                                   ^
> ----------------------------------'
> 
> Is space after "#+attr_XXX:" is required at all? Is something besides 
> spaces allowed here?

Of course, another attributes may be there.

> Untested:
> ":\\s-*:width\\s-+\\(\\S-+\\)"
> I am unsure concerning newlines as space characters, so the following, 
> perhaps, is more correct:
> ":[^\n\\S-]*:width[^\n\\S-]+\\(\\S-+\\)"
> 
> Actually value is everything till line end besides trailing spaces, so 
> precise regexp should be a bit longer.

I am confused. I can not figure out how to create the following as HTML 
export result:

<img src="img.png" alt="An image without :width 600 attribute">

Attempt to add quotes leads to &quot; and does not prevent ":width" to 
become another attribute.

   #+attr_html: :alt An image without :width 600 attribute
   [[file:img.png]]

   <p><img src="img.png" alt="An image without" width="600 attribute" />
</p>

My current variant:

":\\(?:[^\n]*?[[:blank:]]\\)?:width[[:blank:]]+\\(\\S-+\\)"

The regexp should not match e.g.

#+attr_html: :alt something
:width 600

P.S. I would prefer to use the same parser as ox does.

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Max Nikulin <manikulin@gmail.com> writes:

> I am confused. I can not figure out how to create the following as HTML 
> export result:
>
> <img src="img.png" alt="An image without :width 600 attribute">
>
> Attempt to add quotes leads to &quot; and does not prevent ":width" to 
> become another attribute.
>
>    #+attr_html: :alt An image without :width 600 attribute
>    [[file:img.png]]
>
>    <p><img src="img.png" alt="An image without" width="600 attribute" />
> </p>
>
> My current variant:
>
> ":\\(?:[^\n]*?[[:blank:]]\\)?:width[[:blank:]]+\\(\\S-+\\)"
>
> The regexp should not match e.g.
>
> #+attr_html: :alt something
> :width 600
>
> P.S. I would prefer to use the same parser as ox does.

(cadr par) contains all the attr_ keywords with their values. I think it
would be better to use this instead of doing a regex search, which I
expect would be slower and more prone to errors. If we want to be strict
(and probably more correct), we could use
org-export-registered-backends. Otherwise, we could look for any key
that starts with attr_.

I can probably implement this if people want. But, let me know if I
should indeed use org-export-registered-backends. However, I'm starting
to feel like this should be separate patch (the goal for mine was just
to prioritize attr_org). I also still don't really like the behavior
here. I don't think it makes sense to interpret a width as 120% if we
have something like

#+attr_latex: :width 1.2\columnwidth

Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

Hi Matt,

> I also still don’t really like the behavior
> here. I don’t think it makes sense to interpret a width as 120% if we
> have something like
>
> #+attr_latex: :width 1.2

What would be a more sensible interpretation in your mind? The “true” value
depends on the number of columns, and fetching that information seems a bit
unreasonable. Since this isn’t just used if nothing else if given, I see a 120%
interpretation as fairly reasonable.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 29/11/2021 07:23, Matt Huszagh wrote:
> Max Nikulin writes:
> 
>> My current variant:
>>
>> ":\\(?:[^\n]*?[[:blank:]]\\)?:width[[:blank:]]+\\(\\S-+\\)"
>>
>> The regexp should not match e.g.
>>
>> #+attr_html: :alt something
>> :width 600
>>
>> P.S. I would prefer to use the same parser as ox does.
> 
> I can probably implement this if people want. But, let me know if I
> should indeed use org-export-registered-backends. However, I'm starting
> to feel like this should be separate patch (the goal for mine was just
> to prioritize attr_org).

I am against regexps that have obvious flaws. I admit however that the 
regexp that appeared long time ago before your patch fails for some 
corner cases as well.

I will left decision to you and to Org developers and maintainers.

Additionally, I like that Timothy transformed a code fragment into 
`org-display-inline-image--width' function and, I suppose, it deserves 
some unit tests (see testing/README file).

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Timothy <tecosaur@gmail.com> writes:

> What would be a more sensible interpretation in your mind? The “true” value
> depends on the number of columns, and fetching that information seems a bit
> unreasonable. Since this isn’t just used if nothing else if given, I see a 120%
> interpretation as fairly reasonable.

I think there are several different questions/considerations here, which
I'll address in a second. But first, I think the essential disagreement
is whether org should take an action if not explicitly told to do so. I
think org should only perform some action if given a clear directive. In
this context, I feel that org is guessing what the user wants and taking
an action based on that guess.

Ok, back to the fact that there are multiple considerations here. The
first issue is whether specifying a width for a backend reflects an
intention to have that same width in the org buffer. As I previously
stated, I don't agree that one implies the other. But, as also
previously discussed, this was a decision that was made almost 10 years
ago, so changing it would be a breaking change, etc. Because of that,
I'm not totally sure what org should do, and I expect a lot of people
won't want to change this.

The other consideration is if we take the first point as a given (that
org should use width directives for other backends), should it also
attempt to interpret directives that are ambiguous? In this case, do we
interpret 1.2\somemacro as 1.2? If \somemacro could only be \linewidth,
I'd be inclined to agree that this is logical. I also understand the
case for \columnwidth, though this is slightly less clear. But, what if
someone used 1.2\columnsep? Seems a bit unusual I know, but maybe
someone would want this. Again, I don't think we should guess if there's
a chance we could be wrong.

I totally agree with you that we don't want to implement a pseudo latex
parser here. But I feel like all this complexity is easily resolved by
just requiring that people be explicit about their intentions (i.e.,
specify #+attr_org: :width). That would avoid all the complex behavior
and surprises that could result from making intelligent guesses about
what the user wants.

Anyway, let me know what you want in terms of the patch. I still think
prioritizing attr_org should be its own patch and changing the regex and
all the other behavior should be a separate issue. But, if you'd like me
to perform the change I mentioned in my last email, I can take the time
to write that up and include it in the same patch.

Thanks
Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Timothy]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

Hi Matt,

Thanks for your thoughtful deliberation on this.

> I think the essential disagreement is whether org should take an action if not
> explicitly told to do so. I think org should only perform some action if given
> a clear directive. In this context, I feel that org is guessing what the user
> wants and taking an action based on that guess.

I broadly agree with this, but I think this is provided by the four forms that
`org-image-actual-width' can take:
⁃ `t', in which case the actual image width is always used
⁃ an integer, in which case that will always be used as the width
⁃ `nil', which produces the guessing behaviour we’re discussing
⁃ `(val)', which guesses, falling back on `val'

> Ok, back to the fact that there are multiple considerations here. The
> first issue is whether specifying a width for a backend reflects an
> intention to have that same width in the org buffer. As I previously
> stated, I don’t agree that one implies the other. But, as also
> previously discussed, this was a decision that was made almost 10 years
> ago, so changing it would be a breaking change, etc. Because of that,
> I’m not totally sure what org should do, and I expect a lot of people
> won’t want to change this.

I’m not opposed to /expanding/ the behaviour (with due consideration), which could
resolve some of your concerns, but I don’t think it would be good to prevent the
current behaviour, which at this point seems well-established.

> The other consideration is if we take the first point as a given (that
> org should use width directives for other backends), should it also
> attempt to interpret directives that are ambiguous? In this case, do we
> interpret 1.2 as 1.2? If  could only be ,
> I’d be inclined to agree that this is logical. I also understand the
> case for , though this is slightly less clear. But, what if
> someone used 1.2? Seems a bit unusual I know, but maybe
> someone would want this. Again, I don’t think we should guess if there’s
> a chance we could be wrong.

I feel this very much depends on how bad “guessing wrong” is, and as previously
discussed, since it’s rather easy to correct or set `org-image-actual-width' to
prevent this, I’m not sure it warrants being terribly concerned about.

> I totally agree with you that we don’t want to implement a pseudo latex
> parser here. But I feel like all this complexity is easily resolved by
> just requiring that people be explicit about their intentions (i.e.,
> specify #+attr_org: :width). That would avoid all the complex behavior
> and surprises that could result from making intelligent guesses about
> what the user wants.

I think prioritising `#+attr_org: :width' makes a lot of sense, but I feel quite
reluctant to /require/ it.

> Anyway, let me know what you want in terms of the patch. I still think
> prioritizing attr_org should be its own patch and changing the regex and
> all the other behavior should be a separate issue. But, if you’d like me
> to perform the change I mentioned in my last email, I can take the time
> to write that up and include it in the same patch.

Thanks for continuing with this. Moving forward, I think it would be best to:
⁃ Make a patch just for prioritising `#+attr_org'
⁃ Make a patch just improving the regex (before or after the `#+attr_org' patch)
⁃ Discuss changing the behaviour of image previews separately later / in another
  thread, linking to this thread when doing so.

How does that sound?

Lastly, a comment on your documentation patch from earlier. I like the changes
to `org-image-actual-width', however I think you’ve been over-eager with scrapping
the current docstring for `org-display-inline-image--width'. Since the behaviour
is implemented there, I think it should at a minimum be documented there.
The docstring for a function referring to a variable’s documentation for how it’s
handled by the function seems a bit weird.

All the best,
Timothy

Re: [PATCH] Fix regex for determining image width from attribute

[Matt Huszagh]

Timothy <tecosaur@gmail.com> writes:

> Thanks for your thoughtful deliberation on this.

No worries, and thanks for continuing to engage with it.

>> The other consideration is if we take the first point as a given (that
>> org should use width directives for other backends), should it also
>> attempt to interpret directives that are ambiguous? In this case, do we
>> interpret 1.2 as 1.2? If  could only be ,
>> I’d be inclined to agree that this is logical. I also understand the
>> case for , though this is slightly less clear. But, what if
>> someone used 1.2? Seems a bit unusual I know, but maybe
>> someone would want this. Again, I don’t think we should guess if there’s
>> a chance we could be wrong.
>
> I feel this very much depends on how bad “guessing wrong” is, and as previously
> discussed, since it’s rather easy to correct or set `org-image-actual-width' to
> prevent this, I’m not sure it warrants being terribly concerned about.

I think the problem here is that `org-image-actual-width` is essentially
a global solution to a potentially local problem. I can't set
`org-image-actual-width` to nil for just one image.

> Thanks for continuing with this. Moving forward, I think it would be best to:
> ⁃ Make a patch just for prioritising `#+attr_org'
> ⁃ Make a patch just improving the regex (before or after the `#+attr_org' patch)
> ⁃ Discuss changing the behaviour of image previews separately later / in another
>   thread, linking to this thread when doing so.
>
> How does that sound?

Yep, sounds good.

> Lastly, a comment on your documentation patch from earlier. I like the changes
> to `org-image-actual-width', however I think you’ve been over-eager with scrapping
> the current docstring for `org-display-inline-image--width'. Since the behaviour
> is implemented there, I think it should at a minimum be documented there.
> The docstring for a function referring to a variable’s documentation for how it’s
> handled by the function seems a bit weird.

I was torn on this as well. However, I ended up feeling that it would be
worse to duplicate the same information in two places. This requires
updating the information in two places instead of one, and, worse, the
documentation could easily diverge. Because the function isn't
public-facing, but the variable very much is, I felt it better just to
include the documentation for the variable.

I also don't think removing the documentation from the function is that
bad. A lot of what that function does is to follow what the variable
tells it to do. Again, the function won't be called directly by an
end-user, so requiring that the developer refer to other documentation
for understanding implementation details seems reasonable. Finally,
there are numerous functions in org that tell you to refer to
documentation elsewhere (especially defcustoms) for further information.

Anyway, I still feel that the earlier patch (before regex changes is the
right one). But, if you want me to revert the documentation removal from
the function I will.

Matt

Re: [PATCH] Fix regex for determining image width from attribute

[Ihor Radchenko]

Matt Huszagh <huszaghmatt@gmail.com> writes:

>> Thanks for continuing with this. Moving forward, I think it would be best to:
>> ⁃ Make a patch just for prioritising `#+attr_org'
>> ⁃ Make a patch just improving the regex (before or after the `#+attr_org' patch)
> ...
>> Lastly, a comment on your documentation patch from earlier. I like the changes
>> to `org-image-actual-width', however I think you’ve been over-eager with scrapping
>> the current docstring for `org-display-inline-image--width'. Since the behaviour
>> is implemented there, I think it should at a minimum be documented there.
>> The docstring for a function referring to a variable’s documentation for how it’s
>> handled by the function seems a bit weird.

I have used parts of Matt's patches + the new org-element-ast API to
implement all the discussed (and agreed upon) changes:

1. #+attr_org is prioritised
2. Docstrings are updated

Handled, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=fede1c990

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Fix regex for determining image width from attribute

[Karthik Chikmagalur]

> 1. #+attr_org is prioritised
> 2. Docstrings are updated
>
> Handled, on main.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=fede1c990

Just a note that we already prioritize #+attr_org when
aligning/centering images.  So this change makes sense.

Karthik

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

On 02/05/2024 23:28, Ihor Radchenko wrote:

> 1. #+attr_org is prioritised

I ma afraid, the code is a bit fragile. Consider

#+attr_html: :alt Image width test
#+attr_beamer: :width \linewidth
#+attr_latex: :width +.5\textwidth
#+attr_md: :width 75%
[[file:babelfish.png]]

- I do not mind that just "\linewidth" is ignored.
- The case of "+.5" should either be supported or at least documented 
since it is a valid floating number.
- It is really confusing that #+attr_html casts shadow on #+attr_md.

> +++ b/etc/ORG-NEWS
> @@ -13,6 +13,18 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
>  
>  * Version 9.7 (not released yet)
>  ** Important announcements and breaking changes
> +*** Inline image width value in =#+attr_org= is preferred over other =#+attr_...= keywords
> +
> +Previously, when ~org-image-actual-width~ is a list, Org used the
> +first =#+attr_...= keyword containing =:width ...= to compute the inline
> +image width:
> +
> +: #+attr_html: :width 30%
> +: #+attr_org:  :width is ignored
> +: [[image.png]]
> +
> +Now, =#+attr_org=, if present, takes precedence.

I think, it is better to avoid "is ignored" here.

Previously, when ~org-image-actual-width~ is a list or nil,
Org used the first =#+attr_...= keyword containing =:width ...=
to compute the inline image width.
Now, =#+attr_org=, if present, takes precedence.
In the following example the image preview has width of 70%
while earlier versions take 33%.

: #+attr_html: :width 33%
: #+attr_org:  :width 0.7
: [[image.png]]

Re: [PATCH] Fix regex for determining image width from attribute

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> 1. #+attr_org is prioritised
>
> I ma afraid, the code is a bit fragile. Consider
>
> #+attr_html: :alt Image width test
> #+attr_beamer: :width \linewidth
> #+attr_latex: :width +.5\textwidth
> #+attr_md: :width 75%
> [[file:babelfish.png]]
>
> - I do not mind that just "\linewidth" is ignored.
> - The case of "+.5" should either be supported or at least documented 
> since it is a valid floating number.

Was it supported before?

> - It is really confusing that #+attr_html casts shadow on #+attr_md.

No, it is not. I see no reason to prioritize markdown attributes.

>> +++ b/etc/ORG-NEWS
>> @@ -13,6 +13,18 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
> ...
>
> I think, it is better to avoid "is ignored" here.
>
> Previously, when ~org-image-actual-width~ is a list or nil,
> Org used the first =#+attr_...= keyword containing =:width ...=
> to compute the inline image width.
> Now, =#+attr_org=, if present, takes precedence.
> In the following example the image preview has width of 70%
> while earlier versions take 33%.
>
> : #+attr_html: :width 33%
> : #+attr_org:  :width 0.7
> : [[image.png]]

May you convert your suggestion into a patch?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Fix regex for determining image width from attribute

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

On 08/05/2024 17:54, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> #+attr_html: :alt Image width test
>> #+attr_beamer: :width \linewidth
>> #+attr_latex: :width +.5\textwidth
>> #+attr_md: :width 75%
>> [[file:babelfish.png]]
>>
>> - I do not mind that just "\linewidth" is ignored.
>> - The case of "+.5" should either be supported or at least documented
>> since it is a valid floating number.
> 
> Was it supported before?

No. It is my expectation based on earlier discussions that fractional 
numbers were added namely for LaTeX. I used leading dot in 
\includegraphics[width=...]{}, so I was surprised that Org ignores it. 
Explicit "+" is just generalization, this form is supported by LaTeX and 
elisp.

>> - It is really confusing that #+attr_html casts shadow on #+attr_md.
> 
> No, it is not. I see no reason to prioritize markdown attributes.

Above #+attr_html does not specify :width while #+attr_md has a valid 
value. It is the reason why I do like current behavior.

>>> +++ b/etc/ORG-NEWS
>>
>> I think, it is better to avoid "is ignored" here.
> 
> May you convert your suggestion into a patch?

See the attachment.

[-- Attachment #2: 0001-ORG-NEWS-Reword-inline-image-width-note.patch --]
[-- Type: text/x-patch, Size: 1472 bytes --]

From 7bc9d909867bf2f99a77d5d1554cd41e4fc664ae Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Thu, 9 May 2024 17:32:54 +0700
Subject: [PATCH] ORG-NEWS: Reword inline image width note

* etc/ORG-NEWS: Avoid possible confusion related to
"#+attr_org: :width" example.
---
 etc/ORG-NEWS | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 3c597db40..a34307295 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -15,16 +15,16 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
 ** Important announcements and breaking changes
 *** Inline image width value in =#+attr_org= is preferred over other =#+attr_...= keywords
 
-Previously, when ~org-image-actual-width~ is a list, Org used the
+Previously, when ~org-image-actual-width~ is a list or nil, Org used the
 first =#+attr_...= keyword containing =:width ...= to compute the inline
-image width:
+image width.  Now, =#+attr_org=, if present, takes precedence.
+In the following example the image preview has width of 75%
+while earlier versions pick 33%.
 
-: #+attr_html: :width 30%
-: #+attr_org:  :width is ignored
+: #+attr_html: :width 33%
+: #+attr_org:  :width 0.75
 : [[image.png]]
 
-Now, =#+attr_org=, if present, takes precedence.
-
 *** =ox-html=: When exporting footnotes with custom non-number names, the names are used as link anchors
 
 Previously, link anchors for footnote references and footnote
-- 
2.39.2

Re: [PATCH] Fix regex for determining image width from attribute

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> I ma afraid, the code is a bit fragile. Consider
>
> #+attr_html: :alt Image width test
> #+attr_beamer: :width \linewidth
> #+attr_latex: :width +.5\textwidth
> #+attr_md: :width 75%
> [[file:babelfish.png]]
>
> - It is really confusing that #+attr_html casts shadow on #+attr_md.

Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f326cd58b

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Fix regex for determining image width from attribute

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> I think, it is better to avoid "is ignored" here.
>> 
>> May you convert your suggestion into a patch?
>
> See the attachment.
> From 7bc9d909867bf2f99a77d5d1554cd41e4fc664ae Mon Sep 17 00:00:00 2001
> From: Max Nikulin <manikulin@gmail.com>
> Date: Thu, 9 May 2024 17:32:54 +0700
> Subject: [PATCH] ORG-NEWS: Reword inline image width note

Thanks!
Applied, onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b4d17c062

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>