openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

126	+ 'Warning - New data directory location contains OpenLP '
127	+ 'data files. These files WILL be replaced during a copy.'))

I recommend making that "Warning" all upper case, and using a colon instead of a dash:

    "WARNING: ..."


140	+ 'Data directory error - Reset to default?'),

If that's the title of the error dialog, it needs to conform to the string standards. I recommend dropping the question and just using the brief description of the problem:

    "Data Directory Error"


157	+ data_path = AppLocation.set_default_data_path()

"set" sounds like you're setting something, but you don't actually pass a value into that method. Did you mean "get"? If not, please adjust your method to pass in the required variable.


158	+ print AppLocation.IsDefaultDataPath

No print statements! Use log.debug(...).


Please change your browse button to conform to the standard everywhere else: a simple pushbutton which only contains the folder icon and is to the right of the field it will be filling. You can use a QHBoxLayout for this purpose.


205	+ translate('OpenLP.AdvancedTab', 'Change data directory?'),

Once again, you need to conform to the string standards. A better dialog title would be:

    "Confirm Data Directory"


Again...

233	+ translate('OpenLP.AdvancedTab', 'Reset data directory to default?'),

    "Reset Data Directory"


263	+ translate('OpenLP.AdvancedTab', 'Replace existing data?'),

"Overwrite" is a better word. Once again, please use the proper string convention:

    "Overwrite Existing Data"


373	+ translate('OpenLP.MainWindow', 'New data directory error'),

    "Data Directory Error"


437	+ path = unicode(QtCore.QSettings().value(
438	+ u'advanced/data path', QtCore.QVariant(u'none')).toString())
439	+ if path == u'none':

Why set it to "none"? Why not just remove the setting completely with QSettings.remove(), then just check it with QSettings.contains().


As I said before,

"""
AppLocation.IsDefaultDataPath should not be a class-level variable. Class-level "variables" are used as enumerations, and should never be altered programmatically. You should create a method instead, called "AppLocation.is_default_data_path()".
"""

In other words, please remove that "IsDefaultDataPath" or rename it to a member variable name according to our naming conventions (like "is_default_data_path").

My preference would be to remove it completely, and create a static function called "is_default_data_path()" which looks like this:

    @staticmethod
    def is_default_data_path():
        return QSettings().contains("advanced/data path")


-- 
https://code.launchpad.net/~smpettit/openlp/data-path/+merge/104288
Your team OpenLP Core is subscribed to branch lp:openlp.