First Stab at I18N by ISNIT0 · Pull Request #20 · ThePhoneProject/advanced-android-camera

ISNIT0 · 2025-01-14T22:29:00Z

Implement a custom string internationalisation, because I hate the Android one (perhaps not a good idea, but oh well)
Create a Gradle task that automatically extracts i18n strings from the code, and adds them to the strings.xml file
Create a Gradle task that automatically translates english strings to french and spanish (for now) using an LLM
Add a CI job for this

How to make a string auto-internationalise:

val greeting = "Hello" // normal

val greeting = "Hello".i18n() // magic

// If device locale is french
print(greeting.resolve()) // prints: bonjour

val subString = "Hello, {name}"
print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

vckttr · 2025-01-16T08:18:49Z

app/src/main/java/co/stonephone/stonecamera/plugins/VideoMode.kt

        viewModel: StoneCameraViewModel,
-        previousMode: String,
-        nextMode: String
+        previousMode: TranslatableString,


is it possible to keep original String type?

vckttr · 2025-01-16T08:21:59Z

app/src/main/java/co/stonephone/stonecamera/plugins/VideoMode.kt


    override val modeLabel
-        get() = "video"
+        get() = @Translatable "video".i18n()


looks a bit too "bulky"
is there any options to use only either annotation or i18n() method?

Assuming that this approach is quite bulky, there may be cases where someone simply hard codes a string without using @Translatable or calling .i18n(). This would mean that string wouldn't be considered for translation and the app would ship with hardcoded strings. How might we check for this happening to reduce the likelihood the app is shipped with hardcoded strings rather than translatable/translated strings?

vckttr · 2025-01-16T08:23:56Z

app/src/main/java/co/stonephone/stonecamera/plugins/VolumeControls.kt

+                    val controlModeGet = viewModel.getSetting<TranslatableString>("volumeControlMode")

