Add Localization on Android #590

filip131311 · 2024-10-03T15:36:46Z

This PR adds localization on android.

Screen.Recording.2024-10-03.at.16.47.57.mov

Additional context:

How Android changes the locale settings when the user does it in the settings app?

Android changes the memory responsible for holding information about locale and tigers a change in two types of persistent storage, one is system prop persist.sys.locale and the second one is system_locales prop in system level settings.
Before user changes the settings both of those fields are empty. When Android boots it either sets a locale to the default one or to the one specified in one of the persistent locations, it seems that if only one location exist android will still retrive the information, but conflict between persist.sys.locale and system_locales leads to unpredictable behavior.

How to change the properties with adb?

adb allows us to change system properties including persist.sys.locale granted we have a root access, this is achieved either by using adb root command or su command inside shell. The root access is only possible on emulators without GMS (google mobile services). adb also allows us to chage system settings using adb shell settings command and it does not require root access, but permanent state change of system_settings require -writable-system flag to be added on emulator boot, so we do it.

How emulator command changes settings when using `-change-locale` option?

Underneath emulator calls adb shell su 0 setprop persist.sys.locale ${locale}.

Uncovered case:

Because of the above I did not find a way to reliably change persist.sys.locale and scenario in which user manually changes locale (in the settings app).

Test Plan:

Changing localization while using the device

turn on IDE and select android device
check if localization settings are disabled during boot process:

change devices localization after it was booted up

Changing localization while using other available device

turn on IDE and select ios device
change devices localization after it was booted up
switch devices to android and check if the localization was configured correctly

vercel · 2024-10-03T15:36:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
radon-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 23, 2024 6:03pm

jakub-gonet · 2024-10-03T15:58:49Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    ]);
+
+    // if user did not use the device before it might not have system_locales property
+    // as en-US is the default locale we assume that no value is the same as en-US


"we assume that no value is the same as en-US" I don't understand this part.

If there is no value and the user selected locale is en-US I don't want to trigger locale change as it is already a correct one

jakub-gonet · 2024-10-03T16:00:32Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+
+  // it is necessary to use SIGTERM 9 as it would take to much time otherwise and would degrade user experience
+  // having said that use this function with caution only in scenarios when you are sure that no user data will be lost
+  private async resetDevice() {


forcefullyResetDevice to signify this isn't typical reboot sequence?

jakub-gonet · 2024-10-03T16:02:01Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+  // If the device uses google mobile services persist.sys.locale can not be changed, as it requires a root access
+  // TODO:  Find a way to change or remove persist.sys.locale without root access. note: removing the whole
+  // data/property/persistent_properties file would also work as we persist device settings globally in Radon IDE
+  private async changeLocale(newLocale: Locale): Promise<boolean> {


Isn't it always returning true? What does the return value mean?

kmagiera · 2024-10-04T10:21:36Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    ]);
+
+    if (!stdout) {
+      return true;


add some logging here such that we can diagnose it for the users who may've set the language using settings app

kmagiera · 2024-10-04T10:22:41Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    // If persist.sys.locale exist try to set it to match the newLocale.
+    try {
+      await exec(ADB_PATH, [
+        "-s",
+        this.serial!,
+        "shell",
+        "su",
+        "0",
+        "setprop",
+        "persist.sys.locale",
+        locale,
+      ]);
+    } catch (e) {
+      // this is expected if device is using google mobile services
+      Logger.debug(
+        "CHANGE LOCALE - Device is not allowing root access, moving on without modifying persist.sys.locale"
+      );
+    }
+
+    return true;


I think it should be ok to remove this part. It is unlikely that someone would change the language via settings, and even if they do, it is unlikely you can actually use sudo on Android devices as it is only available on pure AOSP builds which are rarely used in practice

kmagiera

Please see my inline comments and add test plan to PR description.

I believe the way you tested this was by changing it while the Android device was booted. But what happen when you change language on iOS and then switch to Android, or when you change the language before the device is fully booted.

kmagiera · 2024-10-11T11:17:10Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    ]);
+
+    // this is needed to make sure that changes will persist
+    await exec(ADB_PATH, ["shell", "sync"]);


