Skip to content
This repository has been archived by the owner on Sep 6, 2024. It is now read-only.

WIP: MPRIS #4

Closed
wants to merge 5 commits into from
Closed

WIP: MPRIS #4

wants to merge 5 commits into from

Conversation

rubdos
Copy link
Contributor

@rubdos rubdos commented Mar 30, 2018

PR for early feedback.

todo:

  • rustfmt

Copy link
Owner

@shymega shymega left a comment

Choose a reason for hiding this comment

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

Looks good so far, I'll probably make some changes once merged however. I like the event-driven changes, I think that suits the purpose better.

I need to change the new() method on the Song struct to use String::new() instead of "".to_string(). I don't know why I did that. I guess that was when I was a bit new to Rust!

As we discussed on IRC, there's some changes that need to be made upstream, so I won't merge this PR just yet, given that it fails on CI, and that we're waiting on upstream (mpris-rs) to review your issue you made.

Otherwise, good job! 👍

@rubdos
Copy link
Contributor Author

rubdos commented Mar 31, 2018

I need to change the new() method on the Song struct to use String::new() instead of "".to_string(). I don't know why I did that. I guess that was when I was a bit new to Rust!

Wrt. to that, there's a #[derive(Default)] on struct Song; why not just use that for the time being? ;-)

@shymega
Copy link
Owner

shymega commented Mar 31, 2018

@rubdos I can make the change on master, I just don't want to mess up your PR. Would you mind if I made a few changes on master?

@rubdos
Copy link
Contributor Author

rubdos commented Mar 31, 2018

@rubdos I can make the change on master, I just don't want to mess up your PR. Would you mind if I made a few changes on master?

What's the change you have in mind? Getting rid of new()? FYI: I'm on IRC if you want quicker response :-)

@rubdos
Copy link
Contributor Author

rubdos commented Jul 10, 2018

I just (finally) incorporated a PoC for Mange/mpris-rs#36 .

scrobble starting NOW..
mpris event Ok(Playing)
Event: NowPlaying(Song { title: "", album: "", artist: "", album_artist: "", date: "", genre: "", track: "", composer: "" })
mpris event Ok(Paused)
mpris event Ok(Playing)
Event: NowPlaying(Song { title: "", album: "", artist: "", album_artist: "", date: "", genre: "", track: "", composer: "" })

(Song doesn't get filled yet)

@shymega
Copy link
Owner

shymega commented Jul 10, 2018

@rubdos PR looks good. Don't worry about filing the song. I'll do the DB stuff :-)

There is a conflict with Cargo.{lock,toml}, which I'd like to see resolved in the PR before I merge.

Thank you for your contribution to scrobble.rs! :+1

@rubdos
Copy link
Contributor Author

rubdos commented Jul 12, 2018

There is a conflict with Cargo.{lock,toml}, which I'd like to see resolved in the PR before I merge.

Would you merge it as-is for the rest? Because it's just a PoC at this point :'-)

@shymega
Copy link
Owner

shymega commented Jul 12, 2018

I'll see if I can merge it, but I'll be merging into a development branch for the MPRIS branch, and not into master if at all.

I would prefer that more testing is done on the PR itself before merging into the main codebase.

@shymega
Copy link
Owner

shymega commented Sep 2, 2018

I have merged your PR into the codebase as of 18bc111, which seems to be working correctly.

As I have merged the PR, it will now be closed. Please do open another PR for future improvements. Many thanks for your contribution!

@shymega shymega closed this Sep 2, 2018
@shymega shymega requested review from shymega and removed request for shymega November 11, 2021 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants