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 cyclic import and #806 #807

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

Conversation

vn-ki
Copy link
Member

@vn-ki vn-ki commented Mar 8, 2018

Fix the import problem mentioned in this comment in #799 and fix #806.

@vn-ki
Copy link
Member Author

vn-ki commented Mar 8, 2018

@ids1024 Probably not the best place to ask this, but in this code snippet, why is there a screen.reset_terminal()? In my machine this line messes up the whole color palette of the terminal (tset -c does it too). The code works (at least, on my machine) perfectly without that line.

https://github.com/mps-youtube/mps-youtube/blob/18fa860f7517099634c453a5b4be3cf15b268e93/mps_youtube/player.py#L77-L83

@ids1024
Copy link
Contributor

ids1024 commented Mar 14, 2018

Probably not the best place to ask this, but in this code snippet, why is there a screen.reset_terminal()?

I think the concern is that the mode of the terminal could be messed up if playing is just terminated; the code is probably there because of an actual issue with that happening.

Perhaps there's a better solution, but I'm not too familiar with stty, etc.

@ids1024
Copy link
Contributor

ids1024 commented Mar 17, 2018

I don't really like moving lastfm code out of lastfm.py. Another solution to cyclic imports like this is to move the import into the function that needs it, though that's ugly here since several functions in player.py use it.

Ideally, those calls in player.py would be replaced with some generic hook, that plugins like a lastfm plugin could use.

@vn-ki
Copy link
Member Author

vn-ki commented Mar 17, 2018

I think player shouldn't import anycommand. That's why I did this. What do you propose we do?

@ids1024
Copy link
Contributor

ids1024 commented Mar 18, 2018

For now, perhaps the if g.scrobble: code that is repeated could be replaced with a method call, BasePlayer.start_playing_hook, or something like that. Then call import lastfm in that function, to deal with the cyclic import.

In the future, this would in theory be replaced with the plugin API, so plugins can register hooks to be called there...

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.

q doesn't stop for repeat songs
3 participants