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

Implement new formatting mechanism #1112

Merged
merged 58 commits into from
Mar 21, 2021
Merged

Conversation

MaxVerevkin
Copy link
Collaborator

@MaxVerevkin MaxVerevkin commented Mar 3, 2021

An attempt to close #1108. Any thoughts on this PR are welcome.

Overview

The syntax for the formatting string placeholders is (the order doesn't matter, but the name must always be first)

​{<name>[:[0]<min width>][^<max width>][;<min prefix>][*<unit>][#<bar max value>]}

Examples of configs:

  • format = "{signal_strength:2} {graph_down}{speed_down;K}{speed_up;1*Bi/s}"
  • format = "{volume:03}"
  • format = "{volume:5#110} {volume:03}" - demo

Examples of code (blocks' POV):

Temperature:

            let values = map!(
                "average" => Value::from_integer(avg).degrees(),
                "min" => Value::from_integer(min).degrees(),
                "max" => Value::from_integer(max).degrees()
            );

Speed test:

                let values = map!(
                    "ping" => Value::from_float(ping).seconds().icon(self.ping_icon.clone()),
                    "speed_down" => Value::from_float(down).bits_per_second().icon(self.down_icon.clone()),
                    "speed_up" => Value::from_float(up).bits_per_second().icon(self.up_icon.clone()),
                );

Sound:

        let values = map!(
            "volume" => Value::from_integer(volume as i64).percents(),
            "output_name" => Value::from_string(mapped_output_name)
        );

TODO

  • Add more suffixes.
  • Add suffixes for bytes/bits (Ki, Mi, Gi, Ti)
  • Translate every block to a new formatting and remove render_static_str()
  • disk_space: alert_absolute
  • Allow users to override placeholders' icon with {var~icon} will not implement.
  • Add ability to draw bars for every numeric placeholder.
  • Add max_width for strings. Something like "{var.5}" will truncate the text to first 5 characters.
  • Add proper error handling.
  • Write genral documentation.
  • Document changes in every block
  • Add unit tests
  • Extend Value to have colors (via pango) (not useful because i3 renders pango differently compared to sway)
  • (A lot of) Testing.

@MaxVerevkin MaxVerevkin changed the title implement new formatting mechanism [WIP] Implement new formatting mechanism Mar 3, 2021
@ammgws
Copy link
Collaborator

ammgws commented Mar 4, 2021

I don't understand the use case for var~icon. Any examples?

@MaxVerevkin
Copy link
Collaborator Author

I don't understand the use case for var~icon. Any examples?

Net block displays speed as <icon><speed><suffix><unit>. "Var~icon" gives ability to override the icon. But yeah, this is not very useful, and most likely I'll leave it.

@MaxVerevkin
Copy link
Collaborator Author

MaxVerevkin commented Mar 4, 2021

Also, an error will be shown if any icon was not found. I think it's better than unwrap_or_default()ing and makes more sense.

@MaxVerevkin MaxVerevkin force-pushed the new-formatting branch 4 times, most recently from 6bbdb1e to 8aca111 Compare March 4, 2021 19:10
@MaxVerevkin
Copy link
Collaborator Author

Maybe instead of having per_core option in cpu block it is better to have multiple utilization and frequency placeholders? For example: format = "Average: {utilization} {frequency} 1: {utilization1} {frequency1} ...". This gives much more flexibility. Moreover, this is the only way I see how to implement new formatting for cpu block.

@ammgws
Copy link
Collaborator

ammgws commented Mar 5, 2021

Maybe instead of having per_core option in cpu block it is better to have multiple utilization and frequency placeholders? For example: format = "Average: {utilization} {frequency} 1: {utilization1} {frequency1} ...". This gives much more flexibility. Moreover, this is the only way I see how to implement new formatting for cpu block.

Sounds OK to me.

@MaxVerevkin MaxVerevkin force-pushed the new-formatting branch 5 times, most recently from 8ce54ac to 9ec0323 Compare March 6, 2021 21:42
@MaxVerevkin
Copy link
Collaborator Author

Add ability to draw bars for every numeric placeholder.

Would something like this be OK: "{var:5~110}"? This will create a bar 5 characters long with minimum value of zero and maximum of 110. And I'm not sure if the minimum value should be configurable.

@ammgws
Copy link
Collaborator

ammgws commented Mar 8, 2021

So it would pad the string? I don't get why the minimum is 0 in that example.

Is this what you mean (replaced spaces with □ to make it easier to tell):

format = "{var:5~7}"
(min length of 5, max of 7)

var output
"" "□□□□□" <-- pads to the left?
"x" "□□□□x" <-- pads to the left?
"x□" "□□□x□" <-- pads to the left?
"12345678" "1234567" <-- trims from the right?

@ammgws
Copy link
Collaborator

ammgws commented Mar 8, 2021

Oh sorry I thought you were talking about strings because of your comment on the other PR. Didn't realise you were talking about the barchart.

fn format_vec_to_bar_graph(content: &[f64], min: Option<f64>, max: Option<f64>) -> String

I assume your min and max are referring to the ones used by our current function?

@MaxVerevkin
Copy link
Collaborator Author

MaxVerevkin commented Mar 8, 2021

@ammgws No, I meant this function

i3status-rust/src/util.rs

Lines 348 to 355 in 094cfdc

}
}
pub fn format_percent_bar(percent: f32) -> String {
let percent = percent.min(100.0);
let percent = percent.max(0.0);
(0..10)

As you can see, 0 and 100 are hard-coded.

@MaxVerevkin
Copy link
Collaborator Author

MaxVerevkin commented Mar 8, 2021

This feature would remove redundant placeholders like

let values = map!("{percentage}" => format!("{:.2}%", percentage),
"{bar}" => format_percent_bar(percentage),

and give ability to draw bar for, lets say, sound block, as well as every other block.

@ammgws
Copy link
Collaborator

ammgws commented Mar 8, 2021

Oh wow I totally forgot that even existed. It's not even documented for the sound block.

Anyway, right now I can't think of any cases where configuring the min would be useful, so perhaps it may be OK to hardcode it at 0. One nit: the proposed syntax kinda looks like a number range due to the "~", so perhaps another char would be better. And maybe explicitly declare that it's a barchart to make it more obvious as well. "{var:bar:5:110}" or something.

@MaxVerevkin
Copy link
Collaborator Author

It's not even documented for the sound block.

Sound block doesn't have this feature.

One nit: the proposed syntax kinda looks like a number range due to the "~", so perhaps another char would be better. And maybe explicitly declare that it's a barchart to make it more obvious as well. "{var:bar:5:110}" or something.

The parsing I currently have doesn't work well (at all) with repeated "keys" (:, ;, *), so maybe "{var:5#100}"?

@ammgws
Copy link
Collaborator

ammgws commented Mar 8, 2021

Sound block doesn't have this feature.

src/blocks/sound.rs
40:use crate::util::{format_percent_bar, FormatTemplate};
856:                    format_percent_bar(volume as f32)
870:                format_percent_bar(volume as f32)

image

maybe "{var:5#100}"

Sure why not. We can overview the overall syntax for everything once it gets nearer to completion, and get some more feedback from other interested parties too.

@MaxVerevkin
Copy link
Collaborator Author

Ohhh, well, that's weird 😆

@ammgws
Copy link
Collaborator

ammgws commented Mar 8, 2021

Memory block also has a bunch of undocumented format key specifiers that output a percentage bar 😆

src/blocks/memory.rs
277:            "{MFpb}" => format_percent_bar(mem_free.percent(mem_total)),
282:            "{MUpb}" => format_percent_bar(mem_total_used.percent(mem_total)),
287:            "{Mupb}" => format_percent_bar(mem_used.percent(mem_total)),
292:            "{MApb}" => format_percent_bar(mem_avail.percent(mem_total)),
299:            "{SFpb}" => format_percent_bar(swap_free.percent(swap_total)),
304:            "{SUpb}" => format_percent_bar(swap_used.percent(swap_total)),
309:            "{Bpb}" => format_percent_bar(buffers.percent(mem_total)),
314:            "{Cpb}" => format_percent_bar(cached.percent(mem_total)));

@MaxVerevkin
Copy link
Collaborator Author

What's going on here? Looks like it's applying the engineering notation to the percentage (usage is around 0.03%)

An exception can should be made for percents.

@GladOSkar
Copy link
Contributor

Ah yes the classic millipercent, also known as the centipermille :D

@MaxVerevkin
Copy link
Collaborator Author

Also i always thought for network speeds bits were the default unit instead of bytes?

No, bits are default only for speedtest.

@GladOSkar
Copy link
Contributor

Also i always thought for network speeds bits were the default unit instead of bytes?

No, bits are default only for speedtest.

I meant in general use, not this software. But it doesn't really matter as long as both are possible

@ammgws
Copy link
Collaborator

ammgws commented Mar 20, 2021

is it even necessary?

It may be, since before it would have been displayed as "1.92Kb" and now it's "1.92Kb/s", and I imagine there are people who want to keep it short by removing the "/s" part. (Actually I am one of those people but I don't have it set to bits 😃 )

@ammgws
Copy link
Collaborator

ammgws commented Mar 20, 2021

Also i always thought for network speeds bits were the default unit instead of bytes?

Bits for network speed, but when actually downloading files to disk (browser, torrents, etc), if my memory is correct then it's almost always been bytes.

@GladOSkar
Copy link
Contributor

Honestly same, but i don't really know how we could completely decouple the actual unit from the displayed unit unless we made the entire unit after the prefix customizable, which would then make it hard to semantically get the desired units (b vs B) from it (but it would probably be possible)

@MaxVerevkin
Copy link
Collaborator Author

MaxVerevkin commented Mar 20, 2021

A little TODO:

  • Do not apply engineering notation for percents
  • _b/s should mean "convert to bits per second but don't display it"

@GladOSkar
Copy link
Contributor

GladOSkar commented Mar 20, 2021

Is there a way to do what @ammgws wanted? A way do discard the /s? I'm imagining something like:

"{down*B/s}" -> "1KB/s"
"{down*b/s}" -> "8Kb/s"
"{down*Bps}" -> "1KBps"
"{down*bps}" -> "8Kbps"
"{down*B}" -> "1KB"
"{down*b}" -> "8Kb"
"{down*_B}" -> "1K"
"{down*_b}" -> "8K"

i.e. matching the used unit by finding the substring b or B instead if matching the entire unit and then just appending the rest.

Or is that too difficult?

@MaxVerevkin
Copy link
Collaborator Author

"[units_overrides]" 😆?

@GladOSkar
Copy link
Contributor

GladOSkar commented Mar 20, 2021

"[units_overrides]" laughing?

Oof 😆

@ammgws
Copy link
Collaborator

ammgws commented Mar 20, 2021

How about adding bits as a unit and then just allowing conversion between BitsPerSecond/BytesPerSecond and Bits/Bytes?

@MaxVerevkin
Copy link
Collaborator Author

MaxVerevkin commented Mar 20, 2021

How about adding bits as a unit and then just allowing conversion between BitsPerSecond/BytesPerSecond and Bits/Bytes?

It still would not give enough flexibility to, for example, switch between b/s and Bi/s, so I don't think it's a good idea.

If just hiding the unit with _unit doesn't work good enough either, then I suppose, [units_overrides] doesn't look that bad.

@ammgws
Copy link
Collaborator

ammgws commented Mar 20, 2021

Since we only have bits/bytes per second and not per minute/hour/etc then just remove them and have Bits and Bytes only. The "/s" part can be added in the format string. Especially since there are other ways people might want to write it, such as "Kbps" instead "Kb/s"

@MaxVerevkin
Copy link
Collaborator Author

Since we only have bits/bytes per second and not per minute/hour/etc then just remove them and have Bits and Bytes only. The "/s" part can be added in the format string. Especially since there are other ways people might want to write it, such as "Kbps" instead "Kb/s"

OK, at least it would look the same way it was before.

@MaxVerevkin
Copy link
Collaborator Author

By the way, would it be possible to use rustdoc to generate documentation? This would make it much easier to keep it up to date.

@ammgws
Copy link
Collaborator

ammgws commented Mar 21, 2021

If it makes things easier, I don't see why not. Something to look at in the future.

@ammgws
Copy link
Collaborator

ammgws commented Mar 21, 2021

OK I've done as much testing as I can.

Excellent work on this!

@ammgws ammgws merged commit ee62151 into greshake:master Mar 21, 2021
@MaxVerevkin MaxVerevkin deleted the new-formatting branch March 21, 2021 08:44
@GladOSkar
Copy link
Contributor

Yeah honestly great work :) Thanks!

@ammgws ammgws mentioned this pull request Mar 21, 2021
@ghost ghost mentioned this pull request Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discussion: add formatting parameters for format key specifiers
5 participants