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: Add translations #2963

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: Add translations #2963

wants to merge 1 commit into from

Conversation

revanced-bot
Copy link

@revanced-bot revanced-bot commented Apr 1, 2024

Sync translations from crowdin.com/project/revanced

Issues to sort out

  • Incorrect strings on Crowdin source
  • Support for all YouTube supported languages
  • Non-translatable parts in strings (add Crowdin project placeholder tag of <notranslate> to indicate do not translate) edit: not available with project plan, and using a random placeholder tag gives a different and undesired behavior
  • Fix cron sync to run once on first of the month
  • Add Crowdin quality check regular expression edit: With Crowdin identifying the strings as Android, Crowdin appears to handle quotations correctly
  • Try adding update_as_unapproved to project config to preserve existing translations during GitHub -> Crowdin sync edit: this change will not be needed.
  • Add Crowdin regex preprocessor to strip patches string enclosing xml elements
  • Modify the sync script to copy or rename /he/Strings.xml to /iw/Strings.xml after pulling from Crowdin to GitHub. YT uses the older iw and Crowdin uses the newer he. If both old and new language files are on an phone installation then only one code might be used and Hebrew strings will be partially broken. Copying to both or using only iw fixes this. Edit: For now this is fixed by using a Crowding he -> iw mapping, and this works for Manager/YT/Twitch since none use any he resources. If a new app is added in the future that does use he then this may need to change, but for now this is simple and it works.
  • Modify the sync script to move /ur-IN/Strings.xml to /ur/Strings.xml after pulling from Crowdin to GitHub. Then remove ur-PK from Crowdin (use one Urdu translation for all countries). Edit: a Crowdin ur-IN to ur mapping was added
  • Optional: Add to Crowdin project, an API key for Google Translate or some other free machine translate service to compliment and provide for more languages than the existing Crowdin machine translate. Edit: Skipping this. Can upload other machine translated languages by hand if needed.
  • Fix Crowdin pushing empty strings on sync Edit: issue may be fixed, need to try pulling from Crowdin and verify
  • Fix the last small issues with merging strings during patching

@LisoUseInAIKyrios

This comment was marked as resolved.

@github-actions github-actions bot force-pushed the feat/translations branch 10 times, most recently from 2816acd to f7d6f36 Compare April 1, 2024 10:10
@LisoUseInAIKyrios
Copy link
Contributor

Is the cron job setup to run every hour? I may need an adjustment.

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

The cron job runs at the first of every month, as requested by osumatrix

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

Unless GitHub interpreted it differently - Seems like it did
image

@LisoUseInAIKyrios
Copy link
Contributor

I think it's one of the * values, where it's uploading every hour of the first day of the month.

If the cron isn't changed, it might stop syncing on April 2nd.

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

Odd, GitHub suggests using Crontab.guru which we already have, yet it doesn't follow it correctly
image
image
image

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

I think it's one of the * values, where it's uploading every hour of the first day of the month.

If the cron isn't changed, it might stop syncing on April 2nd.

Right, because the minute value resets every hour

Opened a PR #2967

@LisoUseInAIKyrios
Copy link
Contributor

Any idea why some strings changed here are not syncing with Crowdin?

for example, here on GitHub dev a string is:
<string name="revanced_hide_keyword_toast_invalid_length">Invalid keyword. \'%1$s\' is less than %2$s characters</string>

But on Crowdin it still shows the old (incorrect) prior value:
<string name="revanced_hide_keyword_toast_invalid_length" formatted="false">Invalid keyword. \'%s\' is less than %s characters</string>

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

Oh, just realized something, for the repo to be syncing to Crowdin, the Sync Crowdin needs to be run, we may need to add two different workflows if you want syncing down to the repo to continue being monthly - but this may be unrelated to your issue as the workflows have just run, going to check now

@github-actions github-actions bot force-pushed the feat/translations branch 2 times, most recently from 2fb7b50 to 1b76f90 Compare April 1, 2024 12:17
@oSumAtrIX
Copy link
Member

@Ushie Shouldn't the same workflow push sources? I think that's a parameter of the action that is currently set to true.

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

