openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #21561
Re: [Merge] lp:~tomasgroth/openlp/mupdf into lp:openlp
Review: Needs Fixing
Looks good but these need to be fixed.
* settings screen the grouping has moved from the left side to the middle.
* settings screen "binary" has the "y" half chopped off.
* showing an image live / preview does not work when loaded from the service manager. Fine from the media manager. Have not tried to save / reload but guess that is broken.
* blank lines in functions please remove (general)
* questions in comments please removed.
* check_binary why is this outside the class as a standalone function?
* if os.name != u'nt': swap logic round so is positive and less chance of a miss read.
* 320 would this be better to have in the code a file instead of writing to a file each run?
* if self.pdf_program_path.text() != u'': should be if not self.pdf_program_path.text(): - General point.
* setting do not need "given" in the string.
* img should be image (old bad code copied!
* no tests.
* line 53 far too long.
* lines 52-64 do we need all this. if the images are generated then it would be a case of just loading them not processing them again?
--
https://code.launchpad.net/~tomasgroth/openlp/mupdf/+merge/182009
Your team OpenLP Core is subscribed to branch lp:openlp.
References