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

Show stream details in video information #675

Merged
merged 7 commits into from Aug 24, 2017
Merged

Show stream details in video information #675

merged 7 commits into from Aug 24, 2017

Conversation

ritiek
Copy link
Member

@ritiek ritiek commented Aug 18, 2017

I splitted part of _playsong function in player.py into stream_details function. stream_details() is now also called (in misc.py) to extract stream information (using i <number>) that mpsyt is supposed to play.

Closes #196.

@ids1024
Copy link
Contributor

ids1024 commented Aug 18, 2017

I'm not sure if this information should be displayed in the info for a video, because streams are actually in several formats, although this displays the one it will play. Therefore, show_video for instance changes what it displays.

@ritiek
Copy link
Member Author

ritiek commented Aug 18, 2017

Not sure either but I couldn't find a better place to show this info. Displaying all the stream urls will indeed create a mess. I made a stream info section. IMO It should hint people that it will display information about the stream it will play, so they would know what to expect (mp4, webm or anything else).

Another way could be to add a command like s <number> or something that will show stream info. What do you think?

@ids1024
Copy link
Contributor

ids1024 commented Aug 20, 2017

The long stream urls do add clutter for the (probably) majority of uses where that is not the goal of the info command (four lines for me with a full screen terminal; worse with a smaller one).

Another way to provide the functionality of #196 would be to add a command to copy the url to the clipboard; the x command (which requires pyperclip to be installed) copies the video url; perhaps an X command that copies the stream url?

@ritiek
Copy link
Member Author

ritiek commented Aug 20, 2017

Yeah, the X command would be a nice way to copy the stream url. I'll put some work to it.

However, what about adding a new s command anyway that displays further information (bitrate, extension, etc) about the stream?

@ids1024
Copy link
Contributor

ids1024 commented Aug 20, 2017

However, what about adding a new s command anyway that displays further information (bitrate, extension, etc) about the stream?

That could be useful as well, so sure, it can be added.

@ritiek
Copy link
Member Author

ritiek commented Aug 22, 2017

OK, added both X and s commands. Let me know if anything needs to be changed.

@@ -134,7 +135,7 @@ def _mplayer_help(short=True):
return lines.format(c.g, c.w)


def _playsong(song, failcount=0, override=False, softrepeat=False):
def stream_details(song, failcount=0, override=False, softrepeat=False):
""" Play song using config.PLAYER called with args config.PLAYERARGS."""
# pylint: disable=R0911,R0912
if not config.PLAYER.get or not util.has_exefile(config.PLAYER.get):
Copy link
Contributor

Choose a reason for hiding this comment

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

It should only test for a player when actually playing. Nor should the NOTIFIER be called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, missed that.

@ids1024 ids1024 merged commit 531e6b9 into mps-youtube:develop Aug 24, 2017
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.

None yet

2 participants