The way things currently are, the, source in crowdin will get updated monthly, it should be more frequent so that people can translate on the go

@oSumAtrIX
Copy link
Member

That seems reasonable. In that case the workflow could skip over the step that pulls translations, but it seems unnecessary not to, which is why the workflow can run weekly.

@LisoUseInAIKyrios
Copy link
Contributor

If there is a way to just push changes to Crowdin, that could be done daily.

Because if someone is translating old outdated strings they are wasting their time since their translations will be outdated when the GitHub -> Crowdin push later occurs. (This is going in right now, since Crowdin still has old strings).

It also would be nice if no strings were changed on Dev, then do not push to Crowdin. That would prevent the Crowdin activity page from showing meaningless "ReVanced Bot compiled but changed nothing" events.

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

Syncing to crowdin does not have to be bound by a cron, it can be triggered by modifications to the source file, this also solves the problem you stated at the end of your message

@LisoUseInAIKyrios
Copy link
Contributor

Yeah that is true.

Did we figure out why it's not pushing changed strings from GitHub -> Crowdin?

@Ushie
Copy link
Member

Ushie commented Apr 1, 2024

I got carried away with something meaningless and forgot to look, I'm at work now, I'll see if I can RDP home and check

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Apr 15, 2024

I don't think it is as much of a problem as it is now. Over time, the languages will be translated, and anyone can contribute translations. If necessary, translators can use third-party machine translators on their own. I think the APIs even start to bill after a certain quota.

@LisoUseInAIKyrios LisoUseInAIKyrios changed the title feat: Add translations chore: Sync translations Apr 15, 2024
@LisoUseInAIKyrios LisoUseInAIKyrios changed the title chore: Sync translations feat: Add translations Apr 15, 2024
@MarcaDian
Copy link
Contributor

MarcaDian commented Apr 16, 2024

by the way, who can I contact to obtain the status of a proofleader on Crowdin, I wrote there several times, but they do not answer me. I want to be a proofreader of Ukrainian. I also keep a current translation of Revanced manager. Thanks in advance

@LisoUseInAIKyrios when you loaded the pre-translation, the strings of the translation became out of place, jumped a few lines down, keep this in mind.

@oSumAtrIX
Copy link
Member

We haven't found a comfortable way yet to maintain the Crowdin end of ReVanced properly like we do with GitHub.

@oSumAtrIX
Copy link
Member

@LisoUseInAIKyrios Regarding the last checkmark with pulling empty strings, ReVanced Manager handled this issue by post-processing strings that were pulled in the pull workflow: https://github.com/ReVanced/revanced-manager/blob/main/.github/workflows/sync_crowdin.yml#L46-L50

I tried checking, if the issue was fixed as assumed in the PR body but it seems there's no new translations: https://github.com/ReVanced/revanced-patches/actions/runs/8725347223/job/23938092361

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Apr 17, 2024

@oSumAtrIX somehow it's trying to pull the strings from Manager:

⚠️ Downloaded translations don't match the current project configuration. The translations for the following sources will be omitted (use --verbose to get the list of the omitted translations): - strings.i18n.json

https://crowdin.github.io/crowdin-cli/faq

I think it's getting confused, because the Patches source file was modified by the regex processor so the file pulled from Crowdin looks different from the source.

@LisoUseInAIKyrios
Copy link
Contributor

If any empty strings are still present, they can be removed using the Crowdin regex file processor.

@oSumAtrIX
Copy link
Member

I think we had discussed the issue with ReVanced Manager strings being attempted to download. Currently it just returns a warning and from what I know doesn't pull the ReVanced Manager strings so we should be good

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Apr 17, 2024

Something else must be wrong then, because there definitely is new translations to pull.

Maybe the old obsolete patches file on Crowdin is confusing it. I'll try deleting it, it has nothing in it and any prior translations users entered should already be stored in the 'translation memories' of the project.

Edit: deleting the old file fixed it.

@LisoUseInAIKyrios
Copy link
Contributor

@oSumAtrIX your commit to change the contribution file got clobbered by the last Crowdin -> GitHub pull.

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Apr 17, 2024

