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

Change default formatting? #1304

Closed
ammgws opened this issue Sep 8, 2021 · 46 comments
Closed

Change default formatting? #1304

ammgws opened this issue Sep 8, 2021 · 46 comments

Comments

@ammgws
Copy link
Collaborator

ammgws commented Sep 8, 2021

Pretty sure this is also #1231 and was added with the new formatting system.

Have a look there to see the right way to format the value.

@MaxVerevkin seems to me like the new behavior might not be a very good default, we might want to have a look at that. (See also #1272)

Originally posted by @GladOSkar in #1303 (comment)

I have a few ideas:

* We can just change the default "min-width" to 1, however those who relied on defaults might get confused.

* We can also add "refer to (...) for more info" in the blocks' docs. And perhaps formatting docs can be rewritten in a more clear way?

* (not an actual option) Maybe not provide the default value for "min-width" at all?

_Originally posted by @MaxVerevkin in #1303

@GladOSkar
Copy link
Contributor

GladOSkar commented Sep 8, 2021

Option 4 i see and my favorite solution would be to do what's asked in (the second part of) #1272 and allow a number format as discussed in #240 and implemented in #704 that can have a fixed width and still not show any gaps, even if it's sometimes a bit suboptimal in the number of significant digits shown.

That might be difficult to enable with the current placeholder syntax, but is probably possible. Alternatively we could also just change the default. I'll propose something when i have time

@ram02z
Copy link
Contributor

ram02z commented Sep 17, 2021

Not sure if this entirely relevant, however, blocks like the brightness block don't have a format key, which makes sense, but that means it can't make use of the formatting available in other blocks. Not sure if it worth adding the format key just so I can change the min_width of the brightness block to 3 though.

@MaxVerevkin
Copy link
Collaborator

Not sure if it worth adding the format key just so I can change the min_width of the brightness block to 3 though.

I think it does.

@GladOSkar
Copy link
Contributor

Yeah now that we have the universal logic that's not an issue. Plus it will provide additional options like showing the brightness as a bar. But i wouldn't consider that relevant to this issue, feel free to open another one :)

@GladOSkar
Copy link
Contributor

Option 4 i see and my favorite solution would be to do what's asked in (the second part of) #1272 and allow a number format as discussed in #240 and implemented in #704 that can have a fixed width and still not show any gaps, even if it's sometimes a bit suboptimal in the number of significant digits shown.

That might be difficult to enable with the current placeholder syntax, but is probably possible. Alternatively we could also just change the default. I'll propose something when i have time

I thought about this a bit again and realized it's only possible for value placeholders with a min_width of at least 4 characters, since without padding a value like 42000 can't really be displayed with 3 value characters in any nice way: 42.K / .04M would be the only (ugly) solutions i can come up with.

Starting at 4 characters we can easily do something like what was discussed in the other issue:

4.20M
0.42M
42.0K
4.20K
0.42K

Now i'm not sure if it's worth the effort/complexity just for avoiding some ugly padding, but we probably could add this as a special case for min_width >= 4 in the default behavior of the value formatter.

What do you think @MaxVerevkin?

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Sep 28, 2021

I still don't like when 420K is displayed as 0.42M but I can understand that it might not be the case for others.

but we probably could add this as a special case for min_width >= 4 in the default behavior of the value formatter

No, I think setting min_width to, say 5, sould do what it does now. However, I think we can either add some kind of config option or just set your proposed solution as the default and disable it if min_width is set (edit: no, I forgot that your solution also uses min_width).

And actually, 42.K doesn't look that bad for me.

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 19, 2021

Okay, perhaps it's time to reconsider the formatting "subsystem". When choosing between verbosity and shortness we've chosen shortness. Was this a good choice?

Formatting configuration is already sort of messy, but it will be even worse it we'll add more options (like how to choose between "always N digits" and engineering notation, no one have proposed anything yet).

Looking back at the work done by @remi-dupre, #750 (comment) looks quite interesting. I can imagine doing something like this:

"{<var>@num-const-digits(<params>)}" # Use "always N digits" formatter
"{<var>@num-eng(<params>)}" # Use engineering notation
"{<var>@num-bar(<params>)}" # Draw a bar (not related to this issue, but looks cool)
"{<var>@str(<params>)}" # String formatter
"{<var>@str-rotating(<params>)}" # Rotating string (would be really cool to have such functionality)

Yes, those are breaking changes, and most likely it will not be a solution in the near future, but still.

@ammgws @GladOSkar @remi-dupre your thoughts?

@GladOSkar
Copy link
Contributor

GladOSkar commented Oct 20, 2021

Yeah i had originally vouched for shortness as well and still prefer it. And although i kinda like your suggestion, I'd prefer not to break configs again. I wonder if there's a way to do this within the confines of currently valid syntax? Something like another "modifier" character to specify the formatter?:

{<name>[!<fix|eng|bar|str|rot>]...

With the default being either fix for fixed width or eng for engineering notation, with all the old options still working.

Granted, we'd be keeping a lot of dead syntax around, but maybe it's not that bad? Unsure about this, you probably know better given you wrote the formatter

@MaxVerevkin
Copy link
Collaborator

In addition, I would also like to bring up two more issues:

  • There is no way to handle missing keys. Currently, in cases when keys may by missing we provide many formatting options, like format, format_missing and so on.
  • Currently, some placeholders may have different types. The bad thing is, it's not possible to set different options for different types. And what's more important, there is no way to change the surrounding spacing for different types. This one isn't solved yet and it's impossible to solve without breaking changes.

Lets imagine that current formatting system is not yet implemented. Now, I would propose something like this:

Format <var> with engineering notation and surround it with spaces. This structure will resolve to nothing if <var> is missing or <var> is not a number.

{ <var>.eng(<params>) |}

Format <var> with fixed digits formatter and prepend num: before it. If <var> a string instead of number, it will be prepended with text: . If <var> is missing, N/A will be printed.

{num: <var>.fix(<params>)|text: <var>.str(<params>)|'N/A'}

While it's more verbose, It's way more flexible. This can be partially (without the typing part) implemented without breaking compatibility (well, it will break stuff if | is used in the config).

@GladOSkar
Copy link
Contributor

GladOSkar commented Oct 23, 2021

I like it. You've convinced me. Seems pretty well thought through.

One detail question: Do you intend to delimit the variable name? Or is it distinguished from the literal text in another way? In other words, would you literally write {volume: <volume>.fix(3)|} or {volume: volume.fix(3)|}? For the latter maybe the dot is what makes it a variable, which would require something like \. to actually print a dot in the literal text.

I prefer delimiting either the variable or the literal text over using the dot

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 23, 2021

Do you intend to delimit the variable name?

I don't have a strong opinion on this one, but using <> will make parsing easier.

I was also thinking about using single quotes (the parsing is still simple) for literal text but it seems to be messy.

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 24, 2021

Also two more details:


How exactly should arguments be represented? For example, in {volume: <volume>.fix(3)|}, 3 is width, but how to pas minimal prefix and unit? One of the options is python-like parameters:

{speed: <speed>.fix(3,unit=_b,prefix=+K)|}

The second option is to use something similar to what we have now (note: + is not currently implemented, it forces the prefix instead of setting the minimal prefix):

{speed: <speed>.fix(3*_b;+K)|}

Again, tradeoffs between shortness and verbosity must be taken. But the first one is much more readable, isn't it?


Should integers and floats be distinguished? Currently they are, sort of.

@GladOSkar
Copy link
Contributor

How about just positional arguments? Like:

{speed: <speed>.fix(3,_b,+K)|}

that way we loose the ability to eg. set the prefix without setting the unit, but if the order is thought through enough and the defaults are documented, i think it'd be fine while not being overly verbose.

Regarding floats vs. integers: currently, the differentiation is made to make sure units like bits never get the lower SI prefixes (a microbit probably only makes sense probabilistically). But if the formatting syntax is well designed i don't think the differentiation is necessary. Unless you see something else.

@MaxVerevkin
Copy link
Collaborator

How about just positional arguments?

That might be a good idea. Looks clean and simple to parse.

the differentiation is made to make sure units like bits never get the lower SI prefixes

Not quite - integers are not formatted at all (they're only padded), and currently, bytes are passed as floats.

So, the issue with micropercents / microbytes and so on is fixed by just checking the unit.

@GladOSkar
Copy link
Contributor

Ah okay - In that case i don't see a reason to keep the differentiation

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 26, 2021

I don't have a strong opinion on this one, but using <> will make parsing easier.

Now I think it might make sence to avoid < and > because if user wants use them inside placeholders, they would need to prepend them with \\\ in order to escape both formatter and pango. Or it is okay?

@GladOSkar
Copy link
Contributor

GladOSkar commented Oct 26, 2021

Also true. How about a bash/php style $var.fix()? I'd assume a literal dollar sign would only be used very rarely, and is easier to escape

@MaxVerevkin
Copy link
Collaborator

And about the positional arguments, if, for example, string formatter's arguments were (max_width,min_width), and suppose I don't want to limit maximum width but want to set min_width, what should be passed as the first argument?

@GladOSkar
Copy link
Contributor

? Jk of course. Just a really big number? Maybe it can also be left empty or have a null-like value. Don't think it's that important.

But we should really try to order the arguments in such a way that it reduces the number of occasions where one has to consider something like this. str(max_width,min_width) makes sense because i can not really imagine a scenario where one would want to set the min width but not the max. Meanwhile it should probably be .fix(min_width,unit,prefix) because the unit is probably configured more often than the prefix and can be easily set to a default if not.

@MaxVerevkin
Copy link
Collaborator

? Jk of course.

Actually, inf doesn't seem bad.

str(max_width,min_width) makes sense because i can not really imagine a scenario where one would want to set the min width but not the max.

Yeah, but... I don't know, in str's case .str(min,max) seems more natural and logical.

@GladOSkar
Copy link
Contributor

GladOSkar commented Oct 28, 2021

I guess you're right. It could also be .str(max) with 1 and .str(min,max) with 2 arguments, but that sounds like a nightmare to document and also isn't very intuitive. In that case .str(min,max) is probably okay and min just has to be set to 0.

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 28, 2021

Given that most of the placeholders are guaranteed to be presented, perhaps {} can be made optional?

Also, maybe multiple placeholders can be combined together? Like format="$signal.eng() $ssid.str()|N/A".

And to take this even further perhaps FormatTemplate can be defined recursively and everything that is inside of {} is treated as FormatTemplate? That would mean that this is a valid formatting string:

[[block]]
block = "net"
format = "{$signal.eng(2) $ssid.str() $frequency.eng()|Wired connection} via $device.str()|N/A"

It tries to print signal, ssid and frequency and if any of those fails "Wired connection" is printed. This is followed by the device name. If device name is missing, then just "N/A" is printed.

So... I've implemented it in https://github.com/MaxVerevkin/swaystatus/tree/new-formatting, mostly because I wanted to write a recursive descent parser (I don't think more than two levels of {} is practically useful) (only time, net and speedtest blocks are working now, fix formatter is not implemented yet).

@GladOSkar
Copy link
Contributor

Yeah i like it. The {} seemed a bit weird in your first example anyways, but it totally makes sense sometimes to specify the whitespace handling correctly when placeholders can disappear. So having it optional seems like a logical conclusion. And having it be recursive really allows some neat custom behavior. Awesome!

At this point the new format strings are so different we could probably determine whether a format specifier is old or new at parse time and use the old formatter for old configs while spewing out a deprecation warning. WDYT?

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Oct 29, 2021

At this point the new format strings are so different we could probably determine whether a format specifier is old or new at parse time and use the old formatter for old configs while spewing out a deprecation warning. WDYT?

Yeah, I suppose a simple criteria to determine old formatting is s.contains('{') && !s.contains('$'), and the simplest recovery that can be performed is changing text {var...} to text {$var.str(...)|$var.eng(...)}.

However I'm not sure if it even makes sense since the whole point of the new formatting system it to allow missing placeholders.

@MaxVerevkin
Copy link
Collaborator

I thought about this a bit again and realized it's only possible for value placeholders with a min_width of at least 4 characters

So, fix formatter should throw an error if the width is less than 4?

And by the way, if width is greater of equal to 5 then eng has always the same number of digits, so fix only makes sense for width = 4?

eng(5):
0.001 => 1.000 m
0.01 => 10.00 m
0.1 => 100.0 m
0 => 0.000
10 => 10.00
100 => 100.0
1_000 => 1.000 K
10_000 => 10.00 K

@MaxVerevkin
Copy link
Collaborator

And at this point I don't understand what fix formatter even supposed to do. Is it fixed number of characters or digits? (#1272 refers to just the number of digits, while @GladOSkar refers to... both?)

@GladOSkar
Copy link
Contributor

Well, width < 4 is possible to display, it might just not look nice. As you said, 42.K doesn't look that bad.

It was always supposed to be fixed # of characters, since that's what's supposed to stay constant without using any padding for a visually pleasing, but non-shifting bar.

But you're right, that might only make sense for width<=4, but i think width=4 is a very commonly used amount of digits (3). That's why i was in favor of just special-casing it at first.

@MaxVerevkin
Copy link
Collaborator

As you said, 42.K doesn't look that bad.

If we were to allow a trailing dot (with something like .eng(.3)), eng formatter would also have fixed width without padding for width >= 3:

eng(3)
0.001 => 1.0m
0.01 => 10.m
0.1 => 100m
1 => 1.0
10 => 10.
100 => 100
1_000 => 1.0K
10_000  => 10.K

but we probably could add this as a special case for min_width >= 4 in the default behavior of the value formatter.

That's why i was in favor of just special-casing it at first.

Still, displaying (width=5) 0.104M instead of 104.0K is weird, or is it just me 😆?

By the way, anyone wants to implement fix formatter?

@GladOSkar
Copy link
Contributor

Still, displaying (width=5) 0.104M instead of 104.0K is weird, or is it just me laughing?

Yeah it is. But i don't see another option for some of the cases in the fix formatter.

By the way, anyone wants to implement fix formatter?

Want, yeah, have the time? No. The version i wrote that was used in #704 isn't very nice though, but can probably be adapted though. I'll have a look if/when i have time

@MaxVerevkin
Copy link
Collaborator

"{<var>@str-rotating(<params>)}" # Rotating string (would be really cool to have such functionality)

So I've got a working prototype 🎉 :

rec.mp4
[[block]]
block = "sway_kbd"
format = "Layout \"$layout.rot-str(10,0.5)\""

@MaxVerevkin
Copy link
Collaborator

So, are three any comments on the proposed syntax?

@GladOSkar
Copy link
Contributor

I like it, but i'm still not sure if it's worth breaking configs again. If we manage a graceful transition this time i'm in favor.

@ammgws
Copy link
Collaborator Author

ammgws commented Nov 2, 2021

Even with a graceful transition you'd still end up having to change your config in the future, right?

We can mention the future changes in the release notes for the next bug fix release, but apart from that there's not much we can do...

@GladOSkar
Copy link
Contributor

Yeah absolutely. I suppose if we're gonna break it again it might as well be as close to the last breakage as possible

@ammgws
Copy link
Collaborator Author

ammgws commented Nov 2, 2021

@MaxVerevkin Are there any breaking changes necessary for merging swaystatus' async here? If so then it could be done at the same timing.

@MaxVerevkin
Copy link
Collaborator

@ammgws I'm not sure what exactly do you mean, but I expect this formatting to be merged (if even merged) with swaystatus, so not so soon.

@MaxVerevkin
Copy link
Collaborator

So, are three any comments on the proposed syntax?

Like, maybe we should provide default formatters? ($layout instead of $layout.str()) Or any other recommendations.

@MaxVerevkin
Copy link
Collaborator

Not sure if graceful transition is even possible:

However I'm not sure if it even makes sense since the whole point of the new formatting system it to allow missing placeholders.

@GladOSkar
Copy link
Contributor

Like, maybe we should provide default formatters? ($layout instead of $layout.str()) Or any other recommendations.

Oh yeah i had assumed there was a default. If a user doesn't care about formatting, they shouldn't have to specify it.

@MaxVerevkin
Copy link
Collaborator

Oh yeah i had assumed there was a default

So... eng or fix? 😆

@GladOSkar
Copy link
Contributor

eng for numbers, str for strings. fix seems to be a bit of a niche use case at this point ^^

@MaxVerevkin
Copy link
Collaborator

I just realized that the new formatting system can make use of something similar to if statements!

This can be used in pacman/dnf/apt block:

$noupdates{...}|$single{...}|{...}

where $noupdates is an empty string which is presented only if there are no updates. Same for $single.

It is even possible to create && and || structures!

$a$b{then}|{else} // a&&b
{$a|$b}{then}|{else} // a||b

@GladOSkar
Copy link
Contributor

GladOSkar commented Nov 8, 2021

Oh no. If you manage to get some sort of loop in there we're probably Turing complete -.- :D

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Nov 9, 2021

I've tried to formalize what is available so far in https://maxverevkin.github.io/swaystatus/swaystatus/formatting/index.html

@MaxVerevkin
Copy link
Collaborator

So... Any comments on this topic? I've been using my proposed formatting all this time and it seems fine to me.

@ammgws
Copy link
Collaborator Author

ammgws commented Jun 19, 2022

Closing as this is now the default on master.

@ammgws ammgws closed this as completed Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants