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

spec: Add support for source locales #351

Merged
merged 3 commits into from Sep 10, 2021

Conversation

pwithnall
Copy link
Contributor

This allows specifying the locale of the source translatable strings
in a component. The vast majority of the time, this is en_US, but
that’s not guaranteed. Add a source-locale attribute to allow changing
this default.

Signed-off-by: Philip Withnall pwithnall@endlessos.org

Helps: #345

docs/xml/collection-xmldata.xml Outdated Show resolved Hide resolved
docs/xml/collection-xmldata.xml Outdated Show resolved Hide resolved
@ximion
Copy link
Owner

ximion commented Sep 8, 2021

With the tag name style change and the para typo fixed, I think this is fine.
I do wonder though whether we maybe only need source_locale on the translation tag, and, since the language tags are usually auto-generated, could then make the generator code just "do the right thing" and insert a 100% en_US block or whatever source_locale was set to.
Is there any value in knowing the original language in a language tag block?

If we would go with the source_locale property on both translation and languages approach, I wonder if implementation-wise, libappstream should just abstract this away and "do the right thing", that is, add a "100% en_US" or "100% de_DE" entry if source_locale is either of the two. Then, when serializing the XML, it could restore the original source_locale property and drop the extra language value.
Would probably be a bit annoying to implement, but would save all clients from having to deal with this explicitly.

This allows specifying the locale of the source translatable strings
in a component. The vast majority of the time, this is `en_US`, but
that’s not guaranteed. Add a `source_locale` attribute to allow changing
this default.

The attribute has been added to the `<translation/>` element so that it
can be used by the metadata generator to synthesize a `<lang/>` element
for the source locale.

It’s not been added to the `<languages>` element, as it’s assumed that
element is auto-generated and hence can include the source locale
explicitly as a `<lang/>` element.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: ximion#345
@pwithnall
Copy link
Contributor Author

With the tag name style change and the para typo fixed, I think this is fine.
I do wonder though whether we maybe only need source_locale on the translation tag, and, since the language tags are usually auto-generated, could then make the generator code just "do the right thing" and insert a 100% en_US block or whatever source_locale was set to.

Sure, I’ve changed the spec wording and changes to go with that.

I’ll start working on the appstream-compose changes for this now, so please shout if something still looks wrong in the spec!

@pwithnall pwithnall marked this pull request as ready for review September 9, 2021 13:15
@pwithnall
Copy link
Contributor Author

I’ll start working on the appstream-compose changes for this now, so please shout if something still looks wrong in the spec!

Done!

src/as-translation.c Outdated Show resolved Hide resolved
src/as-translation.c Outdated Show resolved Hide resolved
@ximion
Copy link
Owner

ximion commented Sep 9, 2021

Looks good, except for the memory leak ^^
One peculiar thing about libappstream's code is that a lot of it was written without convenience helpers for the XML stuff at first, because adding if (a != NULL) and (xmlChar*) everywhere isn't so bad... you would think, until you have done that a billion times :-P
So, new code should use those helpers, but reading through all the old code to refactor 100% of it to use the new helpers so far wasn't worth the effort. I admit that the current mix is odd for people touching the code initially though.

This reflects the new `source_locale` attribute added to the
specification.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: ximion#345
Support parsing the new `<translation source_locale=""/>` attribute, and
turn it into a fake `<lang/>` entry at 100% for the source locale.

Add tests.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Fixes: ximion#345
@pwithnall
Copy link
Contributor Author

So, new code should use those helpers, but reading through all the old code to refactor 100% of it to use the new helpers so far wasn't worth the effort. I admit that the current mix is odd for people touching the code initially though.

Makes sense. Branch updated :)

@ximion
Copy link
Owner

ximion commented Sep 10, 2021

Looks good, thank you for working on this! :-)

@ximion ximion merged commit 2008fe9 into ximion:master Sep 10, 2021
11 checks passed
@pwithnall pwithnall deleted the 345-languages-default branch September 20, 2021 17:14
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.

None yet

2 participants