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

Blocking method to read events #32

Closed
rubdos opened this issue Mar 30, 2018 · 12 comments
Closed

Blocking method to read events #32

rubdos opened this issue Mar 30, 2018 · 12 comments
Assignees
Labels
Milestone

Comments

@rubdos
Copy link

rubdos commented Mar 30, 2018

For the project scrobble.rs, I'm experimenting integrate mpris support.

First the good: it compiles on SailfishOS. Hooray! :D

Now, I find the interface quite cumbersome to work with. The ProgressTracker is mostly meant to track progress within a single track, right? I'm more searching for "events", like the start of a track and the end of a track.

I was wondering if something like that could be implement; for starters, just blocking the thread until an event happens would be cool!

I imagine something like

let player = ...;
let event = player.await();
match event {
    PlayerPaused => {},
    ...,
}

what do you think?

In a second stage, (optionally) integrating with dbus-tokio would be very cool, to have futures support. That way, everything can run non-blocking/event driven.

@Mange
Copy link
Owner

Mange commented Mar 31, 2018

I'm glad you like it. Yes, ProgressesTracker is meant for UIs where you want to render "what's playing".
Having a method that blocks until the next event does also make sense. Do you have some idea on what kinds of things you would expect to be events in this fashion? I think that would help us design that feature.

Async support is also something I could look at, but I'm trying to be very conservative with async in Rust until the interfaces are stable; I'm just not ready for the headache of trying to understand the tokio ecosystem yet. But please open up another issue about this and maybe we could start to draft something up. ❤️

@rubdos
Copy link
Author

rubdos commented Mar 31, 2018

I'm glad you like it. Yes, ProgressesTracker is meant for UIs where you want to render "what's playing".
Having a method that blocks until the next event does also make sense. Do you have some idea on what kinds of things you would expect to be events in this fashion? I think that would help us design that feature.

Events that I'd imagine:

  • Player Paused
  • Player Resumed
  • New song playing
  • Song ended

things like that :-)

Async support is also something I could look at, but I'm trying to be very conservative with async in Rust until the interfaces are stable; I'm just not ready for the headache of trying to understand the tokio ecosystem yet. But please open up another issue about this and maybe we could start to draft something up. heart

I've been experimenting with porting dbus-tokio to futures 0.2.0, and it seems like its dependency on tokio will get a lot lighter (seems like only tokio-reactor will be needed). I suggest that whenever tokio releases their futures 0.2 support, we start implementing this.

I'll open a new issue for tokio/futures support for further discussion :-)

@Mange
Copy link
Owner

Mange commented Mar 31, 2018

What about these?

  • Volume change
  • Playback rate change
  • Skipping

What should happen if two "changes" happen between polling? (Say track is changed, player is paused, and volume has changed)

@rubdos
Copy link
Author

rubdos commented Mar 31, 2018

What about these?

Volume change
Playback rate change
Skipping

Sure. Those events are cheap. I can skip those that I don't want, or filter them, whatever. The more events the better.

What should happen if two "changes" happen between polling? (Say track is changed, player is paused, and volume has changed)

they should both fire an event, somehow. I imagine that player.await() gets run in a loop. Look for example at how we use this paradigm with mpd as scrobble source.

@Mange
Copy link
Owner

Mange commented Apr 1, 2018

All right, then. So we need an internal buffer of events and on each poll diff last state with current state and add 0..n events on the internal buffer and return the top event on the buffer.

When called again, return top event from buffer if there is anything there. Actually checking for DBus events only happens if the buffer is empty, pretty much.

Errors I assume would be returned just like other events.

There could also be a variant that spawns a thread and adds to a Channel instead, I guess.

@Mange
Copy link
Owner

Mange commented Apr 1, 2018

I will try to get to this within the coming week.

@rubdos
Copy link
Author

rubdos commented Apr 3, 2018

There could also be a variant that spawns a thread and adds to a Channel instead, I guess.

That's kinda messy, isn't it? Would be nicer to have #33 for that :P

@Mange
Copy link
Owner

Mange commented Apr 3, 2018

Considering async is such a mess on Rust, I don't think so. I'm likely to make the async support optional until it's nice to work with/around.

I don't want all apps to be "infected" with async just because some apps wants support for it, if that makes sense.
What Color is your function? is a nice intro to that perspective.

@rubdos
Copy link
Author

rubdos commented Apr 3, 2018

Considering async is such a mess on Rust, I don't think so. I'm likely to make the async support optional until it's nice to work with/around.

Wouldn't call it a mess, but I agree on making it optional. There's no reason to make it default.

Otoh, I think that a library randomly spawning threads is also messy.

@Mange Mange changed the title Feature request/issue report Blocking method to read events Apr 16, 2018
@Mange Mange added the Feature label Apr 16, 2018
@Mange Mange added this to the 1.1 milestone Apr 16, 2018
@Mange Mange self-assigned this Apr 16, 2018
@Mange Mange mentioned this issue Apr 17, 2018
1 task
@Mange
Copy link
Owner

Mange commented Apr 17, 2018

Hey, so I just pushed a PR where I've tried to implement this. Please check out #36 and test it if possible.

Everything seems to work nicely on my machine, but granted I have not tried to really stress test it either.

@Mange
Copy link
Owner

Mange commented Aug 18, 2018

I've now merged that PR. I'll prepare a release shortly.

@Mange Mange closed this as completed Aug 18, 2018
@Mange
Copy link
Owner

Mange commented Aug 18, 2018

Version 1.1.0 has now been released.

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

No branches or pull requests

2 participants