why we don't pass "-s", this.serial here?

it was a mistake thank you

kmagiera · 2024-10-11T11:17:48Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    // this is needed to make sure that changes will persist
+    await exec(ADB_PATH, ["shell", "sync"]);
+
+    if (stdout) {


would feel more natural if we made this check before the other exec call given you use a transient stdout variable

kmagiera

Almost there, I left a few more comments inline

kmagiera · 2024-10-23T10:57:10Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+  // it is necessary to use SIGTERM 9 as it would take to much time otherwise and would degrade user experience
+  // having said that use this function with caution only in scenarios when you are sure that no user data will be lost


For method documentation we use ``/** */` comments format, but this doesn't seem to be one actually. If you want to document method it should be structured in a way that describes what the method does rather than what is necessary and for what. For example, method docs would say something along the lines "This method restarts the emulator process using SIGKILL signal. Should be used for the situations when quick reboot is necessary and when we don't care about the emulator's process state"

On the other hand, if you want to comment why we use "9", this could be an in-line comment in the code block directly preceeding the kill call, and it could state: "We use 9 (SIGKILL) to terminate the process immediatly, as terminating it gracefully typically takes some significant time"

kmagiera · 2024-10-23T10:59:19Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    // We have to initially boot the device because we use adb shell to change the devices locale
+    // alternative would be to use -change-locale option on the emulator, but that option restarts the device too
+    // the -change-locale option is not covering all cases read more about it in this.changeLocale


This comment doesn't belong here. Why you explain stuff related to changing locale in a place that isn't super relevant. Here we're already using an abstraction, that is "changeSetting" method which returns whether the emulator process should be rebooted. Discussing internals of changeSetting at this level is an abstraction leak.

kmagiera · 2024-10-23T11:00:11Z

packages/vscode-extension/src/webview/components/DeviceSettingsDropdown.tsx

  return (
    <>
      <Label>Device Localization</Label>
      <DropdownMenu.Item
+        disabled={projectState.status !== "running"}


similar change already landed in #643 – we likely want to use that one instead

kmagiera · 2024-10-23T11:02:53Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    return true;
+  }
+
+  // this method changes device locale in two ways, firstly it modifies system_locales setting,


please read my other comment about method level comments I posted below.

kmagiera

Added a couple more inline comments Accepting to unblock this but please address the comments before merging

kmagiera · 2024-10-23T16:49:39Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    ]);
+
+    // if user did not use the device before it might not have system_locales property
+    // as en-US is the default locale we assume that no value is the same as en-US


I don't understand what does it mean "no value is the same as en-US"

I modified the comment to explain it better

kmagiera · 2024-10-23T16:50:09Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+    return true;
+  }
+
+  /**  This method changes device locale by modifying system_locales system setting. */


Suggested change

/** This method changes device locale by modifying system_locales system setting. */

/**

* This method changes device locale by modifying system_locales system setting.

*/

kmagiera · 2024-10-23T16:53:15Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+
+    if (stdout) {
+      Logger.warn(
+        "persist.sys.locale detected while changing locale. It might indicate that the user has changed locale settings in the settings application, which will prevent localization change."


Suggested change

"persist.sys.locale detected while changing locale. It might indicate that the user has changed locale settings in the settings application, which will prevent localization change."

"Updating locale will not take effect as the device has altered locale via system settings which always takes precedence over the device setting the IDE uses."

kmagiera · 2024-10-23T16:53:37Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

+  /** This method restarts the emulator process using SIGKILL signal.
+   *  Should be used for the situations when quick reboot is necessary
+   *  and when we don't care about the emulator's process state */


Suggested change

/** This method restarts the emulator process using SIGKILL signal.

* Should be used for the situations when quick reboot is necessary

* and when we don't care about the emulator's process state */

/**

* This method restarts the emulator process using SIGKILL signal.

* Should be used for the situations when quick reboot is necessary

* and when we don't care about the emulator's process state

*/

kmagiera · 2024-10-23T16:54:46Z

packages/vscode-extension/src/devices/AndroidEmulatorDevice.ts

@@ -144,6 +222,7 @@ export class AndroidEmulatorDevice extends DeviceBase {
        "-no-boot-anim",
        "-grpc-use-token",
        "-no-snapshot-save",
+        "-writable-system",


what is this option? why are we adding it here? It should be explained in PR description

filip131311 added 2 commits October 3, 2024 16:38

init

eff5b0c

init2

5c93259

jakub-gonet reviewed Oct 3, 2024

View reviewed changes

kmagiera requested changes Oct 4, 2024

View reviewed changes

changes after CR

cd2a1bc

vercel bot deployed to Preview October 4, 2024 13:15 View deployment

changes after CR

eb14f0c

filip131311 requested a review from kmagiera October 4, 2024 13:21

vercel bot deployed to Preview October 4, 2024 13:21 View deployment

filip131311 added 2 commits October 10, 2024 11:07

setTimeout workaround

1cc4a2f

add qemu.img syncronisation

f2eaba1

vercel bot deployed to Preview October 10, 2024 10:20 View deployment

kmagiera requested changes Oct 11, 2024

View reviewed changes

changes after CR

2e54c3f

vercel bot deployed to Preview October 11, 2024 16:14 View deployment

kmagiera mentioned this pull request Oct 21, 2024

Disable device settings during booting and UI zoom changes #643

Merged

kmagiera requested changes Oct 23, 2024

View reviewed changes

filip131311 added 2 commits October 23, 2024 16:22

changes in coments

4cced63

Merge branch 'main' into @Filip131311/AddAndroidLocalization

f080b13

vercel bot deployed to Preview October 23, 2024 14:23 View deployment

filip131311 requested a review from kmagiera October 23, 2024 16:25

kmagiera approved these changes Oct 23, 2024

View reviewed changes

fixes after CR

22e981c

vercel bot deployed to Preview October 23, 2024 18:03 View deployment

filip131311 merged commit cccc52c into main Oct 23, 2024
4 checks passed

filip131311 deleted the @Filip131311/AddAndroidLocalization branch October 23, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Localization on Android #590

Add Localization on Android #590

filip131311 commented Oct 3, 2024 •

edited

Loading

vercel bot commented Oct 3, 2024 •

edited

Loading

jakub-gonet Oct 3, 2024

filip131311 Oct 4, 2024

jakub-gonet Oct 3, 2024

jakub-gonet Oct 3, 2024

kmagiera Oct 4, 2024

kmagiera Oct 4, 2024

kmagiera left a comment

kmagiera Oct 11, 2024

filip131311 Oct 11, 2024

kmagiera Oct 11, 2024

kmagiera left a comment

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera left a comment

kmagiera Oct 23, 2024

filip131311 Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

kmagiera Oct 23, 2024

		// it is necessary to use SIGTERM 9 as it would take to much time otherwise and would degrade user experience
		// having said that use this function with caution only in scenarios when you are sure that no user data will be lost

	"persist.sys.locale detected while changing locale. It might indicate that the user has changed locale settings in the settings application, which will prevent localization change."
	"Updating locale will not take effect as the device has altered locale via system settings which always takes precedence over the device setting the IDE uses."

Add Localization on Android #590

Add Localization on Android #590

Conversation

filip131311 commented Oct 3, 2024 • edited Loading

Additional context:

How Android changes the locale settings when the user does it in the settings app?

How to change the properties with adb?

How emulator command changes settings when using -change-locale option?

Uncovered case:

Test Plan:

vercel bot commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filip131311 commented Oct 3, 2024 •

edited

Loading

How emulator command changes settings when using `-change-locale` option?

vercel bot commented Oct 3, 2024 •

edited

Loading