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