Sinono3 / souvlaki

A cross-platform library for handling OS media controls and metadata.
MIT License
80 stars 15 forks source link

Add MacOS backend, switch to callback model #3

Closed jpochyla closed 3 years ago

jpochyla commented 3 years ago

Hi!

There're a couple of unrelated but intertwined things in this PR, I'm sorry. We can split it if it's a problem.

I've tried to adjust the Windows code to the best of my knowledge, but I can't really compile or test it :( Is that OK?

Thanks for the library!

jpochyla commented 3 years ago

Btw the PR branch is writable, in case you want to change something, please feel free!

Sinono3 commented 3 years ago

Hi, sorry for taking so long. I actually was about to commit some changes to the branch, nothing big really, just got it working on Windows again. This weekend I'll pull your new changes and get it working again (hopefully). I actually had written a few notes about some things, and I'll post them this weekend as well.

One thing I'd like to discuss now though is if it's necessary or useful (at least in your platform) to have every media controls function return a Result. Shouldn't all actions with the media controls be infallible? In Windows I don't see a reason it could fail. If I'm correct, originally the Windows MediaControls I wrote returns Result but honestly at the time I actually didn't put much thought into it. So, the question is, what are possible errors that could occur during a change in the media controls (metadata, playback state, etc).

Now that I think about it, in Linux (MPRIS), there could be a point of failure in the D-Bus if it doesn't respond or maybe if it isn't running at all. So maybe it is necessary, though I don't see use of the Results in the macOS implementation. We'll most probably use them anyway for cross-platform compatibility.

Great job on the simplification! Everything is much neater now. You were right on that, the Ext traits felt off, especially importing them for MediaControls creation functions.

Sinono3 commented 3 years ago

I don't see why we shouldn't merge this now, my only concern was that detach wasn't actually enforced, thus the Drop implementation. I have yet to test this last part on Windows.

jpochyla commented 3 years ago

Hi @Sinono3, I'm sorry about the delays as well, I've been vacationing AFK and will be back this weekend. Thanks for the fixing the Windows backend, and sorry that the changeset is so big! The idea behind returning a Result is to keep the same interface between the platforms, yes.

jpochyla commented 3 years ago

Alright I'm back! Are you OK with merging this?

Sinono3 commented 3 years ago

Yes! Merging.

jpochyla commented 3 years ago

Thank you! ❤️