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

Generated file contains strings from default resource value #10

Closed
bmarty opened this issue Mar 3, 2021 · 11 comments
Closed

Generated file contains strings from default resource value #10

bmarty opened this issue Mar 3, 2021 · 11 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bmarty
Copy link

bmarty commented Mar 3, 2021

This gradle plug-in works pretty well, but I noticed a problem when there are some missing template translations

For instance:
With file values/strings.xml

<string name="template_string_1">This is an example for ${app_name}</string>
<string name="template_string_2">This is a second example for ${app_name}</string>

And French file values-fr/strings.xml with missing translation for template_string_2

<string name="template_string_1">C\'est un exemple pour ${app_name}</string>

Generated file values-fr/resolved.xml will contain

<string name="string_1">C\'est un exemple pour My wonderful app</string>
<string name="string_2">This is a second example for My wonderful app</string>

I do not expect string_2 to be include in the generated file for French, since it will be already included in the generated file values/resolved.xml.

This can lead to issue reported from lint (Typos, etc.).

@LikeTheSalad
Copy link
Owner

Hi @bmarty, many thanks for your feedback!

I see. There's a way we can sort it out that hopefully will suit you, if not, please let me know. Though basically you can avoid getting such string into other languages' generated files if you set its translatable parameter to false, e,g:

<string name="template_string_2" translatable="false">This is a second example for ${app_name}</string>

Please let me know if that doesn't work for you.

Regards,
Cesar

@bmarty
Copy link
Author

bmarty commented Mar 4, 2021

Thanks for the fast answer.
In my case, the string has to be translated (so I cannot use translatable="false"), but the translation is not done yet.

@LikeTheSalad
Copy link
Owner

I see, got it. I remember leaving it like that to avoid double checking whether the user provided a translation because doing so could cause some performance penalties (and just leaving the string on the specific language wasn't causing any compilation issues, but I didn't think of lint issues) and also because I was thinking that, if someone didn't need a string translated, then they could just add the translatable="false" param and that'd tell the plugin to not to include it on a language file without having to check whether a translation was available or not. However, it seems like in your case you do plan to have it translated, just that you don't have that translation yet, so I guess one could argue that the issues you are having right now are only temporary?

In any case, I'm planning working on a refactoring soon, mostly to have a better, well defined separation of concerns across the different processes of this plugin, and also to stop using deprecated Gradle plugin tools (based on this other issue: #5) so I can add this extra validation as part of that work as well, hopefully the new design will make it easier to validate this case without the performance penalties. In the meantime though, if all the issues you're facing right now are only lint-related ones, I'd suggest you to either adding the translatable="false" option temporarily until you get the translations, or, temporarily ignoring the specific lint rule that is causing the issues until you get your translations.

I'll update this ticket once the improvements are available, however, being honest those could take up to 2 months from now since they have a lower level priority than the improvements I'm currently working on for https://github.com/LikeTheSalad/android-buddy - however, I hope my suggestions above can work for you in the meantime 👍

Regards,
Cesar

@LikeTheSalad LikeTheSalad self-assigned this Mar 5, 2021
@LikeTheSalad LikeTheSalad added the enhancement New feature or request label Mar 5, 2021
@bmarty
Copy link
Author

bmarty commented Mar 5, 2021

Wonderful, thanks!

For now I have disabled the lint check on those files (like that: https://github.com/vector-im/element-android/blob/develop/vector/lint.xml#L75).

I work on a project with many translations, so I can definitely not add translatable="false" as a workaround.

Anyway, there is no urgency to fix this issue!

I was also wondering if the plugin could also strip out the string starting with template_ at compilation time, since those string should not be used directly in the code. But this is another subject.

Thanks!

@LikeTheSalad
Copy link
Owner

Cool! glad you found a temporary solution 👍

That's also a great thing to consider when making the refactoring, I didn't know whether people would like to keep or not those template strings, so it is nice to have some input on that, thanks! I'll check what can be done about it.

Regards,
Cesar

@LikeTheSalad
Copy link
Owner

Hi @bmarty, this issue should be fixed now in version 1.3.0. Please feel free to reopen this ticket if you find that's not the case.

@bmarty
Copy link
Author

bmarty commented Jan 5, 2022

Hello @LikeTheSalad ,
Thanks for the release 1.3.0, our friend dependabot has upgraded the dependency to version 1.3.0 on our project, but the CI (lint) is not happy (see element-hq/element-android#4854).

The first error is:

/home/runner/work/element-android/element-android/vector/src/main/res/values-bn-rIN/strings.xml:1274: Error: "settings_add_3pid_flow_not_supported" is translated here but not found in default locale [ExtraTranslation]
    <string name="settings_add_3pid_flow_not_supported">আপনি রায়ট মোবাইল থেকে এটি করতে পারবেন না</string>
Lint found errors in the project; aborting build.

but the string (the template actually) is well defined in the default resource: https://github.com/vector-im/element-android/blob/d07dd154b8a4b961f3a14c0a1e30a921798eee3a/vector/src/main/res/values/strings.xml#L1101

Do you have an idea why we have this problem?

@LikeTheSalad LikeTheSalad reopened this Jan 5, 2022
@LikeTheSalad
Copy link
Owner

Oh, I think I know what's going on 🤦‍♂️ - I did add the validation to check for a template's location before storing the resolved string in a language folder, but now that I think about it better, even though this plugin knows about their connection (template and resolved string), still Android's lint rules don't... So to them they're still different resources, and it's not able to find the resolved resource in the raw ones.

So I think this will actually get solved for good as part of #15 where I'm going to get rid of the "template_" prefix for template resources, as I explained there in a comment. The idea is to have both template and resolved strings named the same, that way not only we'd avoid adding unwanted resources into the final binary, but also it will no longer be needed to compile the project for the resolved strings to become available into the code. Not sure how long this change will take, but at least the refactoring needed to make this plugin more flexible for these kind of changes is finished now as part of the code added into 1.3.0, so it shouldn't be as painful as what was needed to be done for 1.3.0, but definitely the way to use this plugin will change, as the "template_" prefix will not longer be needed, so that'll become version 2.0.0.

@LikeTheSalad LikeTheSalad added this to the 2.0.0 milestone Jan 5, 2022
@bmarty
Copy link
Author

bmarty commented Jan 5, 2022

Thanks! I'll wait for v2.0.0 to consider upgrading our dependency then.

@LikeTheSalad
Copy link
Owner

Hi @bmarty - The new version 2.0.0 is now available, it is no longer needed to define your templates by adding the "template_" to your string names anymore, which means that Lint shouldn't treat them as separate strings, thus avoiding the "translation" issue. Although, please bear in mind that a lot of things have changed in this new version 2.0.0, there's a rebranding along with new dependencies parameters and plugin ID, so in order to add the new version into your project, I recommend you to check again the README's instructions: https://github.com/LikeTheSalad/android-stem#21--changes-to-your-roots-buildgradle-file

@bmarty
Copy link
Author

bmarty commented Feb 25, 2022

Thanks @LikeTheSalad ! Migrating to 2.0.0 is fine so far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants