Add duration formatter

[bim9262]

TODO: document duration formatter.

Any thoughts on the names of the arguments, the format, or the defaults?

The formatted can be used by the battery, uptime, tea_timer and pomodoro blocks.

[MaxVerevkin]

It is hard to judge/review until there is at least one block using this. The good thing is that blocks can be migrated in a non-breaking way, for example tea_timer can still have the old (and eventually deprecated and then removed) hours/minutes/seconds, in addition to the new placeholder.

By the way, I think it is safe to delete fix.rs for now.

[bim9262]

Added two examples, uptime and tea_timer. Still need to doc the formatter...

[MaxVerevkin]

Do I understand correctly that currently there is no way to use mm:ss format? I think this can be achieved by allowing to replace the space character between units with : and adding an option to hide units. Maybe we can even add a shortcut parameter to set all these values, because .dur(units:6,show_leading_units_if_zero:false,separator:':',show_units:false) is too much.

By the way, maybe show_leading_units_if_zero should default to false? I'm not %100 sure though.

[bim9262]

Do I understand correctly that currently there is no way to use mm:ss format? I think this can be achieved by allowing to replace the space character between units with : and adding an option to hide units. Maybe we can even add a shortcut parameter to set all these values, because .dur(units:6,show_leading_units_if_zero:false,separator:':',show_units:false) is too much.

That's right. If you wanted to do [hh]:mm:ss you'd want to do something like .dur(max_unit:h,min_unit:s,show_leading_units_if_zero:false,separator:':',show_units:false) otherwise after you get to a day+ you'd end up with a format like dd:hh:mm:ss, which probably isn't a good format.

The most flexible solution would be to allow an explicit format to be specified if we want to allow "sub-formats" like: .dur(format:'{$d |}{$h:|}{$m:|}'{$s:|}',show_leading_units_if_zero:false)

or using the allow argument mentioned in https://github.com/greshake/i3status-rust/issues/1957: .dur(format:'{$d.eng(allow:1..) |}{$h.eng(allow:1..):|}{$m.eng(allow:1..):|}'{$s.eng(allow:1..):|}')

By the way, maybe show_leading_units_if_zero should default to false? I'm not %100 sure though.

I set show_leading_units_if_zero to true, because it means that say you have a countdown timer you you would go from " 1m 0s" to " 0m 59s" to keep a consistent width.

[bim9262]

Another option would be to use strftime style formats.

[bim9262]

Perhaps adding an hms option which sets the max_arg to hours and sets the separator to : and hides the units would be enough instead of adding nested formatters or a new parser.

[MaxVerevkin]

The most flexible solution would be to allow an explicit format to be specified if we want to allow "sub-formats" like: .dur(format:'{$d |}{$h:|}{$m:|}'{$s:|}',show_leading_units_if_zero:false)

That's... too much formatting :smile:

That's right. If you wanted to do [hh]:mm:ss you'd want to do something like .dur(max_unit:h,min_unit:s,show_leading_units_if_zero:false,separator:':',show_units:false) otherwise after you get to a day+ you'd end up with a format like dd:hh:mm:ss, which probably isn't a good format.

...

Perhaps adding an hms option which sets the max_arg to hours and sets the separator to : and hides the units would be enough instead of adding nested formatters or a new parser.

.dur(hms:true) seems pretty good.

or using the allow argument mentioned in #1957: .dur(format:'{$d.eng(allow:1..) |}{$h.eng(allow:1..):|}{$m.eng(allow:1..):|}'{$s.eng(allow:1..):|}')

Again, nested format strings feel weird. But maybe... maybe we can add support for some sort of "fields", like $time.h.eng(w:2):$time.s.eng(w:2)? Not sure if it makes sense.

[bim9262]

Perhaps adding an hms option which sets the max_arg to hours and sets the separator to : and hides the units would be enough instead of adding nested formatters or a new parser.

.dur(hms:true) seems pretty good.

or using the allow argument mentioned in #1957: .dur(format:'{$d.eng(allow:1..) |}{$h.eng(allow:1..):|}{$m.eng(allow:1..):|}'{$s.eng(allow:1..):|}')

Again, nested format strings feel weird. But maybe... maybe we can add support for some sort of "fields", like $time.h.eng(w:2):$time.s.eng(w:2)? Not sure if it makes sense.

I'm not sure that the field concept would be generally useful (outside of the duration format) and much more verbose so I'm leaning towards the hms option.

[bim9262]

I think the behaviour of round_up should be modified from Round up to the nearest min_unit to Round up to the nearest minimum displayed unit

[bim9262]

I guess we should decide if hms should be enabled by default for blocks that used that format previously or if it is better to use the unambiguous default format. (2 units of hms are either hh:mm or mm::ss, but unless you know the interval/or it's a small interval and you can see the seconds changing)

[bim9262]

Once we think the settings/defaults make sense I'll add some unit tests as well.

[bim9262]

@MaxVerevkin I know this is a lot to review, do you want me to move the splitting up of the formatters into modules into its own PR?

[MaxVerevkin]

do you want me to move the splitting up of the formatters into modules into its own PR?

That would be nice :)

[bim9262]

@MaxVerevkin, I rebased on master now that the formatter split and allow empty strings PRs have merged. Any changes to the argument names or default values? Should we make the default of the tea timer block use hms by default, since that's the default format now?

[MaxVerevkin]

Should we make the default of the tea timer block use hms by default, since that's the default format now?

Yes, let's try to keep the defaults as they are.

I'll try to take a closer look tomorrow.

[MaxVerevkin]

The types section of the docs needs to be updated.

[MaxVerevkin]

@bim9262 Is this ready to be merged?

[bim9262]

I still wanted to add some tests

[bim9262]

@MaxVerevkin I've added some tests :)

[bim9262]

• Make Value::Duration hold the default value for hms.

Can you explain what you mean? Hold the string value like I did with time

• Do not touch time and instead deprecate it and add remaining/duration/something.

Got it, I can add another value instead changing the type of time.

[MaxVerevkin]

• Make Value::Duration hold the default value for hms.

Can you explain what you mean? Hold the string value like I did with time

Something like Value::duration(dur).with_default_hms(), DurationFormatter::hms would need to be changed from bool to Option<bool>. Not sure if this is a good idea though.

[bim9262]

Something like Value::duration(dur).with_default_hms(),

The format for time in the battery block isn't even the default hms format, it doesn't display seconds, just hh:mm.

DurationFormatter::hms would need to be changed from bool to Option<bool>. Not sure if this is a good idea though.

I had thought about doing something like:

#[derive(Debug, Default)]
pub struct DurationFormatter(Option<DurationFormatterInner>);

#[derive(Debug, Default)]
pub struct DurationFormatterInner {
    hms: bool,
    max_unit_index: usize,
    min_unit_index: usize,
    units: usize,
    round_up: bool,
    unit_has_space: bool,
    pad_with: PadWith,
    show_leading_units_if_zero: bool,
}

but I dislike having different defaults for different blocks.

So I think I'll follow the approach:

Do not touch time and instead deprecate it and add remaining/duration/something.

[bim9262]

@MaxVerevkin anything else needed here or are we good to merge?

[bim9262]

@MaxVerevkin, I made the suggested changes

[bim9262]

LGTM! Again, sorry for late review.

No worries, I've also been late on the reviews too...