[FR] Please add environment variable substitution in `org-display-inline-images'

[FR] Please add environment variable substitution in `org-display-inline-images'

[Pan Xie]

Hello,

I recently found that the environment variable substitution does not
apply to inline image paths. Supposing I use a path
“/home/pxie/$Gallery/” to store all my image files, “$Gallery”
should be substituted with its corresponding value. The file link DOES
substitute the environment variables, but inline image paths not. So I
think it is a reasonable demand for the consistency.

It is also very important for my and other users' (I strongly believe)
configurations. Environment variable is the key abstraction to make
the same org document works on different machines with different path
hierarchies.

I hunt down the codes and find just one line change can fix that. In
`org-display-inline-images' function, add `substitute-in-file-name' to
the `expand-file-name' call. I can override the function with the
solution but I hope it can be added to the official codes.

So my proposal of the code change would be (find "pxie" in the codes):

#+begin_src elisp
(defun org-display-inline-images (&optional include-linked refresh beg end)
   "Display inline images.

An inline image is a link which follows either of these
conventions:

   1. Its path is a file with an extension matching return value
      from `image-file-name-regexp' and it has no contents.

   2. Its description consists in a single link of the previous
      type.  In this case, that link must be a well-formed plain
      or angle link, i.e., it must have an explicit \"file\" type.

