Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

feat: adapt image refuri if image uri has been adapten by i18n #12849

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

koeppe-at-pdtec
Copy link

@koeppe-at-pdtec koeppe-at-pdtec commented Aug 30, 2024

Subject: adapt image refuri if image uri has been adapten by i18n

Hello Dev-Team, i stumbled accross an issue, when using sphinx-intl and image references in figures.

Here is a solution that solves the problem for me. Maybe you like to include it to Sphinx.

Greets
Alex

Feature or Bugfix

  • Feature

Purpose

  • When using sphinx-intl and let it automatically substitute translated images using config option figure_language_filename = "{root}.{language}{ext}" reference uri's defined via :target: declaration in figures are broken.

Example:

      .. figure:: ./cherry.png
         :alt: Two red cherries
         :figwidth: 30%
         :target: ./_images/cherry.png

         Two red cherries

For language de ./cherry.png is replaced by cherry.de.png if the image is found in the source dir.
When HTML is built the file cherry.de.png is stored in _images but not cherry.png.
So the reference gets broken for alle language specific images.

In my case i like to link to the corresponding translated image also in the reference.

Detail

  • adapt refuri according to image_uri

Example

cd example
sphinx-build .\source\ build\html\de -D language=de
sphinx-build .\source\ build\html\en -D language=en

  • image links ar brocken for german version

@koeppe-at-pdtec koeppe-at-pdtec marked this pull request as draft August 30, 2024 09:10
@koeppe-at-pdtec koeppe-at-pdtec marked this pull request as ready for review August 30, 2024 10:20
@jayaddison
Copy link
Contributor

Hi @koeppe-at-pdtec - thank you for the bugreport and repro zipfile. A note: I don't think that this relates to the sphinx-intl extension, because the figure_language_filename setting is from Sphinx core, and also sphinx-intl does not appear in the extensions in the repro conf.py (nor is it a dependency of sphinx-rtd-theme, which does appear).

To help me understand, could you summarize what happens when you build an affected project, and what you expected to happen?

@jayaddison jayaddison added internals:other awaiting:response Waiting for a response from the author of this issue labels Sep 3, 2024
@koeppe-at-pdtec
Copy link
Author

koeppe-at-pdtec commented Sep 3, 2024

Hi @jayaddison,

thanks for response.

Here es a more detailed explanation .

In case you have a figure:: directive and use the :target: option, the behavior when building it with the html builder is, the image in the document becomes linked to the target.

If sphinx-intl is used to handle a multilanguage document it does provide handling language specific image files.

Maybe i have a misunderstanding at this point. I was thinking spinx-intl is the part handling all language specific stuff, but you are right, it is not part of the config. On the other hand some of the extensions a loaded by defautl, so i was not sure.

There is a configurable mapping, where image.png is transformed to image.de.png in case the language is set to e.g., 'de'. If there is an image named image.de.png it will rewrite the reference in the rst to point to image.de.png otherwise it keeps going wit image.png. This allows to have language specific figures.

If you use the :target: to point to a url or a file it currently is ignored by the option to have language specific targets.

Here comes the issue:
When buildin such documents the link in the html output is kept as is. But as the image.png got replaced by image.de.png, in my example, what ends up the html output contains image.de.png but not image.png.

There are mutlitple options to handle those cases.

  1. let the gettext builder handle :target: (extract the value of :target: to po files). This offers the option to just provide the
    correct target via the po file.
  2. "translate" the taget the same way as the reference so the resulting link again points to the correct file. (my patch)
  3. A combination of both
    • second option will fail for URL's
    • The first option may be used in case there is not translation provided, so automation is taking over, what probably solves
      the problem for most people without noticing

You can reproduce the issue by building my example like described and klick on the images in the english version, they work as expected. The same klick wil point to a nonexisting file in th german version though.

Let me know if this helps to understand the Issue.
Greets
Alex

@jayaddison jayaddison added i18n and removed awaiting:response Waiting for a response from the author of this issue labels Sep 3, 2024
@jayaddison
Copy link
Contributor

You can reproduce the issue by building my example like described and klick on the images in the english version, they work as expected. The same klick wil point to a nonexisting file in th german version though.

Ok, I understand that clicking the image doesn't work in the DE version. A next question is: before clicking, does the image load correctly in both EN and DE? (in other words: is the HTML img element's src attribute set correctly in both languages?)

@koeppe-at-pdtec
Copy link
Author

Hi @jayaddison,

yes the images are displayed correctly for de and en version. This works perfectly fine.

@jayaddison
Copy link
Contributor

Ok, thank you. Apologies for being slow/repetitive, but I'm trying to think through the problem.

When a figure is included in HTML output, if it has the target option specified, then it will be wrapped in an HTML anchor (<a>) allowing the user to click to open the target URL.