-                    if (isZoomMode || controlModeGet == "Zoom") {
+                    if (isZoomMode || controlModeGet?.resolve() == "Zoom") {


.resolve() == "Zoom"
will it still work in different langs?

Good question :) @ISNIT0 ?

vckttr · 2025-01-16T10:16:40Z

app/src/main/java/co/stonephone/stonecamera/utils/TranslatableString.kt

+    }
+
+    private fun sanitizeKey(key: String): String {
+        val sanitized = key.replace(Regex("[^a-zA-Z0-9]"), "_")


not a collision proof solution
a sanitized key for "!Test" will be equal to the key for "?Test"

potentially different parts of the app could have same strings in english but with different translations to other languages
so i would suggest to introduce a module prefix for keys

Also, for example some strings in English are written the same but have very different meanings so the same translation might not suit each use of what is lexically a common string. I'll provide a small example: "row" as in a row in a spreadsheet is not the same word as "row" as in two people had a row, or to row a boat.

vckttr · 2025-01-16T10:35:18Z

app/src/main/java/co/stonephone/stonecamera/StoneCameraApp.kt

-                            Text(text = modeLabel.uppercase(),
-                                color = if (modeLabel == selectedMode) Color(0xFFFFCC00) else Color.White,
+                            Text(text = modeLabel.resolve().uppercase(),
+                                color = if (modeLabel.resolve() == selectedMode.resolve()) Color(0xFFFFCC00) else Color.White,


as i've got you can leave it as it was modeLabel == selectedMode

vckttr · 2025-01-16T10:49:47Z

buildSrc/src/main/kotlin/UpdateStringsXmlTask.kt

+        sourceDir.walkTopDown()
+            .filter { it.isFile && it.extension in listOf("kt", "java") }
+            .forEach { file ->
+                val regex = Regex("@Translatable\\s*\\\"(.*?)\\\"")


cuts the string if you put \" in the middle

Sounds like having automated test cases that check the translation will behave as desired even for multi-line strings, etc. might be useful at checking whether these sorts of regex mistakes are dealt with?

vckttr · 2025-01-16T10:55:19Z

buildSrc/src/main/kotlin/UpdateStringsXmlTask.kt

+    /**
+     * Sanitizes a string to make it a valid XML key.
+     */
+    private fun sanitizeKey(key: String): String {


duplicated logic with

advanced-android-camera/app/src/main/java/co/stonephone/stonecamera/utils/TranslatableString.kt

Line 26 in 6cea681

private fun sanitizeKey(key: String): String {

julianharty

Overall I like this innovative approach to bootstrap translations of textual strings and it's a really nice idea. For production use I suspect the project may need suitably competent humans-in-the-loop for the translations, at least for a while.

It's probably worth another round of code reviews if you decide to refine the code based on my and other reviewer's comments. Happy to do so.

julianharty · 2025-01-16T13:56:44Z

app/src/main/java/co/stonephone/stonecamera/MainActivity.kt

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

+        TranslationManager.loadTranslationsForLocale(applicationContext)


Does this guarantee to return a set of elements (translations) regardless of the application's context? In other words are there ever going to be cases where it wouldn't return a) something the program can run with, b) something the human user can understand?

julianharty · 2025-01-16T13:57:35Z

app/src/main/java/co/stonephone/stonecamera/MainActivity.kt


+        TranslationManager.loadTranslationsForLocale(applicationContext)
+
        if (!isChromeOS()) {


Dumb question and not directly part of this PR... Are we targeting ChromeOS?

julianharty · 2025-01-16T13:59:18Z

app/src/main/java/co/stonephone/stonecamera/StoneCameraApp.kt

With the removal of android.util.log what's happening about logging?

julianharty · 2025-01-16T14:03:40Z

app/src/main/java/co/stonephone/stonecamera/StoneCameraViewModel.kt

                            workList.awaitAll()
-                            Log.d("Analyzer", "All plugins completed successfully")
                        } catch (e: Exception) {
                            Log.e("Analyzer", "Error in one or more plugins", e)


Again a question not directly part of this PR, but triggered when reading the code deleted above, what happens with the worklist.awaitAll() if an exception occurs in one or more of the plugins that are running in parallel?

julianharty · 2025-01-16T14:46:35Z

app/src/main/java/co/stonephone/stonecamera/plugins/AspectRatio.kt

                },
                renderLocation = SettingLocation.TOP,
-                label = "Aspect Ratio"
+                label = @Translatable "Aspect Ratio".i18n()


How does this call respond if a match isn't found for the string being translated?

julianharty · 2025-01-16T16:45:55Z

app/src/main/res/values-es/strings.xml

There's the potential for translated strings to require significant changes to screen real-estate for various reasons such as phrasing in that locale, to mistranslations, to nefarious attacks. Probably worth adding some forms of sanity checking of translated strings before they're incorporated into an app.

julianharty · 2025-01-16T16:50:49Z

build.gradle.kts

How long is the translation expected to take? and is the translation process intended to complete before the app is compiled? Oh, and are the translations something that are expected to be added into a git repo or simply processed on the build machine (which may be an ephemeral computer in the cloud)? I expect that auditability of translations (localisations) will be key at various times in the life of the project, sometimes long after the event that performed the translation/localisation, so auditing and maintaining easily readable snapshots might also be key to do.

julianharty · 2025-01-16T16:53:20Z

buildSrc/src/main/kotlin/GenerateTranslationsTask.kt

+    abstract val resDir: DirectoryProperty
+
+    private val apiKey: String =
+        System.getenv("OPENAI_API_KEY") ?: error("OPENAI_API_KEY environment variable not set")


I think this topic has already been mentioned in discussion so this is more of a reminder that relying on OpenAI (or other similar services) might be problematic from a legal/licensing perspective as and when the app is in production use.

How about adding logging to this file as a partial approach to maintaining a record of what's happening?

julianharty · 2025-01-16T16:55:35Z

buildSrc/src/main/kotlin/GenerateTranslationsTask.kt

+        language: String
+    ): String {
+        val languageName = when (language) {
+            "es" -> "Spanish"


These are great from a proof-of-concept point-of-view. Worth considering supporting locales rather than raw language codes e.g. to support es_MX for Mexican Spanish, etc.

julianharty · 2025-01-16T16:59:10Z

buildSrc/src/main/kotlin/UpdateStringsXmlTask.kt

+        sourceDir.walkTopDown()
+            .filter { it.isFile && it.extension in listOf("kt", "java") }
+            .forEach { file ->
+                val regex = Regex("@Translatable\\s*\\\"(.*?)\\\"")


Sounds like having automated test cases that check the translation will behave as desired even for multi-line strings, etc. might be useful at checking whether these sorts of regex mistakes are dealt with?

🌐 Start on I18n

f87c095

ISNIT0 force-pushed the ISNIT0/i18n branch from 1a9e13a to 858e016 Compare January 14, 2025 22:33

🚨 Fix Detekt Warning

73e6b4b

ISNIT0 force-pushed the ISNIT0/i18n branch 4 times, most recently from a09386a to 496daad Compare January 14, 2025 22:47

👷 Expose OPENAI API KEY secret to generateTranslations job

6cea681

ISNIT0 force-pushed the ISNIT0/i18n branch from 496daad to 6cea681 Compare January 14, 2025 22:49

vckttr reviewed Jan 16, 2025

View reviewed changes

julianharty reviewed Jan 16, 2025

View reviewed changes


		TranslationManager.loadTranslationsForLocale(applicationContext)

		if (!isChromeOS()) {

Conversation

ISNIT0 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vckttr Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vckttr Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vckttr Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianharty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

ISNIT0 commented Jan 14, 2025 •

edited

Loading

vckttr Jan 16, 2025 •

edited

Loading

vckttr Jan 16, 2025 •

edited

Loading

vckttr Jan 16, 2025 •

edited

Loading