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(AddResourcesPatch): Support adding resources directly from the patch resources with localization for strings in mind #2593

Merged
merged 68 commits into from Jan 27, 2024

Conversation

oSumAtrIX
Copy link
Member

@oSumAtrIX oSumAtrIX commented Jan 7, 2024

About

This PR is based on the music-check-for-gmscore branch and depends on it being merged.
This PR aims to add support for the AddResourcesPatch to be able to add resources that are present in the patches. The sidequest it solves this way is to support localization for strings simultaneously by doing that.

The PR is heavily inspired by PR #2440 and aims to supersede it in a more generic way by getting rid of a dedicated "translations" patch.
It does so by providing the ability to add strings to a .json file in the resources. At the same time, it will add alternative strings in other languages under the same resource directory, such as values-de. In general:

  • Adds strings from resources/addresources/strings.json to res/values/strings.xml
  • Adds strings from resources/addresources/values-<language> to res/values-<language>/strings.xml
  • Adds other implementations of BaseResource to the app's resources such as arrays

Todo

  • The PR is in its early stages to discuss and make revisions before starting with the implementations.
    The plan is to make each implementation of BaseResource handle the logic of adding itself to the app's resources. This way, the AddResourcesPatch can iterate over its BaseResources and invoke resource.add(...), for example, which will make the resource add itself to the corresponding app's resources.
    This will prevent having to cram everything into AddResourcesPatch#close, and in case new kinds of BaseResource are implemented (for example, through the public API), the open-closed principle is not violated for AddResourcesPatch#close
  • Complete the functionality of AddResourcesPatch to add resources from the patches resources folder and to be able to add specific resources manually. Some patches may want to add strings or other kinds of resources without creating a resource JSON file in the patches.
  • Implement some edge cases present in feat: Move strings to resources for localization #2440 regarding escaping for example
  • Modify SettingsPatch preferences so that they only specify the key just like in feat: Move strings to resources for localization #2440
  • Add AddResourcesPatch to all patches that need it
  • Migrating all strings to resources
  • Implement integrations (feat: Move strings to resources for localization revanced-integrations#420 should work)

…atch resources with localization for strings in mind
…to feat/translations

# Conflicts:
#	src/main/kotlin/app/revanced/patches/all/misc/resources/AddResourcesPatch.kt
#	src/main/kotlin/app/revanced/patches/twitch/ad/embedded/EmbeddedAdsPatch.kt
#	src/main/kotlin/app/revanced/patches/twitch/misc/settings/SettingsPatch.kt
@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 7, 2024

@LisoUseInAIKyrios I have gone with something similar to your initial design by asking for a single .json file for strings. The json file is cascaded to allow only the necessary strings to be added. If AddResourcesPatch#invoke is called with the patch, just as the inline documentation explains, it'll add the patch strings to the app and any localization. It will do so by accessing the strings in stringsJson["youtube"]["ads.general.GeneralAdsPatch"] whereas it extracts the keys from the patches full qualifier.

This structure has been chosen to reduce duplicate json keys.

@LisoUseInAIKyrios
Copy link
Contributor

If I understand this, all strings would go into the single strings.json file and the strings are grouped by patch/class name.

If two different patches use the same strings (such as GMSCore for YouTube and YT Music), how is that handled?

@oSumAtrIX
Copy link
Member Author

As of now, the patch reads the strings that match to the current class. For abstracts and so on it would still look up the strings using the class of the abstract class implementation. An approach to solve this would be to pass down the class to the function. This way the abstract class would be used to lookup the strings instead of the implementation of that abstract class.

@oSumAtrIX
Copy link
Member Author

@LisoUseInAIKyrios, I have made a small change regarding this. I have noticed you may be referring to "shared" strings. Similarly to that in #2440. I'd imagine this would be useful if you have say a string "Ok" that would be duplicated across patches. Is there a scenario where this is imaginable? As of now, all patches that share strings also have a common super class in which the strings are present.

@LisoUseInAIKyrios
Copy link
Contributor

I'm sure a situation can exist where generic strings are shared. But it could be done using this PR as a simple shared patch that only has strings. It might be a little bloated using a patch only to include strings (instead of merging/including a string file) but it would work.

@oSumAtrIX
Copy link
Member Author

Another idea I had stashed and deleted was a "shared" object in the JSON similar to the shared package in the patches. Somehow patches could reference the strings and AddResourcesPatch would pick them from the shared path. Not sure how to go forward with this idea yet.

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 12, 2024

@LisoUseInAIKyrios Apart from the remaining TODOs, I've settled with what's pushed. If you do not have any comments, I'll finish with the implementation.

One PR change to consider is that the patch reads all resources from the XML files on execution and holds them in memory as BaseResource instances. This was done to be able to instantly add BaseResource instances to the patch as part of its design, instead of first staging PatchClass in a list and then on close of the patch, reading the resources, and adding them to the app's resources. Doing it using the latter method would spare the overhead of converting the XML nodes to BaseResource and keeping them in memory. Still, as part of the patch design (Add BaseResource, and then close merge them), I've settled with this unless we want to change that.

@LisoUseInAIKyrios
Copy link
Contributor

Yes this plan looks good

@LisoUseInAIKyrios
Copy link
Contributor

To verify the merging of translations works, one or more non-English languages can be added. Can use machine translations for these.

The limitation of having literalValues is that either all or no item is a literal, which is odd. Since this parameter found no proper use and items can be manually and individually prefixed with @string, the parameter has been removed
This particularly fixes arrays having "empty" items
@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 26, 2024

I am now finished with this PR, but some mistakes may have still been made:

  • Strings are not correctly escaped
  • Strings that have been removed in this PR may have been included (the XML files were generated off the latest dev branch)
  • Strings that may have been modified in this PR are not applied in the XML because the XML files were generated off the dev branch)
  • Since the title and co. are inferred from the key, it is possible that we have named a title string incorrectly so that, due to inference, the title string will not be found, but the AddResourcesPatch should throw an exception in such a case

The merge process will look the following way:

  1. fix(YouTube - GmsCore support): Gracefully exit when GmsCore is not running LisoUseInAIKyrios/revanced-integrations#2 is merged (by @LisoUseInAIKyrios)
  2. feat: Move strings to resources for localization revanced-integrations#420 is merged
  3. This PR will be merged to feat/music-check-for-gmscore
  4. feat/music-check-for-gmscore will overwrite LisoUseInAIKyrios:localization
  5. feat: Move strings to resources for localization #2440 is merged

@oSumAtrIX oSumAtrIX marked this pull request as draft January 26, 2024 11:09
@oSumAtrIX
Copy link
Member Author

I remember one string was "null_summary", so I converted this PR into a draft to find it. This should be as easy as observing StringResource construction

@oSumAtrIX oSumAtrIX marked this pull request as ready for review January 26, 2024 21:28
@oSumAtrIX oSumAtrIX requested review from LisoUseInAIKyrios and removed request for LisoUseInAIKyrios January 27, 2024 00:34
@oSumAtrIX oSumAtrIX merged commit 4bee51a into feat/music-check-for-gmscore Jan 27, 2024
1 check passed
@oSumAtrIX oSumAtrIX deleted the feat/translations branch January 27, 2024 00:57
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

4 participants