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

Validate that lists don't get translated #610

Merged
merged 2 commits into from
Mar 2, 2024
Merged

Conversation

JakobDev
Copy link
Contributor

See flathub/flathub#5005 for reference

Copy link
Owner

@ximion ximion left a comment

Choose a reason for hiding this comment

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

I could have sworn that check existed, but apparently it does only for Catalog metadata...
Anyway, one small change and this should be good to go!

src/as-validator.c Outdated Show resolved Hide resolved
@JakeSmarter
Copy link

Although @JakobDev’s intentions here are certainly noble, but I am afraid that this PR may make matters unfortunately even worse than they already are.

The core issue here is that the AppStream spec on the matter of translating ol, ul, li elements is incomplete or inconclusive at best, and breaking the XML spec at worst. Imho the latter is the case here because the XML spec says:

A special attribute named xml:lang may be inserted in documents to specify the language used in the contents and attribute values of any element in an XML document.
[…]
The language specified by xml:lang applies to the element where it is specified (including the values of its attributes), and to all elements in its content unless overridden with another instance of xml:lang.

This means that XML parsing apps have to a) implement xml:lang attribute inheritance on nested elements and b) give XML document authors freedom to choose the structure in which xml:lang attributes are inherited in the document. In other words, XML document authors are free to choose and may create completely different content structures for different languages. Please, note also that specs building on top of the XML spec must not preclude using the xml:lang attribute on any element because the xml namespace is always implicitly imported in all valid XML documents and the xml:lang attribute may be literally used on “any element in an XML document” (including a root element!). Hence, this issue affects all AppStream elements, and not just those colloquially called translatable, like name, summary, p, ul, ol, li, or keyword.

@JakeSmarter
Copy link

While this may look daunting to implement at first, there is an easy fix for this when reading AppStream XML documents: You preprocess or transform the source XML document (with xml:lang attributes) into an intermediate resulting XML document with content of the language of interest applied but the xml:lang attribute removed before you do AppStream processing. This way, you get an XML document that pertains to the language of interest only but is no different than an AppStream XML document that is void of xml:lang attributes. You also get and preserve the structure pertaining to a language. Furthermore, you do not have to preclude using the xml:lang attribute in the AppStream spec on any specific element (no need for translatable and non‑translatable elements). You stop breaking the XML spec. You do not have to manage locale contexts for different contents in the lib internally either. It’s like xmas, birthday, and new year’s all together: win, win, win! 😁

If lib users request content for another language than in the current locale then you transform the source XML document again and update the AppStream object.

Transforming very large (hundreds of MBs) XML documents can be expensive but things are far from it most of the time. Furthermore, this type of transformation can be streamed, hence there is no need for large amounts of memory, temp files, caches, nor things like that.

@ximion ximion merged commit 6b2c02c into ximion:main Mar 2, 2024
10 checks passed
@JakobDev JakobDev deleted the listtrans branch March 2, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants