-
Notifications
You must be signed in to change notification settings - Fork 5
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
Custom formatting via config.toml #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you! I think I primarily want to move the replace logic to the OutputFormatter
. It would be nice if we additionally didn't hardcode the unicode icons. I've left a possible solution for that as well that would be relatively simple, even if a bit boilerplate-y.
src/commands/monitor.rs
Outdated
let status_icon = match status { | ||
TrackStatus::Playing => " ", | ||
TrackStatus::Paused => " ", | ||
_ => "" // unable to get player state | ||
}; | ||
output_format | ||
.replace("{artist}", &artist) | ||
.replace("{title}", &title) | ||
.replace("{separator}", separator) | ||
.replace("{status}", status_icon) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to OutputFormatter
? If the replacing is done on all Output.text
, we could make the other messages below configurable as well.
Additionally, the tooltip can be configurable.
You'll need to add a template_fields: HashMap<String, String>
or something similar where you pass what needs replacing. Otherwise the OutputFormatter
won't know.
#[serde(default = "default_format")] | ||
pub format: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having some extra formatting options?
pub struct TemplateConfigs {
#[serde(default = "default_text_template")]
pub text: TemplateConfig,
#[serde(default = "default_tooltip_template")]
pub tooltip: TemplateConfig,
}
pub struct TemplateConfig {
pub default: String,
pub playing: String,
pub stopped: String,
pub paused: String,
pub added: String,
pub removed: String,
}
This would allow configuration of icons and more so we do not hardcode those 2 unicode characters.
I'll take a look at this today and see what I can come up with. |
ca49290
to
b5b82ae
Compare
I've done a bit more work on this. If you like the direction it's going I can add tooltip customization, but would rather wait to see if you want it refactored at all first. I also started a review regarding the TrackStatus::Added/Removed triggers because I don't think they work. |
Thank you! I'll try to give this a review this evening after work. |
Forgot to leave a final comment with the review. While I think the config can be improved, this is an improvement and I'm happy to extend it so it's better for you to use. I left some suggestions and with those applied, I think we can merge this. Also please add an "Added" section to the Unreleased section of |
Co-authored-by: Andreas Backx <[email protected]>
Apply suggestions from code review Co-authored-by: Andreas Backx <[email protected]>
Should be good to merge after this. |
Still waiting on some feedback on the Added/Removed logic not triggering |
@dotaxis I'm unsure if you're referring to feedback from me. You mentioned the below earlier but I'm not sure what you mean with that review.
Let me know if you need any of my help. |
I forgot to submit my review so only I could see it. Oops. |
Sorry for the delay, been busy all week. Will get this done over the weekend. |
No rush at all, take all the time you need. You're the primary user for it. |
6339820
to
08a5342
Compare
Okay, I added a line in the changelog. Let me know if you need anything else done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, though make sure to test that they work. 😅 It does not compile right now.
@@ -25,6 +27,68 @@ pub struct OutputFormatter { | |||
} | |||
|
|||
impl OutputFormatter { | |||
pub fn format_output(&self, response: MonitorResponse, status: TrackStatus, output_format: &str) -> Output { | |||
let text_template = &self.config.text_template; | |||
if let Some(track) = response.track { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not closing this properly:
error: unexpected closing delimiter: `}`
--> src/commands/output.rs:120:1
|
42 | (Some(artist), Some(title)) => {
| - this delimiter might not be properly closed...
...
53 | };
| - ...as it matches this but it has different indentation
...
120 | }
| ^ unexpected closing delimiter
error: could not compile `spotifatius` (bin "spotifatius") due to 1 previous error
bd6ed5f
to
47d32a4
Compare
There was an extra closing brace in the change you requested on line 52. |
Thanks again! It might take me some time to create a new version however. You should be able to use it yourself in the meantime however. |
This allows customizing the output of spotifatius using the config.toml.
Includes
{status}
var which will print a play/pause unicode character depending on if the track is playing or paused.Open to any suggestions on how to make it better, just let me know.
Also address #5