Interesting. From what I saw in the action source it checks out the target branch and then simply commits to it and pushes to remote, albeit via the force flag, which should not make any difference. Maybe it actually checks out the branch the workflow is triggered on. I'll cherry pick the clobbered commits into dev.

@LisoUseInAIKyrios
Copy link
Contributor

@oSumAtrIX Three issues need to be fixed before this can be merged (or at least, before the translations are usable in app).

  1. Since the <app> and <patch> are missing from the strings pulled from Crowdin, AddResourcesPatch either needs to merge all translated strings (without any regard to which strings were included or not), or it needs to selectively import the translated strings only if an English string with the same key was already merged.

  2. Translation files are not merged if they use the full language code including the region, such as pt-rBR. But files with the 2 digit code are merged, such if I rename the translated patch string files to pt.

  3. Some of the language files do not exist on the target app, such as el-rGR when YouTube has only el. AddResourcesPatch needs a minor change, where if a string file does not exist on the target app then it needs to create a new string file under that localized resource name (create and merge strings into a new file of /values-el-rGR/strings.xml).

But with these changes, translations should work as expected as I'm able to manually kludge the files and rename el-rGR to el and stick a dummy tag of <app><patch id="misc.settings.BaseSettingsResourcePatch"> around all translated strings, and then patching works and the strings are in app:

Greek

@LisoUseInAIKyrios
Copy link
Contributor

@oSumAtrIX Or instead doing all 3 of those fixes, this could be fixed by using a version resource path and then copy the translated strings as resources. Super simple, and only a few lines of code and it's done.

ie:
Don't pull from Crowdin to:values-el-rGR/strings.xml
And instead pull to: values-el-rGR-v23/strings.xml

Then create a new Translations patch, that simply copies all the translation files to the target app. Since YouTube and Twitch do not have any v23 (Android 6.0) string files, no merging or xml editing is needed. Android picks up on this and automatically uses the correct strings as well.

What do you think? I can do this very easily in about 20 minutes, and it would avoid doing all 3 of those fixes.

@oSumAtrIX
Copy link
Member

I don't quite understand the problem or why pulling to a different file is necessary. Is it because Crowdin pulled strings do not conform to the format we have in this repository (for example, <app>/<patch> tags)? What do you mean by creating a new translations patch? The existing resources patch automatically copies all translations, why would another patch be necessary?
The AddResourcesPatch is not app specific so we can not assume that v23 would be enough because the patch can be used on apps for Android 6.

@LisoUseInAIKyrios
Copy link
Contributor

Yes one of the issues is the pulled strings do not confirm to the original format, and they do not contain the <app><patch> outer tags.

The other issue is AddResourcePatch currently does not work if an existing string resource does not exist on the target app (such as pt-rBR, which YT only has pt).

Another patch is not needed, and the flat copying of files would work if added to AddResourcePatch. What I proposed is a quick way to get this working without directly fixing the 3 issues I listed above.

V23 is android 6.0+. But that could be v21, which is Android 4.0+ (I am not even sure if Android 4.0 still works anymore). The version resource tag is just a way to get around the need to merging string files by adding a version specific file that works for all supported targets.

@oSumAtrIX
Copy link
Member

Would it not suffice to add any suffix known to be invalid, such as -staging instead of some version? The AddResourcePatch should not be changed to only support a subset of scenarios specific to certain apps such as YouTube, which needs extra treatment, because the AddResourcePatch is agnostic towards any app and acts as a method to merge strings from patches -> app following the Android conventions. If I am misunderstanding you, please correct me, perhaps a small example/pseudocode could help?

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Apr 18, 2024

For add resource patches to be completely agnostic and not use resource directory name work arounds, the patch needs fixes for the 3 problems listed here

If you want to see try checking out this translation branch, patch YT or Twitch, then inspect the localized string files of the final patched app. None of the translations will be present because of the three issues.

@LisoUseInAIKyrios
Copy link
Contributor

There is a fourth problem, and it's with multiple apps declaring the same strings.

