greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.81k stars 472 forks source link

formatter[datetime]: fix timezone abbreviation (%Z) #2057

Closed bim9262 closed 1 month ago

bim9262 commented 1 month ago

Fixes #1965

What is the right behavior if the timezone abbreviation fails? Right now it will display an error, but a few other options would be to still display the old "TimezoneName" (ex -05:00) or fall back to printing just the long name like America/Chicago

Perhaps something like:

diff --git a/src/formatting/formatter/datetime.rs b/src/formatting/formatter/datetime.rs
index 7b288b906..bf506a184 100644
--- a/src/formatting/formatter/datetime.rs
+++ b/src/formatting/formatter/datetime.rs
@@ -121,25 +121,19 @@ impl TimezoneName<Tz> for Tz {

 impl TimezoneName<Local> for Local {
     fn timezone_name(datetime: &DateTime<Local>) -> Result<Box<str>> {
-        match iana_time_zone::get_timezone()
-            .error("Could not get local timezone")?
-            .parse::<Tz>()
-            .map_err(Error::new)?
-            .with_ymd_and_hms(
-                datetime.year(),
-                datetime.month(),
-                datetime.day(),
-                datetime.hour(),
-                datetime.minute(),
-                datetime.second(),
-            ) {
-            LocalResult::Single(datetime) => Ok(datetime
-                .offset()
-                .abbreviation()
-                .to_string()
-                .into_boxed_str()),
-            LocalResult::Ambiguous(..) => Err(Error::new("Timezone is ambiguous")),
-            LocalResult::None => Err(Error::new("Timezone is none")),
+        let tz_name = iana_time_zone::get_timezone().error("Could not get local timezone")?;
+        let tz = tz_name.parse::<Tz>().map_err(Error::new)?;
+
+        match tz.with_ymd_and_hms(
+            datetime.year(),
+            datetime.month(),
+            datetime.day(),
+            datetime.hour(),
+            datetime.minute(),
+            datetime.second(),
+        ) {
+            LocalResult::Single(datetime) => Tz::timezone_name(&datetime),
+            _ => Ok(tz_name.into_boxed_str()),
         }
     }
 }
@@ -161,7 +155,10 @@ impl Formatter for DatetimeFormatter {
                     for item in items {
                         new_items.push(match item {
                             Item::Fixed(format::Fixed::TimezoneName) => {
-                                Item::OwnedLiteral(T::timezone_name(&datetime)?)
+                                match T::timezone_name(&datetime) {
+                                    Ok(timezone) => Item::OwnedLiteral(timezone),
+                                    _ => Item::Fixed(format::Fixed::TimezoneName),
+                                }
                             }
                             item => item.to_owned(),
                         });

or

diff --git a/src/formatting/formatter/datetime.rs b/src/formatting/formatter/datetime.rs
index 7b288b906..5f066f767 100644
--- a/src/formatting/formatter/datetime.rs
+++ b/src/formatting/formatter/datetime.rs
@@ -106,40 +106,42 @@ where
     T: TimeZone,
     T::Offset: Display,
 {
-    fn timezone_name(datetime: &DateTime<T>) -> Result<Box<str>>;
+    fn timezone_name(datetime: &DateTime<T>) -> Item;
 }

 impl TimezoneName<Tz> for Tz {
-    fn timezone_name(datetime: &DateTime<Tz>) -> Result<Box<str>> {
-        Ok(datetime
-            .offset()
-            .abbreviation()
-            .to_string()
-            .into_boxed_str())
+    fn timezone_name(datetime: &DateTime<Tz>) -> Item {
+        Item::OwnedLiteral(
+            datetime
+                .offset()
+                .abbreviation()
+                .to_string()
+                .into_boxed_str(),
+        )
     }
 }

 impl TimezoneName<Local> for Local {
-    fn timezone_name(datetime: &DateTime<Local>) -> Result<Box<str>> {
-        match iana_time_zone::get_timezone()
-            .error("Could not get local timezone")?
-            .parse::<Tz>()
-            .map_err(Error::new)?
-            .with_ymd_and_hms(
-                datetime.year(),
-                datetime.month(),
-                datetime.day(),
-                datetime.hour(),
-                datetime.minute(),
-                datetime.second(),
-            ) {
-            LocalResult::Single(datetime) => Ok(datetime
-                .offset()
-                .abbreviation()
-                .to_string()
-                .into_boxed_str()),
-            LocalResult::Ambiguous(..) => Err(Error::new("Timezone is ambiguous")),
-            LocalResult::None => Err(Error::new("Timezone is none")),
+    fn timezone_name(datetime: &DateTime<Local>) -> Item {
+        let tz_name = match iana_time_zone::get_timezone() {
+            Ok(tz_name) => tz_name,
+            _ => return Item::Fixed(format::Fixed::TimezoneName),
+        };
+        let tz = match tz_name.parse::<Tz>() {
+            Ok(tz) => tz,
+            _ => return Item::OwnedLiteral(tz_name.into_boxed_str()),
+        };
+
+        match tz.with_ymd_and_hms(
+            datetime.year(),
+            datetime.month(),
+            datetime.day(),
+            datetime.hour(),
+            datetime.minute(),
+            datetime.second(),
+        ) {
+            LocalResult::Single(tz_datetime) => Tz::timezone_name(&tz_datetime).to_owned(),
+            _ => Item::OwnedLiteral(tz_name.into_boxed_str()),
         }
     }
 }
@@ -160,9 +162,7 @@ impl Formatter for DatetimeFormatter {
                     let mut new_items = Vec::with_capacity(items.len());
                     for item in items {
                         new_items.push(match item {
-                            Item::Fixed(format::Fixed::TimezoneName) => {
-                                Item::OwnedLiteral(T::timezone_name(&datetime)?)
-                            }
+                            Item::Fixed(format::Fixed::TimezoneName) => T::timezone_name(&datetime),
                             item => item.to_owned(),
                         });
                     }
MaxVerevkin commented 1 month ago

@bim9262 Didn't you want to make it opt-in?

bim9262 commented 1 month ago

@bim9262 Didn't you want to make it opt-in?

Then I found that there's %z which can be used instead of %Z

bim9262 commented 1 month ago

Should we use make_log_macro!(error, "datetime"); instead of make_log_macro!(debug, "datetime");

MaxVerevkin commented 1 month ago

Should we use make_log_macro!(error, "datetime"); instead of make_log_macro!(debug, "datetime");

That would make sense in this case.