4br3mm0rd / mpyg321

mpg321 wrapper for python - command line mp3 player
MIT License
23 stars 5 forks source link

Adds support for events #43

Closed 4br3mm0rd closed 6 months ago

4br3mm0rd commented 7 months ago

Here is my suggestion for event based callbacks.

I believe that we do not need to use the Events library since the logic is already in here and doesn't take too much code. It will allow us to maintain it better than the other repository (we are not sure how well it will be maintained in the future).

Let me know what you think. I also added an example file.

stefets commented 6 months ago

Hi! Good job! Works great in my setup but I have a suggestion.

Is this possible to trigger an event with a parameter? With the Event lib, it is possible to trigger an event with a parameter, I would like to subscribe to an on_error event and having the output of the error in the callback. The advantage is that we can send data through an event for specific case.

# Event lib prototype
# Mpyg
def on_error(self):
        """Process errors encountered by the player"""
        output = self.player.readline().decode("utf-8")
        self.events.on_error(output)

# Client
self.wrapper.events.on_error += self.handle_error
def handle_error(self, error):
        print(f"MP3: An error occurs : {error}")

Thanks!

4br3mm0rd commented 6 months ago

Sure, I will work on it, it's pretty straight forward but we can basically have a context variable passed to any event. We will have a specific class for every event. I'll add it in the coming days

stefets commented 6 months ago

Hi! this is not urgent! I'm not sure I understand correctly, if we have 10 events, we will need a class for each event? Does this risk further complicating the BasePlayer class? I wanted to avoid reinventing the wheel by using the Events lib which by default manages the main use cases I have to deal with; also avoid expanding the BasePlayer class but despite everything, nothing prevents us from implementing it ourselves directly in the lib because in fact, we keep control of this feature THANKS!

4br3mm0rd commented 6 months ago

Hey! I added the feature: from now on, every event will be triggered with a "context" parameter (so you will need to add a "context" argument to all of your event callbacks). The context contains a reference to the player (in case we need it for debug or anything). For errors, the context also contains the error type and message.

Let me know if you have any idea for other information we could put for event context (globals or specific events).

stefets commented 6 months ago

Hello! It works very well and the enum + context parameter is a great addition! For now I would add a trigger for MUTE and UNMUTE. These are implemented for mpg123. This will have no effect for mpg321 users, even it will be possible to register to the un.mute event, it will never triggered. On my use case, I will create a logger on the error event. Thanks a lot!

4br3mm0rd commented 6 months ago

Alright, I'm pushing the changes then :-) Solves #42