Such as Twitch and YT both using: revanced_debug_title

        <patch id="misc.settings.SettingsPatch">
            <string name="revanced_settings">ReVanced Settings</string>
            <string name="revanced_ads_screen_title">Ads</string>
            <string name="revanced_ads_screen_summary">Ad blocking settings</string>
            <string name="revanced_chat_screen_title">Chat</string>
            <string name="revanced_chat_screen_summary">Chat settings</string>
            <string name="revanced_misc_screen_title">Misc</string>
            <string name="revanced_misc_screen_summary">Miscellaneous settings</string>
            <string name="revanced_general_category_title">General settings</string>
            <string name="revanced_other_category_title">Other settings</string>
            <string name="revanced_client_ads_category_title">Client-side ads</string>
            <string name="revanced_surestream_ads_category_title">Server-side surestream ads</string>
            <string name="revanced_debug_title">Debug logging</string>
            <string name="revanced_debug_summary_on">Debug logs are enabled</string>
            <string name="revanced_debug_summary_off">Debug logs are disabled</string>
        </patch>

When the strings are flattened for Crowdin, there then exists two declarations of revanced_debug_title, revanced_debug_summary_on, and revanced_debug_summary_off.

Crowdin gets confused and keeps adding and deleting these strings, but this may also be a problem during patching when it tries to merge the strings (maybe it will ignore the duplicate strings though, who knows).

For now, I renamed these duplicate strings, but in the future a better approach is to use a shared app or some other way for strings to be shared across different apps. But maybe that is possible now and I'm overlooking something.

@MarcaDian
Copy link
Contributor

MarcaDian commented Apr 21, 2024

@LisoUseInAIKyrios How i can now to push translation into app? Only decompile/compile patched app or another way?
Because if i add strings.xml into addresources\values-uk or addresources\values-uk-rUA there no translation in patched app.

@LisoUseInAIKyrios
Copy link
Contributor

@MarcaDian

Two steps described here

Rename
addresources\values-uk-rUA
to
addresources\values-uk

And modify the strings.xml file.

Before the first string add: <app><patch id="misc.settings.BaseSettingsResourcePatch">
After the last string add: </patch></app>

@oSumAtrIX
Copy link
Member

I have a hard time making changes to this repository just to fit Crowdin's needs. Crowdin is supposed to suit our needs, not the other way around. Renaming strings and breaking designs of patches just because Crowdin can't properly deal with them is not the right way of handling this, in my opinion. Instead, we must find a way for Crowdin to suit our needs, perhaps custom string processors can be implemented on Crowdins end.

@LisoUseInAIKyrios
Copy link
Contributor

Any kind of custom string processor in Crowdin would require Crowdin to still recognize the Strings are for an Android project (and not a generic xml file). It would also require Crowdin to identify strings entirely by key (and not xml path, as it was before).

There is not many changes needed here for this to work without resorting to custom Crowdin processors or any kind of post processing.

At a bare minimum, all that is needed is for AddResourcePatch to work with the flattened translated files pulled from Crowdin. It would only merge the translated strings, if an identical English string is also merged from the unflattened original strings file. And that's it, everything then works.

@oSumAtrIX oSumAtrIX self-requested a review April 21, 2024 22:29
@LisoUseInAIKyrios
Copy link
Contributor

For some reason, the push to crowdin action is not running when strings.xml is changed on dev.

I think the action might be automatically running only for file changes on main, when it should be anytime strings.xml is changed on dev.

@oSumAtrIX
Copy link
Member

Perhaps specifying the branch is needed?

on:
  push:
    branches:
      - dev
    paths:
      - /src/main/resources/addresources/values/strings.xml

@LisoUseInAIKyrios
Copy link
Contributor

Perhaps specifying the branch is needed?

Probably. But I'm not familiar enough with yml to make that change.

Somehow the pull sync task is also pushing strings from main. When that ran yesterday it pushed some outdated strings from main to Crowdin:

https://github.com/ReVanced/revanced-patches/actions/runs/8903731021/job/24451935387

STARTING CROWDIN ACTION
UPLOAD SOURCES
✔️  Fetching project info
✔️  File 'strings.xml'
DOWNLOAD TRANSLATIONS
✔️  Fetching project info

This could be fixed by specifying the branch dev branch for both the pull and push task.

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.

feat(patch): Add translations to ReVanced in app strings
5 participants