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

Add track_format config option #800

Merged
merged 14 commits into from
May 28, 2022
Merged

Add track_format config option #800

merged 14 commits into from
May 28, 2022

Conversation

Bettehem
Copy link
Contributor

@Bettehem Bettehem commented May 1, 2022

Hello again!
I added a new config option: tracklist_formatting.
This solves #730 and #558

…mes should be shown before or after the track name.
@hrkfdn
Copy link
Owner

hrkfdn commented May 9, 2022

Thanks for the PR, appreciate it. I think a more flexible/customizable approach would be the one described here: #558

It would allow custom formats for tracks (and maybe albums and other types) so users can decide for themselves, but is probably a little bit more complex.

1 similar comment
@hrkfdn
Copy link
Owner

hrkfdn commented May 9, 2022

Thanks for the PR, appreciate it. I think a more flexible/customizable approach would be the one described here: #558

It would allow custom formats for tracks (and maybe albums and other types) so users can decide for themselves, but is probably a little bit more complex.

@Bettehem
Copy link
Contributor Author

Bettehem commented May 9, 2022

Oh yeah, that sounds like a good idea! I could try changing it to work like that🤔

…h columns are visible in Queue/Library view.

This also removes the need for a separate track_name_first and album_column option.
@Bettehem
Copy link
Contributor Author

Hello, I have made some some modifications now. I added an active_fields config option which lets you configure what should be shown and in which column. This also removes the need for a separate album_column option. I also added some examples to the README. This should now solve both #730 and #558

Copy link
Owner

@hrkfdn hrkfdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reiterating and tackling this. I have a suggestion in the comment below, it would also apply to the other columns. What do you think?

Comment on lines 166 to 201
fields.append(
&mut active_fields
.left
.unwrap_or(vec![ActiveField::Artists, ActiveField::Title])
.clone(),
);
fields.append(
&mut active_fields
.center
.unwrap_or(vec![ActiveField::Album])
.clone(),
);
if fields.contains(&ActiveField::Title) && !fields.contains(&ActiveField::Artists) {
write!(f, "{}", self.title)
} else if fields.contains(&ActiveField::Artists) && !fields.contains(&ActiveField::Title) {
write!(f, "{}", self.artists.join(", "))
} else if !fields.contains(&ActiveField::Default)
&& !fields.contains(&ActiveField::Title)
&& !fields.contains(&ActiveField::Artists)
{
write!(f, "")
} else {
let mut title_index = 1;
let mut artists_index = 0;
for (i, field) in fields.iter().enumerate() {
match field {
config::ActiveField::Title => title_index = i,
config::ActiveField::Artists => artists_index = i,
_ => {}
}
}
if title_index < artists_index {
write!(f, "{} - {}", self.title, self.artists.join(", "))
} else {
write!(f, "{} - {}", self.artists.join(", "), self.title)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of logic here to generate the output string for the columns. I'm wondering if there is a more dynamic approach, i.e. the user can provide something like format_left = "%artists - %title" which is then replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I like the idea. It would probably make the logic simpler. I'll see what I can do!

Updated readme with new instructions.
Reformatted impl member order to match the definitions in traits.rs.
@Bettehem
Copy link
Contributor Author

Hi, Sorry for taking so long, I have been busy with other stuff, but I have now made a more flexible system for customizing the formatting. What do you think?

@Bettehem Bettehem changed the title Added track_name_first config option Added tracklist_formatting config option May 27, 2022
Bettehem and others added 5 commits May 27, 2022 23:29
…mes should be shown before or after the track name.
…h columns are visible in Queue/Library view.

This also removes the need for a separate track_name_first and album_column option.
Updated readme with new instructions.
Reformatted impl member order to match the definitions in traits.rs.
Instead of the lazy static mutex
@hrkfdn
Copy link
Owner

hrkfdn commented May 27, 2022

Thanks, looks pretty good already. I've rebased the latest main and did a small refactoring. I'm wondering if we should support formatting for podcast episodes as well? I think they may stick out if you have a playlist with a mix of tracks and podcasts..

@Bettehem
Copy link
Contributor Author

Oh, true. I didn't know you could mix podcasts and songs in the same playlist 🤦
But yeah the formatting should probably work for podcast episodes too if they can be added to the same list.
Thanks for the feedback again, I'll try to make it work for episodes too!

…e_first

# Conflicts:
#	README.md
#	src/config.rs
#	src/model/album.rs
#	src/model/artist.rs
#	src/model/episode.rs
#	src/model/playable.rs
#	src/model/playlist.rs
#	src/model/show.rs
#	src/model/track.rs
#	src/traits.rs
#	src/ui/contextmenu.rs
#	src/ui/listview.rs
@Bettehem
Copy link
Contributor Author

Hmm, I was just testing, and noticed that the custom formatting already works for Episodes in playlists, because episodes in playlists are being treated as tracks. I made a new commit which moves the custom format function to Playable impl so if in the future playlists differentiate between Episodes and Playlists its easier to implement

@hrkfdn
Copy link
Owner

hrkfdn commented May 28, 2022

Hmm, I was just testing, and noticed that the custom formatting already works for Episodes in playlists, because episodes in playlists are being treated as tracks.

Huh, indeed. Sounds like a bug, I've just checked and it looks like either rspotify or the Spotify API returns them all as tracks. I have reported this upstream at rspotify: ramsayleung/rspotify#320

I made a new commit which moves the custom format function to Playable impl so if in the future playlists differentiate between Episodes and Playlists its easier to implement.

That makes sense, thanks.

I think we can merge this now. I was wondering whether we should rename the config variables to track_format as it's a little bit shorter and reflects the affected playable type better. What do you think?

Once the bug is fixed we can also support custom podcast formatting, which may have different fields. Also in the future we could add support custom formatting for other playable types, i.e. albums (album_format).

Also shorten `format_{left|center|right}` to `{left|center|right}`
@hrkfdn hrkfdn changed the title Added tracklist_formatting config option Add track_format config option May 28, 2022
@hrkfdn hrkfdn merged commit 4ea3d4a into hrkfdn:main May 28, 2022
@hrkfdn
Copy link
Owner

hrkfdn commented May 28, 2022

I have changed the naming and merged it. Thanks for your contribution and considering all the annoying comments 😄

hrkfdn added a commit that referenced this pull request May 28, 2022
* Added track_name_first config option to allow choosing if artists' names should be shown before or after the track name.

* Added active_fields config option, which allows configuration of which columns are visible in Queue/Library view.
This also removes the need for a separate track_name_first and album_column option.

* Fixed README

* Made custom tracklist formatting more flexible.
Updated readme with new instructions.
Reformatted impl member order to match the definitions in traits.rs.

* Added track_name_first config option to allow choosing if artists' names should be shown before or after the track name.

* Added active_fields config option, which allows configuration of which columns are visible in Queue/Library view.
This also removes the need for a separate track_name_first and album_column option.

* Fixed README

* Made custom tracklist formatting more flexible.
Updated readme with new instructions.
Reformatted impl member order to match the definitions in traits.rs.

* Fetch formatting config from library config

Instead of the lazy static mutex

* Moved custom format function to Playable impl as it's a better location to handle both Tracks and Episodes

* Rename from `tracklist_formatting` to `track_format`

Also shorten `format_{left|center|right}` to `{left|center|right}`

Co-authored-by: Henrik Friedrichsen <henrik@affekt.org>
@Bettehem
Copy link
Contributor Author

I have changed the naming and merged it. Thanks for your contribution and considering all the annoying comments 😄

Nice, and thank you for taking the time to give feedback and suggestions, I'm still a Rust noob so it has been very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants