Add device localization on ios #532

filip131311 · 2024-09-10T15:55:54Z

This PR introduces a functionality for changing device localization on ios.

Screen.Recording.2024-09-10.at.17.51.32.mov

vercel · 2024-09-10T15:55:57Z

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

Name	Status	Preview	Comments	Updated (UTC)
react-native-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 19, 2024 0:47am

kmagiera

added a few comments inline. As they may impact parts of the implementation I'll be taking another look once you respond to the comments

kmagiera · 2024-09-11T10:33:34Z

packages/vscode-extension/src/webview/views/DeviceLocalizatioView.tsx

+
+type LocaleWithDescription = { localeIdentifier: Locale; Description: string };
+
+export function DeviceLocalizationView() {


The filename is missing "n" packages/vscode-extension/src/webview/views/DeviceLocalizatioView.tsx

kmagiera · 2024-09-11T10:34:23Z

packages/vscode-extension/src/webview/utilities/localeList.json

@@ -0,0 +1,318 @@
+[


Is this needed? If so explain why in the PR. We can perhaps use some library or built-in localization APi for this, no?

This is the best solution I could think of there are libraries that do this kind of work but none is doing exactly what I need.
this one maps from country to codes not from language.
this one wasn't updated for a long time

and libraries responsible for localization management add unnecessary complexity.

I got the list in the pull request by taking a product of codes available on iOS and on android.

kmagiera · 2024-09-11T10:34:59Z

packages/vscode-extension/src/webview/views/DeviceLocalizatioView.tsx

+  return (
+    <div className="localization-container">
+      <div className="search-bar">
+        <input


should we autofocus this when the modal is shown?

kmagiera · 2024-09-11T10:37:30Z

packages/vscode-extension/src/project/project.ts

+  public async restart(
+    forceCleanBuild: boolean,
+    onlyReloadJSWhenPossible: boolean = true,
+    restartDevice?: boolean


would be cleaner if this had a default value like the previous paameter, in this case if it was set to false it'd work the same way it does now but the type would be strict

kmagiera · 2024-09-11T11:02:26Z

packages/vscode-extension/src/project/deviceSession.ts

+    if (await this.device.checkLocale(deviceSettings.locale)) {
+      await this.device.changeLocale(deviceSettings.locale);
+      await this.device.resetDevice();
+    }


seems like this is implementation detail for a given device, shouldn't it be a part of changeSetting implementation that's specific to device rather than being handled in a generic way?

kmagiera · 2024-09-11T11:04:00Z

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

+      deviceSetLocation,
+      "spawn",
+      this.deviceUDID,
+      "defaults",


I wonder if we could simplify the process and avoid restarts if we use the same method for updating defaults as we do for things like schemeapproval list (see below)

Unfortunately no, we could change the way we modify relavent, plist but we still have no way to modify what operating system keeps in memory at runtime.

ok, but we could still update that before the boot. Now we boot the device to only update this setting and the reboot.

you were right I was able to avoid reset on on boot

kmagiera

Leaving a couple more comments inline

kmagiera · 2024-09-13T08:15:28Z

packages/vscode-extension/src/webview/views/DeviceLocalizationView.tsx

+  return (
+    <div className="localization-container">
+      <div className="search-bar">
+        <input


can we automatically focus search field when the modal is open?

I already did that, there is a bug in the frontend that persist focus on deviceSettingsDropdown after it was closed, but it is outside of scope for this PR so I'll look into it in another one

here is an example of how the component works when deviceSettingsDropdown is not focused:
https://github.com/user-attachments/assets/b31fd863-c4c6-4062-8913-b20004683d00

packages/vscode-extension/src/project/project.ts

kmagiera · 2024-09-13T08:27:10Z

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

+
+    if (await this.shouldUpdateLocale(deviceSettings.locale)) {
+      await this.changeLocale(deviceSettings.locale);
+      await this.resetDevice();


given we call internalBootDevice as a result of this call here, would it make sense to do that before the previous call to inernalBootDevice ? This way we can avoid booting it twice

It is neccesery to boot it twice i added a coment explaining that, in short we use adb shell to change locale so device needs to be booted. The solution you provided (-change-locale) unfortunately has the same short comings as just using adb shell setprop persist.sys.locale and moreover boots device twice anyway (performs cold boot after changing locale).

kmagiera · 2024-09-13T09:11:48Z

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

+    // but if the user has more then languages set up then a setting in settings_system are created
+    // in such a case we need to change them too. It seems that if the setting exists in
+    // settings_system it is persistent even if the number of locales drops back to one.
+    await exec(ADB_PATH, ["-s", this.serial!, "shell", "setprop", "persist.sys.locale", locale]);


Have you tried using -change-language and change-locale options from emulator:

I tried and they seem to work, although when used they trigger cold boot of the device. So perhaps worth combining with some cached information about the current language

unfortunately, this approach won't work because it does the same thing as adb shell setprop persist.sys.locale ${locale} witch means it works only as long as locales are not set in /data/system/users/0/settings_system.xml (system settings) witch will override the persist.sys.locale prop. Moreover the approach with emulator cli tool does not provide significant benefits as it still turns on the device before reseting it agin to load changes.

The easiest way to reproduce this problem is to on device go to settings>system>languages and add more then one language that will ensure that /data/system/users/0/settings_system.xml has system_locales field

kmagiera

Added small comment about wording in the popup dialog. Otherwise looks good 👍

kmagiera · 2024-09-19T12:01:17Z

packages/vscode-extension/src/webview/views/DeviceLocalizationView.tsx

+      <h2 className="localization-change-title">
+        Are you sure you want to change the localization to <i>{locale.Description}</i>?
+      </h2>
+      <p className="localization-change-subtitle">This action will reset the device.</p>


"reset" is a wrong wording here. This may indicate we want to clear the device settings or something while we only want to reboot the device. I'd rather state it
"Changing localization requires rebooting the simulator"

filip131311 requested review from kmagiera, kacperkapusciak and jakub-gonet September 10, 2024 15:57

kmagiera reviewed Sep 11, 2024

View reviewed changes

vercel bot deployed to Preview September 11, 2024 14:52 View deployment

vercel bot deployed to Preview September 11, 2024 14:54 View deployment

filip131311 requested a review from kmagiera September 11, 2024 14:58

vercel bot deployed to Preview September 12, 2024 16:18 View deployment

filip131311 changed the title ~~Add device localization for iOS~~ Add device localization Sep 12, 2024

vercel bot deployed to Preview September 12, 2024 16:19 View deployment

kmagiera reviewed Sep 13, 2024

View reviewed changes

vercel bot deployed to Preview September 16, 2024 08:35 View deployment

vercel bot deployed to Preview September 16, 2024 08:43 View deployment

ios

a75b756

filip131311 force-pushed the @Filip131311/AddDeviceLocalization branch from baf62da to a75b756 Compare September 19, 2024 10:30

filip131311 requested a review from kmagiera September 19, 2024 10:30

vercel bot deployed to Preview September 19, 2024 10:30 View deployment

filip131311 changed the title ~~Add device localization~~ Add device localization on ios Sep 19, 2024

filip131311 added 2 commits September 19, 2024 12:33

Merge branch 'main' into @Filip131311/AddUiForLaunchCOnfiguration

695ddd7

ios

e3ed46b

vercel bot deployed to Preview September 19, 2024 10:34 View deployment

kmagiera approved these changes Sep 19, 2024

View reviewed changes

filip131311 added 4 commits September 19, 2024 14:08

changes after cr

38ecde7

Merge branch 'main' into @Filip131311/AddDeviceLocalization

0ab7248

Merge branch 'main' into @Filip131311/AddDeviceLocalization

8d897a6

changes after CR

01fccb0

vercel bot deployed to Preview September 19, 2024 12:47 View deployment

filip131311 merged commit e6292db into main Sep 19, 2024
3 checks passed

filip131311 deleted the @Filip131311/AddDeviceLocalization branch September 19, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add device localization on ios #532

Add device localization on ios #532

filip131311 commented Sep 10, 2024 •

edited

Loading

vercel bot commented Sep 10, 2024 •

edited

Loading

kmagiera left a comment

kmagiera Sep 11, 2024

kmagiera Sep 11, 2024

filip131311 Sep 11, 2024

kmagiera Sep 11, 2024

kmagiera Sep 11, 2024

kmagiera Sep 11, 2024

kmagiera Sep 11, 2024

filip131311 Sep 11, 2024

kmagiera Sep 12, 2024

filip131311 Sep 12, 2024

kmagiera left a comment

kmagiera Sep 13, 2024

filip131311 Sep 16, 2024

kmagiera Sep 13, 2024

filip131311 Sep 16, 2024

kmagiera Sep 13, 2024

filip131311 Sep 16, 2024

filip131311 Sep 16, 2024

kmagiera left a comment

kmagiera Sep 19, 2024


		type LocaleWithDescription = { localeIdentifier: Locale; Description: string };

		export function DeviceLocalizationView() {

Add device localization on ios #532

Add device localization on ios #532

Conversation

filip131311 commented Sep 10, 2024 • edited Loading

vercel bot commented Sep 10, 2024 • edited Loading

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

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

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

filip131311 commented Sep 10, 2024 •

edited

Loading

vercel bot commented Sep 10, 2024 •

edited

Loading