wathiede/i3xs
1
0
Fork 1
Code Issues Pull Requests Releases Wiki Activity

Have DateTimeWidget include the time zone in its type. #1

Merged
wathiede merged 2 commits from ggriffiniii/i3xs:master into master 2019-10-03 11:19:52 -07:00
Conversation 1 Commits 2 Files Changed 3 +53 -58
3 changed files with 53 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/bin/home.rs
View File

@ -25,8 +25,8 @@ fn main() {
// Realtime upload/download rate for a interface
bar.push(NetworkSpeedWidget::new(&opts.nic, 6));
let mut dt = DateTimeWidget::new("%m/%d %H:%M".to_string());
dt.set_colors(&vec![
let mut dt = DateTimeWidget::new("%m/%d %H:%M");
dt.set_colors(vec![
TimeColor {
start: NaiveTime::from_hms(19, 0, 0),
color: ColorRGB::yellow(),

8
src/bin/work.rs
View File

@ -25,8 +25,8 @@ fn main() {
// Realtime upload/download rate for a interface
bar.push(NetworkSpeedWidget::new(&opts.nic, 6));
let mut dt = DateTimeWidget::tz("%H:%M %Z".to_string(), "Europe/London".to_string());
dt.set_colors(&vec![
let mut dt = DateTimeWidget::tz("%H:%M %Z", chrono_tz::Europe::London);
dt.set_colors(vec![
TimeColor {
start: NaiveTime::from_hms(0, 0, 0),
color: ColorRGB::red(),
@ -46,8 +46,8 @@ fn main() {
]);
bar.push(dt);
let mut dt = DateTimeWidget::new("%m/%d %H:%M".to_string());
dt.set_colors(&vec![
let mut dt = DateTimeWidget::new("%m/%d %H:%M");
dt.set_colors(vec![
TimeColor {
start: NaiveTime::from_hms(0, 0, 0),
color: ColorRGB::red(),

99
src/widgets/datetime.rs
View File

@ -1,19 +1,17 @@
use chrono::NaiveTime;
use chrono_tz::Tz;
use chrono::{offset::TimeZone, Local, NaiveTime, Utc};
use std::fmt::Display;
use i3monkit::Block;
use i3monkit::ColorRGB;
use i3monkit::{Widget, WidgetUpdate};
/// The widget that shows local time
pub struct DateTimeWidget {
pub struct DateTimeWidget<Tz> {
/// fmt to format the datetime, see
/// https://docs.rs/chrono/*/chrono/format/strftime/index.html for documentation.
fmt: String,
/// tz string, values from https://docs.rs/chrono-tz/*/chrono_tz/
/// None will use local time.
tz: Option<String>,
colors: Option<Vec<TimeColor>>,
tz: Tz,
colors: Vec<TimeColor>,
}
#[derive(Clone)]
@ -22,66 +20,63 @@ pub struct TimeColor {
pub color: ColorRGB,
}
impl DateTimeWidget {
// Create a new time widget
pub fn new(fmt: String) -> Self {
DateTimeWidget {
fmt,
tz: None,
colors: None,
}
}
pub fn tz(fmt: String, tz: String) -> Self {
DateTimeWidget {
fmt,
tz: Some(tz),
colors: None,
}
}
pub fn set_colors(&mut self, colors: &[TimeColor]) {
let mut colors = colors.to_vec();
colors.sort_by(|l, r| l.start.cmp(&r.start));
self.colors = Some(colors);
impl DateTimeWidget<Local> {
pub fn new(fmt: impl Into<String>) -> Self {
DateTimeWidget::tz(fmt, Local)
}
}
impl Widget for DateTimeWidget {
impl<Tz> DateTimeWidget<Tz>
where
Tz: TimeZone,
Tz::Offset: Display,
wathiede commented 2019-10-01 20:33:34 -07:00
Review
Copy Link

I don't get why "Tz::Offset: Display" is used here, can you explain that?

I don't get why "Tz::Offset: Display" is used here, can you explain that?
ggriffiniii commented 2019-10-01 22:54:56 -07:00
Review
Copy Link

Well I think it may not actually be necessary here (and should probably be removed), but it is necessary on the Widget impl. Short answer for why it's necessary there is because the compiler said so. Longer answer is because the Widget impl formats the time, which requires displaying the timezone which can only be done if the timezone's offset has a Display impl.

Well I think it may not actually be necessary here (and should probably be removed), but it is necessary on the Widget impl. Short answer for why it's necessary there is because the compiler said so. Longer answer is because the Widget impl formats the time, which requires displaying the timezone which can only be done if the timezone's offset has a Display impl.
{
pub fn tz(fmt: impl Into<String>, tz: Tz) -> Self {
DateTimeWidget {
fmt: fmt.into(),
tz,
colors: Vec::new(),
}
}
pub fn set_colors(&mut self, colors: impl Into<Vec<TimeColor>>) {
wathiede commented 2019-10-01 20:40:37 -07:00
Review
Copy Link

Now that I look at the docs, sort_by() is defined on slice. So this function only needs colors to be sort_by()'able, why make it something that can go into vec?

Is it because the thing eventually needs to be a Vec, so make the parameter Into?

I get a little confused what the better parameter is smart containers or references to base types. It seems like &str is better than String, if your function allows it, so I was thinking &[TimeColor] would be better than Vec. Thoughts?

Now that I look at the docs, sort_by() is defined on slice. So this function only needs colors to be sort_by()'able, why make it something that can go into vec? Is it because the thing eventually needs to be a Vec, so make the parameter Into<Vec>? I get a little confused what the better parameter is smart containers or references to base types. It seems like &str is better than String, if your function allows it, so I was thinking &[TimeColor] would be better than Vec<TimeColor>. Thoughts?
ggriffiniii commented 2019-10-01 23:20:20 -07:00
Review
Copy Link

Here are the benefits that I see, but I think there's plenty of room for reasonable people to disagree on these things.

Yes, it's because you eventually need a Vec to store in your struct. Since that's the end result there are potential efficiency gains by accepting anything that can be Into<Vec<TimeColor>> rather than &[TimeColor] and additional flexibility for the caller.

Consider the following ways to call the function if it accepts &[TimeColor]:

foo.set_colors(&[color1, color2]);
foo.set_colors(&vec![color1, color2]);

both of these will allocate a new vector within set_colors. The second case is really unfortunate because the caller just allocated their own vec just for the purpose of passing it to set_colors only to have set_colors allocate it's own identical copy.

Consider the following ways to call the function if it accepts impl Into<Vec<TimeColor>>:

foo.set_colors(&[color1, color2]);
foo.set_colors(&vec![color1, color2]);
foo.set_colors(vec![color1, color2]);

The first 2 are identical and result in identical code where set_colors allocates a new vector. The 3rd is the new variant. This will take the vec that's already created by the caller and pass it to set_colors and set_colors will simply sort it and assign it into the struct without needing to allocate a new vec.

Here are the benefits that I see, but I think there's plenty of room for reasonable people to disagree on these things. Yes, it's because you eventually need a `Vec` to store in your struct. Since that's the end result there are potential efficiency gains by accepting anything that can be `Into<Vec<TimeColor>>` rather than `&[TimeColor]` and additional flexibility for the caller. Consider the following ways to call the function if it accepts `&[TimeColor]`: ```rust foo.set_colors(&[color1, color2]); foo.set_colors(&vec![color1, color2]); ``` both of these will allocate a new vector within `set_colors`. The second case is really unfortunate because the caller just allocated their own vec just for the purpose of passing it to `set_colors` only to have `set_colors` allocate it's own identical copy. Consider the following ways to call the function if it accepts `impl Into<Vec<TimeColor>>`: ```rust foo.set_colors(&[color1, color2]); foo.set_colors(&vec![color1, color2]); foo.set_colors(vec![color1, color2]); ``` The first 2 are identical and result in identical code where `set_colors` allocates a new vector. The 3rd is the new variant. This will take the `vec` that's already created by the caller and pass it to `set_colors` and `set_colors` will simply sort it and assign it into the struct without needing to allocate a new `vec`.
let mut colors = colors.into();
colors.sort_by(|l, r| l.start.cmp(&r.start));
self.colors = colors;
}
fn now(&self) -> chrono::DateTime<Tz> {
Utc::now().with_timezone(&self.tz)
}
}
impl<Tz> Widget for DateTimeWidget<Tz>
where
Tz: TimeZone,
Tz::Offset: Display,
{
fn update(&mut self) -> Option<WidgetUpdate> {
let (time, time_string) = match &self.tz {
Some(tz) => {
let tz: Tz = tz.parse().unwrap();
let now = chrono::Local::now();
let now = now.with_timezone(&tz);
(now.time(), now.format(&self.fmt).to_string())
}
None => {
let now = chrono::Local::now();
(now.time(), now.format(&self.fmt).to_string())
}
};
let color = if let Some(colors) = &self.colors {
colors
.iter()
.filter(|tc| tc.start < time)
.last()
.map(|tc| tc.color.clone())
} else {
None
};
let now = self.now();
let time_string = format!(
r#"<span size="x-large" weight="heavy">{}</span>"#,
time_string
now.format(&self.fmt)
);
let mut data = Block::new()
.append_full_text(&time_string)
.use_pango()
.clone();
if let Some(color) = color {
let time = now.time();
if let Some(color) = self
.colors
.iter()
.filter(|tc| tc.start < time)
.last()
.map(|tc| tc.color.clone())
{
data.color(color);
}
Some(WidgetUpdate {
refresh_interval: std::time::Duration::new(1, 0),
refresh_interval: std::time::Duration::from_secs(1),
data: Some(data),
})
}