openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Review: Needs Fixing

168	+ except appscript.CantLaunchApplicationError:
169	+     pass
Please log that the app could not be launched.


285	+ except appscript.CommandError:
286	+    pass
Please log your error (u'Could not close the presentation')


299	+ if len(windows) == 0:
302	+ if len(slideshows) == 0:
Just do "if my_list"


304	+ except:
305	+     return False
Please add a log message. Probably you should log the whole exception, so it is available when debugging.


Lines 550-560: Is this code needed? If not, remove it. If you need it for "information purpose" then rather add it to the methods docs or add a comment.


Can you explain lines 607-613. "Could not create tmp dir"? There is a function (check_directory_exists) which can be used to check/create dirs. 


608	+ self.presentation.save(in_ = thumbnail_folder, as_ = appscript.k.save_as_PNG)
Should be self.presentation.save(in_=thumbnail_folder, as_=appscript.k.save_as_PNG)
(No spaces around the "=" sign after keyword arguments.)


837	+ text = u''
838	+ for idx in range(len(shapes)):
839	+     shape = shapes[idx + 1]
840	+     if shape.has_text_frame():
841	+         text += shape.text_frame.text_range.content() + '\n'
842	+ return text
Better do:
return u'\n'.join([shape.text_frame.text_range.content() for shape in shapes if shape.has_text_frame()])
But please test if it works, I haven't tested it. Also your function has a '\n' at the end (mine does not), is this wanted?


913	+ u'PresentationModeUseSecondary': u'',
914	+ u'PresentationModeEnableFeedbackDisplay': False,
915	+ u'PresentationModePlayWellWithOthers': False, 
What are these needed for? Why do they not follow the section/key_name conversion?

Thanks for your code, if you have any questions to my comment feel free to contact me (see my profile, or just ask here). I don't want to scare you off, but we have to make sure that the code works as best as possible (we have hardly any MAC devs and the presentation plugin was always a bit our sorrow plugin ;) ).
-- 
https://code.launchpad.net/~marmyshev/openlp/presentation/+merge/156713
Your team OpenLP Core is subscribed to branch lp:openlp.