4br3mm0rd / mpyg321

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

Feature Request: Enhance --version Switch in mpg123/mpg321 for Version Checking #45

Open stefets opened 4 months ago

stefets commented 4 months ago

I would like to submit a feature request related to the version-checking functionality of mpg123 and mpg321. Currently, both utilities support the --version switch to display version information. However, I propose an enhancement to this feature to facilitate conditional checks based on the version number.

The requested enhancement is as follows:

  1. Detailed Version Information: Expand the --version output to include more detailed version information, ideally formatted in a consistent and parseable manner. This information must include the version number.
  2. Standardized Output Format: Ensure that the version information is presented consistently across different releases and versions of mpg123/mpg321. This will enable reliable parsing of the output in the wrapper.
  3. Compatibility: Maintain backward compatibility in the wrapper for conditions that rely on the --version switch.
  4. Conditional Version Checking: Enable conditional checks based on the version number retrieved from the --version output. This could involve providing an easily accessible version number (e.g., just the version string without additional information) that can be used in the wrapper for conditional logic.

The actual output for mpg123 is :

$ mpg123 --version
mpg123 1.32.6

Where 1.32.6 represents the version number. This format would allow the wrapper to reliably extract and compare version numbers.

I appreciate your consideration of this feature request. If you have any questions or require further clarification, please feel free to reach out!

Best regards,

4br3mm0rd commented 4 months ago

Hey @stefets ! Thanks for your contribution (again!) This would sure be fun to develop, but I have a few questions:

Thank you :-)

stefets commented 4 months ago

Hello!

* Do you know any output differences from one version to another (of the `--version` command)?

I know for mpg123 and the stdout is mpg123 1.32.6 easy to parse with Python I don't know for mpg321 since I don't use it. Do you use it ?

* Do you have something in mind for the use that we could have of the additional information we would get? How would this information change the behavior of the library?

Yes!

  1. Handle the P 3 command in remote, I have to deal with it in my code and this is the responsibility of the wrapper; The wrapper should take care of this by checking the version for example in the base class can be like :
    # For mpg123 >= v1.3*.*
    if self.version_major >= 1 and self.version_minor >= 3:
    self.mpg_outs.append(
    {
    "mpg_code": "@P 3",
    "action": "end_of_song",
    "description": "Player has reached the end of the song.",
    }
    )
  2. Handle future changes in the remote module of mpg123; The remote module in mpg123 has changed since we got the P 3 command in the version 1.30.0 not yet implemented in the wrapper.

Keep in mind that mpg123 evolve and mpg321 is dead since almost 15 years.

Thanks!

stefets commented 4 months ago

Another refactor possible with this feature would be to move the def volume to the BasePlayer class. Anyway, we support two different players, I think it make sense having a condition in the BasePlayer class that will send "GAIN" or "VOLUME" depending the player type. With this move, the mpyg321 child class will have no specifics behaviour and will be 100% managed by the base class.

stefets commented 4 months ago

I also suggest a PlayerVersion class that will be created by each child class

class PlayerVersion:
    def __init__(self):
        self.name = "mpg123"
        self.major = None # Get from subprocess
        self.minor = None # Get from subprocess
        self.patch = None # Get from subprocess

As an usage example:

if version.name == "mpg123":
   RemoteCommands.Volume = "VOLUME"
else:
   RemoteCommands.Volume = "GAIN"

Or

if version.name == "mpg123" and version.major >= 1 and version.minor >= 30:
    # Implement specific version feature like @ P 3
    pass
4br3mm0rd commented 4 months ago

About the versions, I understand. I think that we can start to add parsing for the version. Can you give me a list of the different versions you want to handle and I will implement it the way I see it (for example, I think the versions should be handled in mpg_outs_ext).

Another refactor possible with this feature would be to move the def volume to the BasePlayer class. Anyway, we support two different players, I think it make sense having a condition in the BasePlayer class that will send "GAIN" or "VOLUME" depending the player type. With this move, the mpyg321 child class will have no specifics behaviour and will be 100% managed by the base class.

I believe that the base class should not have any handling of the versions. That's the whole idea behind it.

stefets commented 4 months ago

I think the versions should be handled in mpg_outs_ext

Yes I agree!

I believe that the base class should not have any handling of the versions. That's the whole idea behind it.

Yes, I totally agree! my idea behind was to use constants for the sendline calls.

Can you give me a list of the different versions you want to handle

Yes, the @ P 3 for the end of song asked by us in the 1.30.0 version, this is my code to handle it, but added to the mpg_outs list. https://www.mpg123.de/cgi-bin/news.cgi#2022-06-26

# For mpg123 >= v1.3*.*
self.wrapper.mpg_outs.append(
    {
        "mpg_code": "@P 3",
        "action": "end_of_song",
        "description": "Player has reached the end of the song.",
    }
)

Also @ PROGRESS that is the opposite of @ SILENCE; I think that we can improve this part, if we are not @ SILENCE, frames are stdout @ F 139 9823 3.63 256.60 so we can track the frame number in real time. I have not need for this but it is possible.

After, In version 1.31.0 https://www.mpg123.de/cgi-bin/news.cgi#2022-10-28

Change error message from 'unknown command' to 'unknown command with arguments' to avoid confusion why 'help foo' is unknown, as opposed to 'help'.

This impact the mpg_errors list.

Thanks!

4br3mm0rd commented 4 months ago

Alright I will do that on my free time, thank you!