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

Add new events plugin - Synchronized Lyrics #1723

Merged
merged 2 commits into from Oct 27, 2015

Conversation

Projects
None yet
2 participants
@elfalem
Contributor

elfalem commented Oct 25, 2015

This plugin reads .lrc files and shows timed lyrics.

return vb

def getTextColor(self):
v = config.get("plugins", self.PLUGIN_ID + "_txt",

This comment has been minimized.

@declension

declension Oct 26, 2015

Member

There's actually a class to wrap all this (consistently).
See PluginConfigMixin.

return int(config.get("plugins", self.PLUGIN_ID + "_fsize",
self._defaultFontSize))

def setTextColor(self, button):

This comment has been minimized.

@declension

declension Oct 26, 2015

Member

..and for set etc.

timeStamp = (timeObject.minute * 60000 + timeObject.second * 1000
+ timeObject.microsecond / 1000)
words = lyricsLine[closeBracket + 1:]
if words == "":

This comment has been minimized.

@declension

declension Oct 26, 2015

Member

Is if not words, or better, if words with reversed logic sufficient here?

If not, depending on the difference between various falsey things (empty string, None, False etc) is risky.

@declension

This comment has been minimized.

Member

declension commented Oct 26, 2015

Thanks. It looks good to me, other than a few tweaks.

Can we ensure adherence to PEP 8 naming - notably underscores for method names, not camel case.

@elfalem

This comment has been minimized.

Contributor

elfalem commented Oct 27, 2015

Thanks for the comments. I've followed PEP 8 conventions and used PluginConfigMixin.

I'm not sure I understand the comment about depending on falsey things. That area of the code will act on strings such as [03:28.87][03:09.36][01:37.52][00:48.97]Some words broken up by the brackets. The first three slices would match the empty string condition while the fourth one would fail.

declension added a commit that referenced this pull request Oct 27, 2015

Merge pull request #1723 from elfalem/plugin-synclyrics
Add new events plugin - Synchronized Lyrics

@declension declension merged commit c20cad8 into quodlibet:master Oct 27, 2015

@declension

This comment has been minimized.

Member

declension commented Oct 27, 2015

Great - thanks for the plugin.

@elfalem elfalem deleted the elfalem:plugin-synclyrics branch Jul 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment