[WIP] feat: property template string #309
Conversation
|
fwiw, I have replaced 1:1 every single property occurence in my theme (close to the default one) and ran into no issues Click to see diffdiff --git a/dot_config/rmpc/themes/def.ron b/dot_config/rmpc/themes/def.ron
index a36983f..355c22e 100644
--- a/dot_config/rmpc/themes/def.ron
+++ b/dot_config/rmpc/themes/def.ron
@@ -28,31 +28,16 @@
panes: [
(
size: "23",
- pane: Pane(Property(
- content: [
- (kind: Text("["), style: (fg: "yellow", modifiers: "Bold")),
- (kind: Property(Status(StateV2())), style: (fg: "yellow", modifiers: "Bold")),
- (kind: Text("]"), style: (fg: "yellow", modifiers: "Bold")),
- ], align: Left,
- )),
+ pane: Pane(Property(format: "$'['{fg: yellow, mods: bold}$state(){fg: yellow, mods: bold}$']'{fg: yellow, mods: bold}", align: Left)),
),
(
size: "100%",
borders: "LEFT | RIGHT",
- pane: Pane(Property(
- content: [
- (kind: Property(Song(Filename)), style: (modifiers: "Bold"),
- default: (kind: Text("No Song"), style: (modifiers: "Bold")))
- ], align: Center, scroll_speed: 1
- )),
+ pane: Pane(Property(format: "$s:title{mods: bold}|$'No Song'{mods: bold}", align: Center, scroll_speed: 1)),
),
(
size: "23",
- pane: Pane(Property(
- content: [
- (kind: Property(Widget(Volume)), style: (fg: "blue", modifiers: "Bold"))
- ], align: Right
- )),
+ pane: Pane(Property( format: "$w:volume{fg: blue, mods: bold}", align: Right)),
),
],
),
@@ -65,63 +50,22 @@
panes: [
(
size: "23",
- pane: Pane(Property(
- content: [
- (kind: Property(Status(Elapsed))),
- (kind: Text(" / ")),
- (kind: Property(Status(Duration))),
- (kind: Group([
- (kind: Text(" (")),
- (kind: Property(Status(Bitrate))),
- (kind: Text(" kbps)")),
- ])),
- ], align: Left,
- )),
+ pane: Pane(Property(format: "$elapsed$' / '$duration$[$' ('$bitrate$' kpbs)']", align: Left)),
),
(
size: "100%",
borders: "LEFT | RIGHT",
- pane: Pane(Property(
- content: [
- (kind: Property(Song(Artist)), style: (fg: "yellow", modifiers: "Bold"),
- default: (kind: Text("Unknown"), style: (fg: "yellow", modifiers: "Bold"))),
- (kind: Text(" - ")),
- (kind: Property(Song(Album)), default: (kind: Text("Unknown Album")))
- ], align: Center, scroll_speed: 2
- )),
+ pane: Pane(Property(format: "$s:artist{fg: yellow, mods: bold}|$'Unknown Artist'{fg: yellow, mods: bold}$' - '$s:album{fg: yellow, mods: bold}|$'Unknown Album'", align: Center, scroll_speed: 2)),
),
(
size: "23",
- pane: Pane(Property(content: [
- (kind: Property(Status(RepeatV2(
- on_label: " ",
- off_label: " ",
- on_style: (fg: "yellow", modifiers: "Bold"),
- off_style: (fg: "blue", modifiers: "Dim"),
- )))),
- (kind: Property(Status(RandomV2(
- on_label: " ",
- off_label: " ",
- on_style: (fg: "yellow", modifiers: "Bold"),
- off_style: (fg: "blue", modifiers: "Dim"),
- )))),
- (kind: Property(Status(SingleV2(
- on_label: " ",
- off_label: " ",
- oneshot_label: " ",
- on_style: (fg: "yellow", modifiers: "Bold"),
- off_style: (fg: "blue", modifiers: "Dim"),
- oneshot_style: (fg: "red", modifiers: "Bold"),
- )))),
- (kind: Property(Status(ConsumeV2(
- on_label: " ",
- off_label: " ",
- oneshot_label: "",
- on_style: (fg: "yellow", modifiers: "Bold"),
- off_style: (fg: "blue", modifiers: "Dim"),
- oneshot_style: (fg: "red", modifiers: "Dim"),
- )))),
- ], align: Right)),
+ pane: Pane(Property(
+ format: "
+ $repeat(onLabel: ' ', offLabel: ' ', onStyle: {fg: yellow, mods: bold}, offStyle: {fg: yellow, mods: dim})
+ $random(onLabel: ' ', offLabel: ' ', onStyle: {fg: yellow, mods: bold}, offStyle: {fg: yellow, mods: dim})
+ $single(onLabel: ' ', offLabel: ' ', oneshotLabel: ' ', onStyle: {fg: yellow, mods: bold}, offStyle: {fg: yellow, mods: dim}, oneshotStyle: {fg: red, mods: bold})
+ $consume(onLabel: ' ', offLabel: ' ', oneshotLabel: ' ', onStyle: {fg: yellow, mods: bold}, offStyle: {fg: yellow, mods: dim}, oneshotStyle: {fg: red, mods: dim})
+ ", align: Right)),
),
],
),
@@ -147,9 +91,9 @@
direction: Horizontal,
panes: [
(
- pane: Pane(Property(content: [(kind: Property(Status(StateV2(playing_label: " ", paused_label: " ", stopped_label: " ",
- playing_style: (fg: "blue"), paused_style: (fg: "green"), stopped_style: (fg: "red")
- ))))], align: Left)),
+ pane: Pane(Property(
+ format: "$state(playingLabel: ' ', pausedLabel: ' ', stoppedLabel: ' ', playingStyle: {fg: blue}, pausedStyle: {fg: green}, stoppedStyle: {fg: red})",
+ align: Left)),
size: "3",
),
(
@@ -158,13 +102,7 @@
),
(
size: "13",
- pane: Pane(Property(
- content: [
- (kind: Property(Status(Elapsed))),
- (kind: Text(" / ")),
- (kind: Property(Status(Duration))),
- ], align: Right,
- )),
+ pane: Pane(Property(format: "$elapsed$' / '$duration", align: Right)),
),
]
),
@@ -190,22 +128,7 @@
thumb_style: (fg: "blue"),
),
browser_column_widths: [20, 38, 42],
- browser_song_format: [
- (
- kind: Group([
- (kind: Property(Track)),
- (kind: Text(" ")),
- ])
- ),
- (
- kind: Group([
- (kind: Property(Artist)),
- (kind: Text(" - ")),
- (kind: Property(Title)),
- ]),
- default: (kind: Property(Filename))
- ),
- ],
+ browser_song_format_v2: "$[$s:track$' ']$[$s:artist$' - '$s:title]|$s:filename$'.'$s:fileextension",
tab_bar: (
active_style: (fg: "black", bg: "blue", modifiers: "Bold"),
inactive_style: (),
@@ -215,25 +138,10 @@
borders_style: (fg: "blue", modifiers: "Bold"),
highlight_border_style: (fg: "red"),
song_table_format: [
- (
- prop: (kind: Property(Other("albumartist")), default: (kind: Property(Artist), default: (kind: Text("Unknown")))),
- width: "20%",
- label: "Artist"
- ),
- (
- prop: (kind: Property(Title), default: (kind: Text("Unknown"))),
- width: "35%",
- ),
- (
- prop: (kind: Property(Album), default: (kind: Text("Unknown Album"))),
- width: "45%",
- ),
- (
- prop: (kind: Property(Duration),default: (kind: Text("-"))),
- width: "5",
- alignment: Right,
- label: "Len"
- ),
+ (format: "$s:albumartist|$s:artist|$'unknown'", width: "20%", label: "Artist"),
+ (format: "$s:title|$'Unknown'", width: "35%"),
+ (format: "$s:album|$'Unknown Album'", width: "45%"),
+ (format: "$s:duration|$'-'", width: "5", alignment: Right, label: "Len"),
],
header: (rows: []),
) |
|
I haven't been able to offer anything to this because I've been dealing with life etc. for a while. Got sick and had some personal issues and stuff so I don't know how long until I'll have anything to offer. I will say that the syntax is a bit strange but I'll get to that when I'm not feeling terrible 24/7. |
|
This seems really nice. The only feedback I can think of right now is maybe taking some inspiration from the waybar config. |
|
@mierak I've been reviewing the PR and the discussions, and this looks really great. That said, I want to challenge a few aspects to explore potential pitfalls, if you don’t mind: 1. Robust Error Handling and User Feedback With this new "mini-language," I think strong error reporting becomes crucial to maintaining a good user experience. While technical users might be able to debug most issues, less technical users, especially once config files become more complex, may struggle to understand vague or cryptic errors. This might be less of an issue now since the majority of users are on Linux and probably more technical, but I think you should still consider when users who are not much technical might use rmpc when the project become very popular. 2. Feature Scope and Complexity Features like I believe adding too many features too early might lead to a higher maintenance cost in the long run. 3. Documentation In my eyes, this is the most critical piece moving forward. Since you’re introducing a whole new syntax, a clear and complete documentation is very much essential for adoption.
This will make it much easier for users to adopt and use the language and even contribute safely. |
src/config/tabs.rs
Outdated
| (Some(format), Some(_)) | (Some(format), None) => parser::parser() | ||
| .parse(&format) | ||
| .into_result() | ||
| .map_err(|e| { | ||
| anyhow::anyhow!("Failed to parse property format: {:?}", e) | ||
| })? | ||
| .into_iter() | ||
| .map(|prop| -> Result<_> { prop.try_into() }) | ||
| .try_collect()?, |
There was a problem hiding this comment.
Yup, I am not even 100% on whether it will stay like this though its very likely, but this will for sure be documented.
src/config/theme/parser.rs
Outdated
| |digits, e, emitter| { | ||
| char::from_u32( | ||
| u32::from_str_radix(digits, 16) | ||
| .expect("Only valid digits should have been parsed in text::digits"), | ||
| ) |
There was a problem hiding this comment.
The thing is, this invariant should be ensured by this text::digits(16) a few lines above.
This panic should never really happen and if it does it should get fixed asap. But I suppose a more graceful error is fine too here.
First of all, thank you for taking your time to look at this!
Yup! Definitely a good point, chumsky has support for error reporting so I need to look into that.
Well these are more or less already coming in, the
I have been meaning to rework docs on all this for quite a while, the properties are a mess in general with them being documented in the |
src/config/tabs.rs
Outdated
| (Some(format), Some(_)) | (Some(format), None) => parser::parser() | ||
| .parse(&format) | ||
| .into_result() | ||
| .map_err(|e| { | ||
| anyhow::anyhow!("Failed to parse property format: {:?}", e) | ||
| })? | ||
| .into_iter() | ||
| .map(|prop| -> Result<_> { prop.try_into() }) | ||
| .try_collect()?, |
There was a problem hiding this comment.
This should be documented, users should know that format will take precedence, or it might spark confusion
|
Hi, I noticed how much activity this is getting. Sorry for not meaningfully pitching in since this was created, I'll be coming home from vacation in a couple days and by then I'll come around and actually fully look into this and offer my thoughts. So far this looks very promising. I assume it's too late for breaking syntax changes but I will say that I think improvements could still be made for readability. Not to imply that all aspects of the language should be made extremely streamlined immediately, I just feel that at the moment there're a lot of tiny UX things here that still feel somewhat ambiguous/confusing. For one, all the dollar symbols are hard to follow, foobar's templating engine used various symbols to differentiate between different functionalities and it seems to be as terse as you can get with this sort of thing and I wonder if some notes could still be taken from that. I've brought that up multiple times before, though, so I won't repeat myself too much here. At any rate, good luck with the implementation and thank you for going through with this. I'll have a look at the code when I get home. |
Experimenting with chumsky's companion crate ariadne. We can get pretty nice error reports on the formats (dont mind the missing component message). It would be nice if I could get list of supported properties out of it, but this is already pretty good imho. Though it will have some issues on very small terminal sizes, but I think its still worth having. |
|
I introduced some shorter variants for the properties, for example "artist" and "ar" are equivalent. I have also decided to namespace the properties to avoid current and future conflicts. Still undecided on the namespacing, opinions anyone? For string, $ is no longer needed, simply surrounding text in double or single quotes is fine, same for groups, no dollar sign needed, only brackets. Introduced the truncate transform as either Attributes on properties are now And some general cleanup. Whats left is mostly:
example of the current syntax: |
|
So this is basically done but hit a bit of a roadblock ron-rs/ron#577 The current implementation with this issue will break existing configs if they use stuff like Either we introduce a new set of options (browser_song_format_v2, format instead of content for Property panes, etc) like it was in the first place, or we wait whether this will get fixed at some point. I dont see any other options. |
|
What would the disadvantage be if we introduce a new set of options? I am not sure I understand what you mean with this
|
If the issue I reported didnt exist, then we could have browser_song_format: "$file"and browser_song_format: [(kind: Property(Filename))]But without it getting fixed we would need Same goes for the other places: song table format, property panes, tabs and layout. And the disadvantage to introducing new options: Just that, its a new set of options that does not need to exist, its an additional complexity that can be avoided. |
|
I see. I think temporally adding new options appended with something like "_templating" (easy find and replace) would be good until those issues you linked are fixed. If you think this is worth it. |
I personally would not mind, but once something is added and used by anyone it will then be a breaking change to their config to remove the option again and I want to avoid those as much as possible. |
This is a working implementation of templating as discussed here #263. I am mostly looking for opinions/suggestions at this point. Tagging you because you have shown interest in this @roxwize.
Currently it works only forWorks for layout, borwser_song_format and the song_table_formatPropertypanes in the layout and you have to provideformatinstead ofcontent.Queuefor example and the status one is usable in theHeaderandPropertypanes.{ mods: bold, underlined }It allows to transform for example this:
( size: "23", pane: Pane(Property( content: [ (kind: Property(Status(Elapsed))), (kind: Text(" / ")), (kind: Property(Status(Duration))), (kind: Group([ (kind: Text(" (")), (kind: Property(Status(Bitrate))), (kind: Text(" kbps)")), ])), ], align: Left, )), ),into this:
( size: "23", pane: Pane(Property( format: "$st:el' / '$st:dur{mods: bold}[' ('$st:br ' kbps)']", align: Left, )), ),These definitions are equivalent.
Some syntax remarks:
Groupkind is specified as adollar sign andsquare brackets[ ... ]Textkind isa dollar sign andeither single or double quotes:'this is raw text',"also raw text","escaping \"works too\""Stickeris currently$sticker(name: "playCount")'$consume(on_label: "on", off_label: "off")or shortened to$cons(on_label: "on", off_label: "off")default(fallback value) is delimited by|:$s:title|'No song'$s:title{fg: yellow, bg: black, mods: bold}|'No song'{fg: red, bg: black}bold,dim,italic,underlined,crossedout,reversed$st:consume(onLabel: 'on', offLabel: 'off')and$st:consume(onLabel:'on',offLabel:'off')should both workYou can see some tests in parser.rs to see more examples.