System locale #5081

ashu26jha · 2024-06-19T05:28:24Z

Total tasks:

Retrieval of system locale for MacOS, Windows, Linux
Calendar preferences
Time Zone
Convert String into icu4x components

Optional:

Number preferences
Currency preferences
Temperature
First Day of week

ashu26jha · 2024-06-19T05:33:09Z

utils/system_locale/src/linux.rs

+            if !locale_ptr.is_null() {
+                let c_str = CStr::from_ptr(locale_ptr);
+                if let Ok(str_slice) = c_str.to_str() {
+                    return vec![(str_slice.to_string(), String::from("Gregorian"))];


Gregorian is default calendar for linux (doesn't change even changing the locale, won't work unless install extensions). Ubuntu uses GNOME calendar. Here is something in their repo: https://gitlab.gnome.org/GNOME/gnome-calendar/-/issues/998

please add this as a code comment

still needs to be done

robertbastian · 2024-06-19T09:20:11Z

utils/system_locale/src/lib.rs

+pub fn get_locale() -> Vec<String> {
+    #[cfg(target_os = "linux")]
+    return linux::get_locale();
+
+    #[cfg(target_os = "macos")]
+    return apple::get_locale();
+
+    #[cfg(target_os = "windows")]
+    return windows::get_locale();
+}
+
+pub fn get_system_calendar() -> Vec<(String, String)> {
+    #[cfg(target_os = "linux")]
+    return linux::get_system_calendar();
+
+    #[cfg(target_os = "macos")]
+    return apple::get_system_calendar();
+
+    #[cfg(target_os = "windows")]
+    return windows::get_system_calendar();
+}


reexport the methods instead of wrapping them:

Suggested change

pub fn get_locale() -> Vec<String> {

#[cfg(target_os = "linux")]

return linux::get_locale();

#[cfg(target_os = "macos")]

return apple::get_locale();

#[cfg(target_os = "windows")]

return windows::get_locale();

}

pub fn get_system_calendar() -> Vec<(String, String)> {

#[cfg(target_os = "linux")]

return linux::get_system_calendar();

#[cfg(target_os = "macos")]

return apple::get_system_calendar();

#[cfg(target_os = "windows")]

return windows::get_system_calendar();

}

#[cfg(target_os = "linux")]

pub use linux::*;

#[cfg(target_os = "macos")]

pub use apple::*;

#[cfg(target_os = "windows")]

pub use windows::*;

still needs to be done?

utils/system_locale/src/linux.rs

robertbastian · 2024-06-19T09:30:46Z

utils/system_locale/src/linux.rs

+        let mut locale_map = HashMap::new();
+
+        if let Ok(str_slice) = c_str.to_str() {
+            if str_slice.contains(';') {


contains requires a full scan of the string, try to write the algorithm with just split.

robertbastian · 2024-06-19T09:31:54Z

utils/system_locale/src/linux.rs

+        ptr::null,
+    };
+
+    pub unsafe fn fetch_locale_settings() -> HashMap<i32, String> {


this method should be either be safe, or private. If safe, any unsafeness should be encapsulated inside. If unsafe, it should have a comment stating the requirements for it to be safe.

I'd suggest abstracting the LC_* to an enum and have this return HashMap<THIS_ENUM, Locale>

utils/system_locale/src/linux.rs

robertbastian · 2024-06-19T09:44:43Z

utils/system_locale/src/lib.rs

+    return windows::get_locale();
+}
+
+pub fn get_system_calendar() -> Vec<(String, String)> {


also, plural?

get_env_calendars ?

still needs to be done for windows

robertbastian · 2024-06-19T09:46:37Z

utils/system_locale/src/linux.rs

+#[cfg(target_os = "linux")]
+mod linux_locale {


This file (= module) is already gated on linux, you don't need a nested module.

Suggested change

#[cfg(target_os = "linux")]

mod linux_locale {

still nested

utils/system_locale/src/linux.rs

utils/system_locale/src/apple.rs

utils/system_locale/src/windows.rs

zbraniecki · 2024-07-02T17:15:43Z

Cargo.toml

@@ -66,6 +66,7 @@ members = [
    "utils/litemap",
    "utils/pattern",
    "utils/resb",
+    "utils/system_locale",


This name suggests that the crate only retrieves "system" "locale". My understanding is that your crate aims to retrieve environment information from the runtime. I'd use a term like "environment", "preferences" etc.

My concern about the "system" is that it indicates that what you get is "OS Locale" - but you may not. Increasingly OSes sandbox applications, so OS may be in English, but the application may only see that the language of its own environment is French.
My concern about "locale" is that it indicates that all you get is a Unicode Locale - while what you actually get are things beyond that. For example, short date pattern is not part of a Locale but should be retrievable by this crate.

Maybe "utils/env_preferences"?

zbraniecki · 2024-07-02T17:16:48Z

utils/system_locale/src/lib.rs

+    return windows::get_locale();
+}
+
+pub fn get_system_calendar() -> Vec<(String, String)> {


get_env_calendars ?

zbraniecki · 2024-07-02T17:18:58Z

utils/system_locale/src/linux.rs

+        ptr::null,
+    };
+
+    pub unsafe fn fetch_locale_settings() -> HashMap<i32, String> {


I'd suggest abstracting the LC_* to an enum and have this return HashMap<THIS_ENUM, Locale>

robertbastian

There are still a lot of unaddressed comments from my first review

robertbastian · 2024-07-08T08:59:34Z

utils/env_preferences/tests/test.rs

+// called LICENSE at the top level of the ICU4X source tree
+// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).
+
+// The test is just here to silence the compiler warning, meaningful test will be added later


this should not be required, what's the compiler warning?

robertbastian · 2024-07-08T09:00:17Z

tools/graveyard/Cargo.lock

issue: remove

robertbastian · 2024-07-08T09:03:18Z

utils/env_preferences/src/apple.rs

+    };
+    use std::ffi::CStr;
+
+    pub fn get_locales_mac() -> Vec<String> {


mac? apple? macos? You shouldn't need suffixes, as all these methods are in the apple namespace already.

robertbastian · 2024-07-08T09:03:40Z

utils/env_preferences/src/linux.rs

+        locale_map
+    }
+
+    pub fn get_locales_linux() -> Vec<String> {


suffixes also not needed

robertbastian · 2024-07-08T09:05:36Z

utils/system_locale/src/linux.rs

+            if !locale_ptr.is_null() {
+                let c_str = CStr::from_ptr(locale_ptr);
+                if let Ok(str_slice) = c_str.to_str() {
+                    return vec![(str_slice.to_string(), String::from("Gregorian"))];


still needs to be done

robertbastian · 2024-07-08T09:10:09Z

utils/env_preferences/src/linux.rs

+                            Cow::Owned(str_slice.to_string()),
+                            Cow::Borrowed("Gregorian"),
+                        ))
+                        .into_iter(),


If you add

Suggested change

.into_iter(),

.into_iter().chain(None),

then both paths return the same type, and you don't need to wrap things in boxes and use dyn.

robertbastian · 2024-07-08T09:12:00Z

utils/system_locale/src/linux.rs

+    pub fn get_locale() -> Vec<String> {
+        let mut unique_locales = HashSet::new();
+        unsafe {
+            let locale_map = fetch_locale_settings();
+            for value in locale_map.values() {
+                unique_locales.insert(value.clone());
+            }
+        }
+        unique_locales.into_iter().collect()
+    }


still needs to be done, now it's just fetch_locale_settings().into_values()

robertbastian · 2024-07-08T09:12:30Z

utils/system_locale/src/lib.rs

+#[cfg(target_os = "windows")]
+mod windows;
+
+pub fn get_locale() -> Vec<String> {


still needs to be done for windows

robertbastian · 2024-07-08T09:12:55Z

utils/system_locale/src/lib.rs

+    return windows::get_locale();
+}
+
+pub fn get_system_calendar() -> Vec<(String, String)> {


still needs to be done for windows

robertbastian · 2024-07-08T09:13:04Z

utils/system_locale/src/linux.rs

+#[cfg(target_os = "linux")]
+mod linux_locale {


still nested

ashu26jha added 8 commits June 2, 2024 23:17

Linux support added

1c7b634

improved interface of linux

f6b96a9

calendar for linux

df361b8

added readme

a3d8154

apple locale and calendar

bb95d79

windows locale retrieval

ee23fd2

windows calendar added

1b1b514

Update Readme

9b3d8e5

ashu26jha commented Jun 19, 2024

View reviewed changes

robertbastian reviewed Jun 19, 2024

View reviewed changes

zbraniecki reviewed Jul 2, 2024

View reviewed changes

jasonwilliams mentioned this pull request Jul 3, 2024

Tracking issue for Now namespace boa-dev/temporal#71

Open

2 tasks

ashu26jha and others added 8 commits July 5, 2024 09:48

Fixed linux

0adf4b9

apple fi

fe54b79

Minor fix

d7ca69e

Change module name

1c10d33

rustdocs

c243451

readme license name fix

0aa332e

Rename Readme.md to README.md

e96b2d3

Change crate name

f7212b5

ashu26jha marked this pull request as ready for review July 7, 2024 06:18

ashu26jha requested a review from a team as a code owner July 7, 2024 06:18

ashu26jha requested review from zbraniecki and robertbastian July 7, 2024 06:18

robertbastian reviewed Jul 8, 2024

View reviewed changes

ashu26jha added 5 commits July 19, 2024 18:46

Added comment for calendar in linux.rs

aba5c24

Re-export modules

72c0404

Fix bug in locale retrieval

6dc1973

Added enum in linux.rs

7cc7c65

tools/graveyard/Cargo.lock removed

4c594be

fmt

514f4a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System locale #5081

System locale #5081

ashu26jha commented Jun 19, 2024 •

edited

Loading

ashu26jha Jun 19, 2024

robertbastian Jun 19, 2024

robertbastian Jul 8, 2024

robertbastian Jun 19, 2024

robertbastian Jul 8, 2024

robertbastian Jun 19, 2024

robertbastian Jun 19, 2024

zbraniecki Jul 2, 2024

robertbastian Jul 8, 2024

robertbastian Jun 19, 2024

zbraniecki Jul 2, 2024

robertbastian Jul 8, 2024

robertbastian Jun 19, 2024

robertbastian Jul 8, 2024

zbraniecki Jul 2, 2024

zbraniecki Jul 2, 2024

zbraniecki Jul 2, 2024

robertbastian left a comment

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

robertbastian Jul 8, 2024

System locale #5081

Are you sure you want to change the base?

System locale #5081

Conversation

ashu26jha commented Jun 19, 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

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

robertbastian 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

ashu26jha commented Jun 19, 2024 •

edited

Loading