In this particular case, the target was written to match the figure filename -- but it is not identical -- an _images directory prefix is added to the target.

In the general case, the target may be a different object than the image -- it could be an internal or an external (different domain) URL.

At some stage during the build, because figure_language_filename is enabled, the filename of the figure is adjusted. This affects the correponding binary image filename written to the build directory, and also the image src attribute -- but the a href is not updated, because that originates from the target value. The feature request is to update this logic.

Does that seem correct @koeppe-at-pdtec?

@koeppe-at-pdtec
Copy link
Author

Hi @jayaddison ,

that exactly describes the issue. No worries about asking back. I think this issue is not that obvious an as i'm not a native englich speaker may lack clearity :)

Not updating the a href is the problem.
But i can also see the need to maybe not update automatically in case an URL is used.
For that reason i suggested to let the target be handled optionally in the po file, to define a language spacific target, in case you e.g., want to have an external url to be used as target different for each language.

@jayaddison
Copy link
Contributor

For that reason i suggested to let the target be handled optionally in the po file, to define a language spacific target, in case you e.g., want to have an external url to be used as target different for each language.

What would the effect(s) of that be for translation teams? Would they be editing the relevan image URL(s) in each case, typically only to adjust the language code?

@koeppe-at-pdtec
Copy link
Author

koeppe-at-pdtec commented Sep 9, 2024

In case you have separated files for different languages the translator need to adapt the language codes.
In other cases it might be more a question for the authors, as they may like to point to specific urls per language, what can not be done by a translation, as it need knowledge of the actual content or intention of the document.
There also might be cases, where the target points to a high-res version of the image.

So it would be kind of a misuse of the PO file here, but i think it is at least the most clear and unintrusive way to handle this.
Otherwise it probably would need a custom directive https://docutils.sourceforge.io/docs/howto/rst-directives.html to add multiple targets by option, one per language directly in the source, like

      .. figure:: ./cherry.png
         :alt: Two red cherries
         :figwidth: 30%
         :target: ./_images/cherry.png
         :target.de: ./_images/cherry.de.png


         Two red cherries

@jayaddison
Copy link
Contributor

         :target.de: ./_images/cherry.de.png

I'm not too keen on the per-language-target-option approach, because it implies that updating that part of a translated copy of the original work would require updating the original work itself.

@jayaddison
Copy link
Contributor

Would adding support for a target value of self -- a hyperlink to the diagram image file -- resolve the use-case here? Or is there a requirement to allow divergent URLs in some translations?

@koeppe-at-pdtec
Copy link
Author

If i understand correctly, this is close to the fix i proposed in my MR. For my specific use-case this probably would be a fix. But I can see other users that have the need to point to language specific url's, e.g. pointing to the englisch or german version of a wikipedia page. For those users the solution might not fullfill their requirements. And i can also see in our case this might be a thing in the future. They probalby would get their requirement solved using the po files to handle this.

Means, if there is a 'translation' of the target specified, take this, otherwise kepp going with the default language. Like handling untranslated text.

Or maybe we need a totally different approach, but i currently have not any good idea.

Just to be clear, i'm pleased with any kind of solutiuon, as i lack deep insight to sphinx, i can not judge what is the better solution.
I really appreciate your efforts .

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I more-or-less agree that this feature is reasonable. A few things that I think would help:

  • Filing a feature request that describes this could be useful; generally that's a good way to record and confirm the details of a bug/feature request before accepting an implementation. Exceptions are possible, including for small changes, but I think this change could be nontrivial.
  • Test coverage would be useful to demonstrate what the change is intended to do (and perhaps to confirm/test some things that it should not do).
  • I think that the other branch of the if condition here (imguri.endswith(os.extsep + '*')) might need to be handled here. If so, test coverage could again help - if not, it may be helpful to explain why it isn't needed.

@jayaddison
Copy link
Contributor

Ah, and an optional extra idea: if we do add a localized version of a hyperlink -- whether automatically by replacement, or using gettext / translation resources -- perhaps we should indicate that using the HTML hreflang attribute: https://www.w3.org/International/questions/qa-link-lang#hreflang

@koeppe-at-pdtec
Copy link
Author

Hi @jayaddison

ok i'll create a feature request.
I'll also will create the necessarry tests, covering the expected behavior. I just will need some time for this.
I think tests for other builders are required as well, not only html.

I'll notify you when i made progress or get stuck.

@jayaddison
Copy link
Contributor

Sounds good, thank you @koeppe-at-pdtec 👍

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable @koeppe-at-pdtec. It should have a CHANGES entry and ideally tests.

A

@koeppe-at-pdtec koeppe-at-pdtec marked this pull request as draft October 21, 2024 06:15
@koeppe-at-pdtec
Copy link
Author

Converted to draft until i have time to fix it and provide the unittests i offered above.
Currently i basically miss the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants