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

Big Album Cover View Now Updates on Song Change #2972

Merged
merged 1 commit into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@TheYokai
Contributor

TheYokai commented Sep 16, 2018

The big album cover view used to stay on the same image regardless of what song was currently playing. Now, when the album widget changes due to a new song, it will also change the album art displayed. If there's no album art found, the widget will simply close.

edit: Some changes should be made to this class in the future, such as allowing the album art to update without creating / destroying the widget. Some groundwork was set for splitting functionality into it's own functions.

@TheYokai TheYokai force-pushed the TheYokai:bigcover-patch branch from 843ebca to f28270f Sep 16, 2018

@TheYokai TheYokai changed the title from Big Album Cover View Now Updates On Song Change to Big Album Cover View Now Updates on Song Change Sep 17, 2018

@frestr

Thanks for the PR, this is a nice feature. See my comments for some minor things that can be changed. Otherwise it looks good!

event_box.connect('button-press-event', self.__destroy)
event_box.connect('key-press-event', self.__destroy)
self.get_child().show_all()
return (width, height)

This comment has been minimized.

@frestr

frestr Sep 19, 2018

Member

If indented to the (outermost) function scope, the other two returns can be removed

#if there's a big image displaying, it should update.
if self.__current_bci is not None:
self.__current_bci.destroy()
if file is not None:

This comment has been minimized.

@frestr

frestr Sep 19, 2018

Member

Nitpicking, but it's more pythonic to have not file. Also, perhaps use another variable name as to not override the builtin

if self.__current_bci is not None:
self.__current_bci.destroy()
if file is not None:
self.__current_bci

This comment has been minimized.

@frestr

frestr Sep 19, 2018

Member

Is this supposed to be here, or is something missing?

@TheYokai TheYokai force-pushed the TheYokai:bigcover-patch branch from f28270f to 1e6b672 Sep 19, 2018

@TheYokai

This comment has been minimized.

Contributor

TheYokai commented Sep 19, 2018

Made some of those necessary corrections. Thanks for the feedback!

@frestr

This comment has been minimized.

Member

frestr commented Sep 20, 2018

Nice. Just one last thing that I noticed: set_image should either return false (or raise some exception) on failure, such that the rest of the code in __init__ can be skipped.

As it is now, if the pixbuf fails to load in set_image the first time it is run, self.__image will not exist, but still tried to be accessed in __init__ after the call to set_image.

Large Album Cover Now Updates When Song Changes
The large album cover will now change when the song playing changes.

@TheYokai TheYokai force-pushed the TheYokai:bigcover-patch branch from 1e6b672 to 5ea359a Sep 20, 2018

@TheYokai

This comment has been minimized.

Contributor

TheYokai commented Sep 20, 2018

Ok I believe I've made changes to make that more secure.

I'm not sure on the validity of calling self.destroy inside of the constructor. That's how the code was before I contributed though, so I'm assuming that type of behavior is ok / safe?

@frestr

This comment has been minimized.

Member

frestr commented Sep 21, 2018

Yeah, I think that should be okay.

@frestr frestr merged commit f3312b8 into quodlibet:master Sep 21, 2018

6 checks passed

ci/circleci: job.fedora28 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu18.04 Your tests passed on CircleCI!
Details
ci/circleci: job.win32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
quodlibet.quodlibet #20180920.7 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment