-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
base: dev
Are you sure you want to change the base?
feat: Add translations #2963
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
2816acd
to
f7d6f36
Compare
Is the cron job setup to run every hour? I may need an adjustment. |
The cron job runs at the first of every month, as requested by osumatrix |
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 |
Any idea why some strings changed here are not syncing with Crowdin? for example, here on GitHub dev a string is: But on Crowdin it still shows the old (incorrect) prior value: |
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 |
2fb7b50
to
1b76f90
Compare
@Ushie Shouldn't the same workflow push sources? I think that's a parameter of the action that is currently set to true. |
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 |
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. |
1b76f90
to
2d4e4e3
Compare
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. |
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 |
Yeah that is true. Did we figure out why it's not pushing changed strings from GitHub -> Crowdin? |
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 |
2d4e4e3
to
07dc50e
Compare
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. |
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. |
We haven't found a comfortable way yet to maintain the Crowdin end of ReVanced properly like we do with GitHub. |
@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 |
@oSumAtrIX somehow it's trying to pull the strings from Manager:
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. |
If any empty strings are still present, they can be removed using the Crowdin regex file processor. |
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 |
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. |
202e3ff
to
3c74198
Compare
@oSumAtrIX your commit to change the contribution file got clobbered by the last Crowdin -> GitHub pull. |
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. |
@oSumAtrIX Three issues need to be fixed before this can be merged (or at least, before the translations are usable in app).
But with these changes, translations should work as expected as I'm able to manually kludge the files and rename |
@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: Then create a new What do you think? I can do this very easily in about 20 minutes, and it would avoid doing all 3 of those fixes. |
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, |
Yes one of the issues is the pulled strings do not confirm to the original format, and they do not contain the The other issue is AddResourcePatch currently does not work if an existing string resource does not exist on the target app (such as 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. |
Would it not suffice to add any suffix known to be invalid, such as -staging instead of some version? The |
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. |
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
When the strings are flattened for Crowdin, there then exists two declarations of 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. |
@LisoUseInAIKyrios How i can now to push translation into app? Only decompile/compile patched app or another way? |
Two steps described here Rename And modify the strings.xml file. Before the first string add: |
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. |
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 |
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. |
Perhaps specifying the branch is needed? on:
push:
branches:
- dev
paths:
- /src/main/resources/addresources/values/strings.xml |
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
This could be fixed by specifying the branch dev branch for both the pull and push task. |
Sync translations from crowdin.com/project/revanced
Issues to sort out
<notranslate>
to indicate do not translate) edit: not available with project plan, and using a random placeholder tag gives a different and undesired behaviorupdate_as_unapproved
to project config to preserve existing translations during GitHub -> Crowdin sync edit: this change will not be needed./he/Strings.xml
to/iw/Strings.xml
after pulling from Crowdin to GitHub. YT uses the olderiw
and Crowdin uses the newerhe
. 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 onlyiw
fixes this. Edit: For now this is fixed by using a Crowdinghe
->iw
mapping, and this works for Manager/YT/Twitch since none use anyhe
resources. If a new app is added in the future that does usehe
then this may need to change, but for now this is simple and it works./ur-IN/Strings.xml
to/ur/Strings.xml
after pulling from Crowdin to GitHub. Then removeur-PK
from Crowdin (use one Urdu translation for all countries). Edit: a Crowdinur-IN
tour
mapping was added