Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hour Cycle Preferences ignored #671

Closed
zbraniecki opened this issue Apr 22, 2021 · 4 comments · Fixed by #840
Closed

Hour Cycle Preferences ignored #671

zbraniecki opened this issue Apr 22, 2021 · 4 comments · Fixed by #840
Assignees
Labels
A-tailoring Area: User preferences, locale extensions, tailoring C-datetime Component: datetime, calendars, time zones S-small Size: One afternoon (small bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Milestone

Comments

@zbraniecki
Copy link
Member

I can't seem to make the preferences work in the length bag:

diff --git a/components/datetime/examples/work_log.rs b/components/datetime/examples/work_log.rs
index 390e44b..de434be 100644
--- a/components/datetime/examples/work_log.rs
+++ b/components/datetime/examples/work_log.rs
@@ -11,6 +11,7 @@ icu_benchmark_macros::static_setup!();
 
 use icu_datetime::mock::datetime::MockDateTime;
 use icu_datetime::{options::length, DateTimeFormat};
+use icu_datetime::options::preferences;
 use icu_locid::Locale;
 use icu_locid_macros::langid;
 
@@ -53,6 +54,9 @@ fn main(_argc: isize, _argv: *const *const u8) -> isize {
     let options = length::Bag {
         date: Some(length::Date::Medium),
         time: Some(length::Time::Short),
+        preferences: Some(preferences::Bag {
+            hour_cycle: Some(preferences::HourCycle::H23)
+        }),
         ..Default::default()
     };

I'd expect it to make cargo run --example work_log display 23 hour time, but it stays 11h.

@zbraniecki
Copy link
Member Author

This seems to be a carry over from 0.1 where it also didn't work.

@sffc sffc added A-tailoring Area: User preferences, locale extensions, tailoring C-datetime Component: datetime, calendars, time zones S-small Size: One afternoon (small bug fix or enhancement) T-bug Type: Bad behavior, security, privacy labels Apr 22, 2021
@sffc sffc added this to the ICU4X 0.3 milestone Apr 22, 2021
@zbraniecki
Copy link
Member Author

Ahh, I didn't realize Greg took it and started exploring it and thought "maybe it's trivial and can get it into 0.2?" - oh how foolish of me.

Greg, I'd be happy to collab with you on that, as this seems to me non-trivial.

I started looking into this and I'm no longer sure how we should approach it since length:Bag gets resolved pattern, and now we want to change the pattern based on preferences.

My initial take, that I started exploring in https://github.com/zbraniecki/icu4x/tree/length_preferences is that we would take a pattern and just apply the hour_cycle preferences as a two step algo:

  1. Filter out FieldSymbol::DayPeriod if H23/H24
  2. Swap h/K for H/K or reverse

But that unfortunately leads to two issues:

  1. If you remove DayPeriod field, you're left with literals surrounding it (in en HH:mm - notice the trailing space), and I'm not sure if we can assume that in all locales patterns without day period are the same as with, just with dayperiod removed)
  2. If you need to add day period, I don't think we should algorithmically try to detect the right position.

I looked into what SpiderMonkey is doing - https://searchfox.org/mozilla-central/source/js/src/builtin/intl/DateTimeFormat.cpp#749 - and it seems that anba ( I'm sorry Anba, I added the preferences to ECMA-402 style bag!) does:

  1. Find the best skeleton for the given pattern
  2. Swap h/H/k/K in skeleton for the preferences provided one
  3. Get the pattern for the new skeleton

That seems costly, but reasonable.

If we wanted to support it, we'd need to add get_skeleton_for_pattern, which I don't think we have yet, and decide whether we want to fine tune skeleton-pattern data structure to a bimap or do we consider this to be an outlier and okay with slower retrieval of get_skeleton_for_pattern.

I'm also wondering if we can leverage input skeleton symbols like j/J/C for this and for components preferences. Is there a way for us to construct logic like:

  1. Make our components::Bag and length::Bag coalesce on skeletons with j/J/C only
  2. Resolve j/J/C to h/H/k/K based on preferences if present, or default locale hour cycle if not present
  3. get pattern for skeleton created in (2)

This would mean that for cases where length::Bag is used without preferences, we're adding extraneous logic, but for components and for length bags where preferences are used it should result in comparable amount of instructions with more reliable architecture.
We could then add a fast-path for the length-without-prefs scenario.

Greg - thoughts?

@gregtatum
Copy link
Member

I haven't looked into this issue yet, so I don't have thoughts on your feedback so far. I'm only lightly attached to this issue so feel free to take over, or when I started working on it I'll provide feedback to the above.

@gregtatum
Copy link
Member

gregtatum commented Jun 30, 2021

Thanks for the brain dump Zibi. I looked at what you suggested.

#840

I think the approach here will be to invoke the skeleton matching machinery, but do it in a CLDR transform. It will double the memory size of the time styles, but it means we can have a fast path of not having to invoke that code at runtime. The two variants would be either the h or H variants, so dateStyle-h and dateStyle-H. Then either of those patterns could be selected, and the h11/h12 or h23/h24 could be swapped out in the pattern after it is retrieved. I think this would mitigate any larger regressions here in order to support this.

I have code now that does this at runtime, but I'd like to port the skeleton matching code to happen in the transfrom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tailoring Area: User preferences, locale extensions, tailoring C-datetime Component: datetime, calendars, time zones S-small Size: One afternoon (small bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants