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

[Timothy <tecosaur@gmail.com>]

[-- 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