Merge lp:~maikels/openlp/myfixes into lp:openlp

Proposed by Maikel Stuivenberg
Status: Superseded
Proposed branch: lp:~maikels/openlp/myfixes
Merge into: lp:openlp
Diff against target: None lines
To merge this branch: bzr merge lp:~maikels/openlp/myfixes
Reviewer Review Type Date Requested Status
Raoul Snyman Needs Fixing
Review via email: mp+10802@code.launchpad.net

This proposal has been superseded by a proposal from 2009-08-28.

To post a comment you must log in.
Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Looks good, just a few changes before we'll accept it:

1. Don't break strings up, otherwise we end up with lots of smaller strings to be translated; see below.
2. Use my wording, as below.
3. I haven't seen any usage of QMessageBox passing strings. Please can you update your code to look like mine below.
4. Place your comments immediately under your if statements. If your comment is only one line long, use # comments. """ is generally used for docstrings.
5. There's no need for a "Cancel" option on the confirmation dialog - either they overwrite the theme, or they don't save it at all.

    result = QtGui.QMessageBox.Yes
    if os.path.exists(theme_file):
        result = QtGui.QMessageBox.question(
            self,
            translate(u'ThemeManager',u'Theme Exists'),
            translate(u'ThemeManager',u'A theme with this name already exists, would you like to overwrite it?'),
            (QtGui.QMessageBox.Yes | QtGui.QMessageBox.No),
            QtGui.QMessageBox.No)
    if result == QtGui.QMessageBox.Yes:
        # Save the theme, overwriting the existing theme if necessary.
        outfile = open(theme_file, u'w')
        outfile.write(theme_xml)
        outfile.close()
        if image_from is not None and image_from != image_to:
            shutil.copyfile(image_from, image_to)
            self.generateAndSaveImage(self.path, name, theme_xml)
            self.loadThemes()
    else:
        # Don't close the dialog - allow the user to change the name of the theme or to cancel the theme dialog completely.
        return False

review: Needs Fixing
lp:~maikels/openlp/myfixes updated
514. By Maikel Stuivenberg

fixed the theme code

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'openlp/core/ui/amendthemeform.py'
--- openlp/core/ui/amendthemeform.py 2009-08-26 05:00:19 +0000
+++ openlp/core/ui/amendthemeform.py 2009-08-27 16:21:00 +0000
@@ -135,8 +135,8 @@
135 unicode(self.theme.display_horizontalAlign), unicode(self.theme.display_verticalAlign),135 unicode(self.theme.display_horizontalAlign), unicode(self.theme.display_verticalAlign),
136 unicode(self.theme.display_wrapStyle))136 unicode(self.theme.display_wrapStyle))
137 theme = new_theme.extract_xml()137 theme = new_theme.extract_xml()
138 self.thememanager.saveTheme(theme_name, theme, save_from, save_to)138 if self.thememanager.saveTheme(theme_name, theme, save_from, save_to) is not False:
139 return QtGui.QDialog.accept(self)139 return QtGui.QDialog.accept(self)
140140
141 def loadTheme(self, theme):141 def loadTheme(self, theme):
142 log.debug(u'LoadTheme %s', theme)142 log.debug(u'LoadTheme %s', theme)
143143
=== modified file 'openlp/core/ui/thememanager.py'
--- openlp/core/ui/thememanager.py 2009-08-26 05:00:19 +0000
+++ openlp/core/ui/thememanager.py 2009-08-27 16:21:00 +0000
@@ -329,13 +329,38 @@
329 if os.path.exists(theme_dir) == False:329 if os.path.exists(theme_dir) == False:
330 os.mkdir(os.path.join(self.path, name))330 os.mkdir(os.path.join(self.path, name))
331 theme_file = os.path.join(theme_dir, name + u'.xml')331 theme_file = os.path.join(theme_dir, name + u'.xml')
332 outfile = open(theme_file, u'w')332 log.debug(theme_file)
333 outfile.write(theme_xml)333 if os.path.exists(theme_file):
334 outfile.close()334 result = QtGui.QMessageBox.information(
335 if image_from is not None and image_from != image_to:335 self,
336 shutil.copyfile(image_from, image_to)336 translate(u'ThemeManager',u'Theme already exist!'),
337 self.generateAndSaveImage(self.path, name, theme_xml)337 translate(u'ThemeManager',u'This theme name already exist.\n') + \
338 self.loadThemes()338 translate(u'ThemeManager',u'do you want to overwrite it?'),
339 translate(u'ThemeManager',u'Save'),
340 translate(u'ThemeManager',u'Discard'),
341 translate(u'ThemeManager',u'Cancel'),
342 0,
343 2)
344 else:
345 result = 0
346 if result == 0:
347 outfile = open(theme_file, u'w')
348 outfile.write(theme_xml)
349 outfile.close()
350 if image_from is not None and image_from != image_to:
351 shutil.copyfile(image_from, image_to)
352 self.generateAndSaveImage(self.path, name, theme_xml)
353 self.loadThemes()
354 """
355 Case 1, Discard (Only Reload Theme's)
356 """
357 if result == 1:
358 self.loadThemes()
359 """
360 Case 2, Cancel (Back to New Theme Screen)
361 """
362 if result == 2:
363 return False
339364
340 def generateAndSaveImage(self, dir, name, theme_xml):365 def generateAndSaveImage(self, dir, name, theme_xml):
341 log.debug(u'generateAndSaveImage %s %s %s', dir, name, theme_xml)366 log.debug(u'generateAndSaveImage %s %s %s', dir, name, theme_xml)