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

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

Open
stefets opened this issue May 6, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@stefets
Copy link
Collaborator

stefets commented May 6, 2024

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,

@stefets stefets added the enhancement New feature or request label May 7, 2024
@4br3mm0rd
Copy link
Owner

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

  • Do you know any output differences from one version to another (of the --version command)?
  • 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?

Thank you :-)

@stefets
Copy link
Collaborator Author

stefets commented May 7, 2024

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.",
        }
    )
  1. 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
Copy link
Collaborator Author

stefets commented May 14, 2024

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
Copy link
Collaborator Author

stefets commented May 14, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

stefets commented May 15, 2024

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
Copy link
Owner

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

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

No branches or pull requests

2 participants