openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing
Just a couple of small things as I browse through this:

215	+ def getDisplayJavascript(self):

Technically, it is "JavaScript", not "Javascript"

329	+ self.webView.settings().setAttribute(7, True)
394	+ self.webView.settings().setAttribute(7, True)

What does the 7 stand for? Is there an enumeration you could use?

439	+ or not self.isVisible():# or self.videoWidget.isVisible():

Please either remove the commented part, or put it on its own line.

754	=== added file 'openlp/core/ui/media/mediaapi.py'

(Optional) You can also name it "media_api.py" if you want. It's a little easier to read :-)

734	+ Cd = 3
735	+ Dvd = 4

CD and DVD can be all capital letters, it also makes it slightly easier to read.

789	+ Specialiced Media API class
790	+ to reflect Features of the related API

"A generic media API class to provide OpenLP with a pluggable media display framework."

938	+class MediaManager(object):

Can we rather name this the MediaController? We already have another class named "MediaManager" and I don't want developers to get confused by the two.

1414	+ Specialiced MediaAPI class
1415	+ to reflect Features of the Phonon API


"A specialised version of the MediaAPI class, which provides a Phonon display."

1633	+ Specialiced MediaAPI class
1634	+ to reflect Features of the QtWebkit API

"A specialised version of the MediaAPI class, which provides a QtWebKit display."

2028	+ sender = self.sender().objectName() or self.sender().text()

This is the old-style ternary operator shortcut for Python. Rather use the new one, which is a little longer, but is much more accurate: sender = self.sender().objectName() if self.sender().objectName() else self.sender().text()

2386	+ #(path, name) = os.path.split(filename)

If it's commented out, and you're not going to use it, remove it.

Phew, that's all I have time for right now.
-- 
https://code.launchpad.net/~crichter/openlp/media_rewrite/+merge/73286
Your team OpenLP Core is subscribed to branch lp:openlp.