openlp-core team mailing list archive

[Meinert Jordan <meinertjordan@xxxxxxxxxx>]

> I think we could/should append something like openlpdata.  If we don't append
> something, the data could be put into a "general" directory intermixed with
> other sub-folders. This would "complicate" subsequent data directory changes
> or possibly impact performance of OpenLP.

Well, I see your point. It could be sensible to restrict users fantasy in directory naming. In this case you should provide some more meaningfull name like "OpenLP data files" for example. That way people know more exactly, what is in it. (later we can even translate this name)
In this case most users will propably select the "OpenLP" or "OpenLP data files" folder instead of the root folder, when they want to switch to a existing data set. Therefore you have to detect this case and do not add another subfolder.

> > [...] Never mention the full path.[...]
> Good point.

Keep the "OpenLP data file" or so folder visible. So people know, that they don't need to create a own subfolder. Of cause you should hide the 'data' folder.

> I'll work on that.  Still new to GUI coding.

Everyone starts this way. Just keep close to the mentioned code in the same file.

> > Changes should be applied in the save() method. If I press cancel in the
> > dialog, I don't want the settings to be applied.
> 
> In my first proposal on this I was asked to move the change directory code out
> of the settings manager and put it in mainwindow to execute after everything
> has been "shutdown".  There is a "Cancel data directory change" button.  Also,
> if the user Cancels the dialog, the change is not applied (Lines 410-411)

I haven't read your code. I just run it and therefore I realized, that after pressing cancle and reopening the settings dialog the changes were still present. If you do not write any configuration, than you have a problem with initializing values while starting the config dialog.

> > You show lots of dialogs. Think about, which are necessary. You can maybe
> > merge some of them. For example: Don't show two dialogs at program end, but
> > merge them to one.
> 
> I added a dialog stating the change is about to be made in case the data is
> being moved to removable media.  This can take a bit of time so telling the
> user that something "not normal" is going to happen, and whether it was
> successful or not, was something I thought was needed.

In a first dialog you may hint the user at the fact, that the location gets changed (and report that copying is perform now). That's it. Afterwards you can limit information to error messages. People will realize when OpenLP terminated and we don't have to report, that the program worked as expected.

I went now through your code and have some remarks:

49: There are different icon for revert and default: http://goo.gl/HSJWa ATM. there is just one in OpenLP.

54-57: should move to load(). In general: Each GUI property which you change should be initialized in load(); (e.g. QEditLine.setText(<get from somewhere>))

67f, 79f, 82f : use clicked(). That way the action is triggered after a complete click (normal behavior) and not in case you keep the button pressed. In this regard the code of the existing buttons is wrong as well.

157, 293: The print statement should be removed.

158-161: self.dataDirectoryDefaultButton.setVisible(not AppLocation.IsDefaultDataPath)

194,221f,288f: shift this to save()

243, 350, 352, 374: needs to be unicode()

257: test for existence of u'data'.

272-275: self.dataDirectoryCopyCheckBox.setChecked(answer == QtGui.QMessageBox.Yes)

285:  self.newDataDirectoryEdit.clear()

294-297: self.dataDirectoryDefaultButton.setVisible(not AppLocation.IsDefaultDataPath)

338: QtCore.QSettings().contains(u'advanced/new data path')
    better to move it to the changeDataDirectory() method: if not ...: return

353, 356: You do not need to give the default

354, 378: do all remove calls in one place

357: Your default value QVariant(u'none').toBool is True!!! Just remove it and it becomes False.

436-368: No default value, if path == u''

I hope my comments are more motivating then annoying. You are working at a piece with much user interaction and high risc of of data loss if the user does things wrong.
-- 
https://code.launchpad.net/~smpettit/openlp/data-path/+merge/89612
Your team OpenLP Core is requested to review the proposed merge of lp:~smpettit/openlp/data-path into lp:openlp.