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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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?

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"// normalval greeting ="Hello".i18n() // magic// If device locale is frenchprint(greeting.resolve()) // prints: bonjourval subString ="Hello, {name}"print(greeting.resolve(mapOf("name" to "World!"))) // prints: Hello, World!

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

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


is it possible to keep original String type?

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


 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

+ 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

+ }
+
+ 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

- 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

+ 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

+ /**
+ * 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
privatefunsanitizeKey(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

 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


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


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

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

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

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

 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

 },
 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

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

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

+ 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

+ 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

+ 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 858e016Compare January 14, 2025 22:33

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

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

vckttr reviewed Jan 16, 2025
View reviewed changes

julianharty reviewed Jan 16, 2025
View reviewed changes

🌐 Start on I18n
f87c095

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

🚨 Fix Detekt Warning
73e6b4b

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

👷 Expose OPENAI API KEY secret to generateTranslations job
6cea681

ISNIT0 force-pushed the ISNIT0/i18n branch from 496daad to 6cea681Compare 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()) {

Uh oh!

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!

vckttrJan 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!

vckttrJan 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!

vckttrJan 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

Uh oh!

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!

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

ISNIT0 commented Jan 14, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading

ISNIT0 commented Jan 14, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading

vckttrJan 16, 2025•
edited
Loading