Equip each image with the key-map `image-map'.

When optional argument INCLUDE-LINKED is non-nil, also links with
a text description part will be inlined.  This can be nice for
a quick look at those images, but it does not reflect what
exported files will look like.

When optional argument REFRESH is non-nil, refresh existing
images between BEG and END.  This will create new image displays
only if necessary.

BEG and END define the considered part.  They default to the
buffer boundaries with possible narrowing."
   (interactive "P")
   (when (display-graphic-p)
     (when refresh
       (org-remove-inline-images beg end)
       (when (fboundp 'clear-image-cache) (clear-image-cache)))
     (let ((end (or end (point-max))))
       (org-with-point-at (or beg (point-min))
     (let* ((case-fold-search t)
            (file-extension-re (image-file-name-regexp))
            (link-abbrevs (mapcar #'car
                      (append org-link-abbrev-alist-local
                          org-link-abbrev-alist)))
            ;; Check absolute, relative file names and explicit
            ;; "file:" links.  Also check link abbreviations since
            ;; some might expand to "file" links.
            (file-types-re
         (format 
"\\[\\[\\(?:file%s:\\|attachment:\\|[./~]\\)\\|\\]\\[\\(<?file:\\)"
             (if (not link-abbrevs) ""
               (concat "\\|" (regexp-opt link-abbrevs))))))
       (while (re-search-forward file-types-re end t)
         (let* ((link (org-element-lineage
               (save-match-data (org-element-context))
               '(link) t))
                    (linktype (org-element-property :type link))
            (inner-start (match-beginning 1))
            (path
             (cond
              ;; No link at point; no inline image.
              ((not link) nil)
              ;; File link without a description.  Also handle
              ;; INCLUDE-LINKED here since it should have
              ;; precedence over the next case.  I.e., if link
              ;; contains filenames in both the path and the
              ;; description, prioritize the path only when
              ;; INCLUDE-LINKED is non-nil.
              ((or (not (org-element-property :contents-begin link))
               include-linked)
               (and (or (equal "file" linktype)
                                (equal "attachment" linktype))
                (org-element-property :path link)))
              ;; Link with a description.  Check if description
              ;; is a filename.  Even if Org doesn't have syntax
              ;; for those -- clickable image -- constructs, fake
              ;; them, as in `org-export-insert-image-links'.
              ((not inner-start) nil)
              (t
               (org-with-point-at inner-start
             (and (looking-at
                   (if (char-equal ?< (char-after inner-start))
                   org-link-angle-re
                 org-link-plain-re))
                  ;; File name must fill the whole
                  ;; description.
                  (= (org-element-property :contents-end link)
                 (match-end 0))
                  (match-string 2)))))))
           (when (and path (string-match-p file-extension-re path))
         (let ((file (if (equal "attachment" linktype)
                 (progn
                                   (require 'org-attach)
                   (ignore-errors (org-attach-expand path)))
;;; {{{ pxie changes begin
                               (expand-file-name 
(substitute-in-file-name path)))))
;;; }}} pxie changes end
           (when (and file (file-exists-p file))
             (let ((width (org-display-inline-image--width link))
               (old (get-char-property-and-overlay
                 (org-element-property :begin link)
                 'org-image-overlay)))
               (if (and (car-safe old) refresh)
                           (image-flush (overlay-get (cdr old) 'display))
             (let ((image (org--create-inline-image file width)))
               (when image
                 (let ((ov (make-overlay
                        (org-element-property :begin link)
                        (progn
                      (goto-char
                       (org-element-property :end link))
                      (skip-chars-backward " \t")
                      (point)))))
                               ;; FIXME: See bug#59902.  We cannot rely
                               ;; on Emacs to update image if the file
                               ;; has changed.
                               (image-flush image)
                   (overlay-put ov 'display image)
                   (overlay-put ov 'face 'default)
                   (overlay-put ov 'org-image-overlay t)
                   (overlay-put
                    ov 'modification-hooks
                    (list 'org-display-inline-remove-overlay))
                   (when (boundp 'image-map)
                 (overlay-put ov 'keymap image-map))
                   (push ov org-inline-image-overlays))))))))))))))))
#+end_src

Thanks
Pan

Re: [FR] Please add environment variable substitution in `org-display-inline-images'

[Ruijie Yu via General discussions about Org-mode.]

Pan Xie <xiepan@skyguard.com.cn> writes:

> So my proposal of the code change would be (find "pxie" in the codes):

Thanks for proposing a change.  It would be best if you produced a patch
file from this :) But someone with a bit more free time could do that as
well.

This is just FYI, and by no means a big deal, but it would become a huge
burden for the maintainers to apply the proposed changes if the
changeset becomes larger.

For future reference, I'm talking about something like this (assuming
you have git and a shell supporting IO redirect):

    $ git diff this-file.el > something.diff

, or even better:

    $ git commit -m commit-message...
    $ git format-patch ...

In either case, you would attach the generated file in your message.

See also https://orgmode.org/worg/org-contribute.html for details.

-- 
Best,


RY

Re: [FR] Please add environment variable substitution in `org-display-inline-images'

[Ihor Radchenko]

Pan Xie <xiepan@skyguard.com.cn> writes:

> I recently found that the environment variable substitution does not
> apply to inline image paths. Supposing I use a path
> “/home/pxie/$Gallery/” to store all my image files, “$Gallery”
> should be substituted with its corresponding value. The file link DOES
> substitute the environment variables, but inline image paths not. So I
> think it is a reasonable demand for the consistency.

Agree.
Done, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3123caa8e

-- 
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: [FR] Please add environment variable substitution in `org-display-inline-images'

[Pan Xie]

Fantastic!! Thanks for your help. I can remove my ugly override codes.

BTW, Please keep in mind that the org export codes also need to 
substitute the environment variables. My proposal only works for the org 
file itself, When exports the org file to html, the image file path will 
still include the environment variable, which is of course not correct.

On 5/31/23 16:24, Ihor Radchenko wrote:
> Pan Xie <xiepan@skyguard.com.cn> writes:
>
>> I recently found that the environment variable substitution does not
>> apply to inline image paths. Supposing I use a path
>> “/home/pxie/$Gallery/” to store all my image files, “$Gallery”
>> should be substituted with its corresponding value. The file link DOES
>> substitute the environment variables, but inline image paths not. So I
>> think it is a reasonable demand for the consistency.
> Agree.
> Done, on main.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3123caa8e
>

Re: [FR] Please add environment variable substitution in `org-display-inline-images'

[Samuel Wales]

org-lint might need changing too?

On 5/31/23, Pan Xie <xiepan@skyguard.com.cn> wrote:
> Fantastic!! Thanks for your help. I can remove my ugly override codes.
>
> BTW, Please keep in mind that the org export codes also need to
> substitute the environment variables. My proposal only works for the org
> file itself, When exports the org file to html, the image file path will
> still include the environment variable, which is of course not correct.
>
> On 5/31/23 16:24, Ihor Radchenko wrote:
>> Pan Xie <xiepan@skyguard.com.cn> writes:
>>
>>> I recently found that the environment variable substitution does not
>>> apply to inline image paths. Supposing I use a path
>>> “/home/pxie/$Gallery/” to store all my image files, “$Gallery”
>>> should be substituted with its corresponding value. The file link DOES
>>> substitute the environment variables, but inline image paths not. So I
>>> think it is a reasonable demand for the consistency.
>> Agree.
>> Done, on main.
>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3123caa8e
>>
>
>


-- 
The Kafka Pandemic

A blog about science, health, human rights, and misopathy:
https://thekafkapandemic.blogspot.com

Re: [FR] Please add environment variable substitution in `org-display-inline-images'

[Pan Xie]

I think that depends on whether the linter will verify the existence of 
each file path. Strictly speaking, it should, thus the org-lint also 
need changing. On the other hand, if the tool just verify the grammar of 
the org file itself, then it might be OK without changing. I never use 
the linter before, might give it a try sometime.


But the export functionality definitely need changing, otherwise the 
images won't be shown in exported html (or other format) files, and 
someone will report a bug.

On 6/1/23 08:12, Samuel Wales wrote:
> org-lint might need changing too?
>
> On 5/31/23, Pan Xie <xiepan@skyguard.com.cn> wrote:
>> Fantastic!! Thanks for your help. I can remove my ugly override codes.
>>
>> BTW, Please keep in mind that the org export codes also need to
>> substitute the environment variables. My proposal only works for the org
>> file itself, When exports the org file to html, the image file path will
>> still include the environment variable, which is of course not correct.
>>
>> On 5/31/23 16:24, Ihor Radchenko wrote:
>>> Pan Xie <xiepan@skyguard.com.cn> writes:
>>>
>>>> I recently found that the environment variable substitution does not
>>>> apply to inline image paths. Supposing I use a path
>>>> “/home/pxie/$Gallery/” to store all my image files, “$Gallery”
>>>> should be substituted with its corresponding value. The file link DOES
>>>> substitute the environment variables, but inline image paths not. So I
>>>> think it is a reasonable demand for the consistency.
>>> Agree.
>>> Done, on main.
>>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3123caa8e
>>>
>>
>

[FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Ihor Radchenko]

Pan Xie <xiepan@skyguard.com.cn> writes:

> Fantastic!! Thanks for your help. I can remove my ugly override codes.
>
> BTW, Please keep in mind that the org export codes also need to 
> substitute the environment variables. My proposal only works for the org 
> file itself, When exports the org file to html, the image file path will 
> still include the environment variable, which is of course not correct.

It may or may not be correct.
For example, when exporting to Org (ox-org), one may want to keep the
environment variables.

If we add this feature for export, it should be an export option.
It will also be a breaking change.

I generally feel that it should be reasonable to go ahead with this
breaking change, possibly disabling variable resolution for ox-org only.

-- 
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: [FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Pan Xie]

I think we need keep consistent with the file link. Since file link will 
substitute the environment variables in its file path, I guess it will 
do the same thing when export to html files. It may keep the variables 
when export to Org (ox-org). Since both image link and file link are 
links, from user's perspective they should be consistent.

On 6/1/23 16:20, Ihor Radchenko wrote:
> Pan Xie <xiepan@skyguard.com.cn> writes:
>
>> Fantastic!! Thanks for your help. I can remove my ugly override codes.
>>
>> BTW, Please keep in mind that the org export codes also need to
>> substitute the environment variables. My proposal only works for the org
>> file itself, When exports the org file to html, the image file path will
>> still include the environment variable, which is of course not correct.
> It may or may not be correct.
> For example, when exporting to Org (ox-org), one may want to keep the
> environment variables.
>
> If we add this feature for export, it should be an export option.
> It will also be a breaking change.
>
> I generally feel that it should be reasonable to go ahead with this
> breaking change, possibly disabling variable resolution for ox-org only.
>

Re: [FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Ihor Radchenko]

Pan Xie <xiepan@skyguard.com.cn> writes:

> I think we need keep consistent with the file link. Since file link will 
> substitute the environment variables in its file path, I guess it will 
> do the same thing when export to html files. It may keep the variables 
> when export to Org (ox-org). Since both image link and file link are 
> links, from user's perspective they should be consistent.

This feature is not even documented. We may as well go other way and
remove variable substitution from the file links and keep the rest
unchanged. Not that we have to. But I think that the topic is worth
discussing first.

-- 
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: [FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Pan Xie]

Oh, that path will be horrible! Variable substitution is really 
important since it is an abstraction to make the same org file works on 
different hosts with different path hierarchies. I believe there are 
lots of users already rely on it, even without being documented. The 
reasonable thinking would be supporting more substitutions, rather than 
remove any.

On 6/1/23 16:35, Ihor Radchenko wrote:
> Pan Xie <xiepan@skyguard.com.cn> writes:
>
>> I think we need keep consistent with the file link. Since file link will
>> substitute the environment variables in its file path, I guess it will
>> do the same thing when export to html files. It may keep the variables
>> when export to Org (ox-org). Since both image link and file link are
>> links, from user's perspective they should be consistent.
> This feature is not even documented. We may as well go other way and
> remove variable substitution from the file links and keep the rest
> unchanged. Not that we have to. But I think that the topic is worth
> discussing first.
>

Re: [FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Ihor Radchenko]

Pan Xie <xiepan@skyguard.com.cn> writes:

> Oh, that path will be horrible! Variable substitution is really 
> important since it is an abstraction to make the same org file works on 
> different hosts with different path hierarchies. I believe there are 
> lots of users already rely on it, even without being documented. The 
> reasonable thinking would be supporting more substitutions, rather than 
> remove any.

Do not worry. https://bzg.fr/en/the-software-maintainers-pledge/:

    I won't break your user experience.
    I won't remove features.
    ...
    
I just wanted to point out that a discussion may be needed wrt export.
Maybe some other export backends also support env variables in file
paths? We should be a bit more careful if we want to support the
substitution everywhere, not just when opening file links from Emacs.

-- 
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: [FR] Should we resolve environment variables in the file link path when exporting? (was: [FR] Please add environment variable substitution in `org-display-inline-images')

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> I generally feel that it should be reasonable to go ahead with this
> breaking change, possibly disabling variable resolution for ox-org only.

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

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