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. _______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : [email protected] Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
