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

Fix edge cases of player app setup #870

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix edge cases of player app setup #870

wants to merge 2 commits into from

Conversation

Eskuero
Copy link

@Eskuero Eskuero commented Jul 28, 2018

Allow VLC to be autodetected and allow changing player if the old one was uninstalled.

Eskuero added 2 commits July 28, 2018 12:37
Even if vlc is the only installed supported player mpsyt would not pick it upon config first generation and force to manually specify it within app
If the configured player have been uninstalled before specifying a new one you could end locked out of mpsyt because of the program crashing to FileNotFoundError.
Handle this exception and still allow to enter the main interface to change the player without having to reinstall the older one.

Traceback (most recent call last):
  File "/home/eskuero/.local/bin/mpsyt", line 7, in <module>
    from mps_youtube import main
  File "/home/eskuero/.local/lib/python3.5/site-packages/mps_youtube/__init__.py", line 8, in <module>
    init.init()
  File "/home/eskuero/.local/lib/python3.5/site-packages/mps_youtube/init.py", line 59, in init
    assign_player(config.PLAYER.get)  # Player is not assigned when config is loaded
  File "/home/eskuero/.local/lib/python3.5/site-packages/mps_youtube/util.py", line 544, in assign_player
    g.PLAYER_OBJ = pl(player)
  File "/home/eskuero/.local/lib/python3.5/site-packages/mps_youtube/players/mplayer.py", line 27, in __init__
    self.mplayer_version = _get_mplayer_version(player)
  File "/home/eskuero/.local/lib/python3.5/site-packages/mps_youtube/players/mplayer.py", line 238, in _get_mplayer_version
    o = subprocess.check_output([exename]).decode()
  File "/usr/lib/python3.5/subprocess.py", line 316, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.5/subprocess.py", line 383, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.5/subprocess.py", line 676, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.5/subprocess.py", line 1282, in _execute_child
    raise child_exception_type(errno_num, err_msg)
FileNotFoundError: [Errno 2] No such file or directory: 'mplayer'
@tommysolsen
Copy link
Member

LGTM

Copy link
Member

@vn-ki vn-ki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vlc support is really bad. For example try
playing a playlist.

IMO, it shouldn't be auto detected.

assign_player(config.PLAYER.get) # Player is not assigned when config is loaded
except FileNotFoundError:
print("Configured player " + config.PLAYER.get + " does not exist.")
print("A new one must be specified by the command 'set player <mplayer|mpv|vlc>' within the app")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set player is not limited to mplayer, mpv and vlc.

@Eskuero
Copy link
Author

Eskuero commented Jul 28, 2018

What if I put the elif of vlc on the last spot to hold it as last resort?

Which other players should I put on the example command?

@Eskuero
Copy link
Author

Eskuero commented Aug 23, 2018

Any updates on this? Is it gonna get merged, what other changes should I do?

@tommysolsen
Copy link
Member

I just came across a case where I was installing mpsyt on a new computer without mplayer and having the program crash before being able to configure mpv as my player of choice. Having it autodetect players will be great.

I don't really use VLC, but if its not working properly I think we should only autodetect the two actually supported players, mpv and mplayer.

I would like it to say something along the lines of Install and set the player using _set player mpv|mplayer instead of the current message. Because the reason the user sees the message in the first place is because mpsyt couldnt find any of them.

And while we do technically make it possible to use other players than those two, we don't support them officially so there is no need to mention that here.

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

Successfully merging this pull request may close these issues.

3 participants