Merge lp://qastaging/~orangeshirt/openlp/bibles into lp://qastaging/openlp

I've not read each single line, but the code looks nice.
There are some small things (spaces and u''s) you've missed at a lot of places

184 Greate, that you're already using it ;)
207 book name (with space)
211, 214 Combo box items shouls be title case
355, 175, 210, 215, 309, 468 spaces missing
357, 389, 449, 451 u'' missing
314, 320 not sure about capitalisation

What is this?
285 + # can this be automated?
286 + self.width = 400

"self.width" is not used. Other than that you can access width() and height of the dialog after setupUi(). Width would be 600 in that case.

153: it should be bookNameWidget

159ff: QFormLayout::addRow ( QWidget * label, QWidget * field )

178ff: The spacer takes place, if the scrollArea is visible. use: addStretch()

317f, 330ff: What is that supposed to do?

357: White is the wrong color. It does not work on dark themes. Implement something like:
... color=None):
pal = QtGui.QPalette(self.palette())
if color:
    pal.setColor(QtGui.QPalette.Base,QtGui.QColor(color))

495: return self.save_object(book)

777, 789: if bible:

838, 843: shift linebreaks

637, 743, 761, 811: I personally prefere "is None" as you expect a instance of None

> 317f, 330ff: What is that supposed to do?

330ff.: If a bible doesn't contain all books it removes all unnecessary ones, so that the user only see bookname that could be changed.
317f: If it is a webbible the hole scrollarea is obsolete and removed, because it is not possible to change the booknames.

> 314, 320 not sure about capitalisation
I think it is ok, because you recommand me to write book name (with space) then I think it is bookNameNotice and not booknameNotice

> 330ff.: If a bible doesn't contain all books it removes all unnecessary ones, so that the user only see bookname that could be changed.
> 317f: If it is a webbible the hole scrollarea is obsolete and removed, because it is not possible to change the booknames.

Well, I understood that so far. But you could just hide the widgets. I see no advantage and the reassigning the parent adds complexity and even some (tiny) more things to process.

> 314, 320 not sure about capitalisation
I think it is ok, because you recommand me to write book name (with space) then I think it is bookNameNotice and not booknameNotice

I thought about the strings, not the names. e.g.: "It is not possible to customize the Book Names."

148, 161: delete

Just realized your strings arround the combo box:
General Settings > Global Settings
The tooltip: remove it. The options should be self-explaining (even if not, users will hardly discover the tooltip). see also pad.lv/960386

But: Users do not know, what is the effect of book names languages. Find a better label or add a explanation text label, that the setting affects the reference search. Are the custom names shown in the slide footer lines?
Maybe discuss the strings with others on IRC. I'm now offline for the next five days.

You have a conflict in openlp/plugins/bibles/forms/__init__.py

Please fix this conflict and resubmit.

I'm not sure the Bible Editor window needs to be quite as wide as it is. It could be closer to where the text at the top of the Custom Book Names tab wraps perhaps?

The red backgrounds for errors are pretty, but non-standard, please leave them white.
(If you want to introduce a new UI standard it needs to be discussed on the mailing list, and then if agreed, introduced everywhere at the same time)

Case consistency with the global settings page:
212: -> Global settings
215: -> Bible language
218: -> Application language

316: For consistency: "webbible" -> "Web Download Bible"

321/322: -> 'To use the customized book names, "Bible language" must be selected on the Meta Data tab or, if "Global settings" is selected, on the Bible page in Configure OpenLP.'

359/367: Could you just return at these places, and get rid of the save variable?

439: "Decimal digits only could" -> "Numbers can only"
441: "non-digit" -> "non-numeric"

456: Book Name Exists Twice -> Duplicate Book Name
458: 'The Book Name "%s" exists twice. Please change one.' -> 'The Book Name "%s" has been entered more than once."

> The red backgrounds for errors are pretty, but non-standard, please leave them
> white.
> (If you want to introduce a new UI standard it needs to be discussed on the
> mailing list, and then if agreed, introduced everywhere at the same time)

Oh - I did not thought about the UI standard in that moment. It was not my intention to introduce a new standard, it was only an idea to find errors faster. I'll remove.

Thanks for all other hints. I'll adapt that.

>
> Oh - I did not thought about the UI standard in that moment. It was not my
> intention to introduce a new standard, it was only an idea to find errors
> faster. I'll remove.
>
Thanks. Yes I could see the benefit, but since we have other places where fields are mandatory (e.g. song editor, Bible import, etc as well as others I'm sure) it would lead to inconsistency. Also we need to consider that there are some people who use different desktop themes and so if there was some strange user who liked to use a red theme with red text, we'd need to be sure it still worked OK for them :)

Looks good to me.

Combo Box strings are title case:
http://wiki.openlp.org/Development:String_Standards

And you could replace the other occurence of .removeWidget() && .setParent(None) with .hide() as well.

Following on from Meinert said then, please could you fix the case of the corresponding combo box on the Bible settings page too. Thanks.

> Combo Box strings are title case:
> http://wiki.openlp.org/Development:String_Standards

I'll fix this.
>
> And you could replace the other occurence of .removeWidget() &&
> .setParent(None) with .hide() as well.

No I couldn't.
Otherwise if I have a bible which only contain the New Testament, there will be much space before the first Label & LineEdit is visible.
And at the End the same. If the bible doesn't contain apocrypha.
I tried it before you ask me to change it and after.
That's why I use hide only for scrollArea.
If there is an other possibility please tell me.

> No I couldn't.
> Otherwise if I have a bible which only contain the New Testament, there will
> be much space before the first Label & LineEdit is visible.
> And at the End the same. If the bible doesn't contain apocrypha.
> I tried it before you ask me to change it and after.
> That's why I use hide only for scrollArea.
> If there is an other possibility please tell me.

Oh, I didn't knew, that there is this bug in the QFormLayout (other Layouts aren't doing that).

One solution would be: self.bookNameWidgetLayout.setVerticalSpacing(0) . For your list I don't think, that there is any advantage of the additional space assigned by the layout.

In case you prefere the lavish layout, you there are two possible ways: You can just replace the .setParent(None) with .hide(). The other option would be to keep .setParent(None). In that case you don't need the .removeWidget() and I prefere an additional small comment.

line 344 extra space
353,354,355 these can be local variables and not class variables
368 and 369 should be before 360 and the "if not self.webbible" optomised
validateMetaData should use local variables and get them passed as args

604 and 627 should request list of bibles each time and not save the list!

662 should be None not -1. None is missing -1 is a value this will confuse later. It may need to be handled in a different place in the code.

> 368 and 369 should be before 360
Two Questions:
1. Why only before 360 (book validation) and not before 358 (meta validation)?
2. Is this really necessary, the Validation needs only some milliseconds - saving needs much time, if many values was changed. If I switch the cursor to busy before the validation i have to but a "cursor normal" at every possibility where validation could show a warning (417,424,436 and maybe 387,393,402), otherwise the cursor shows busy as long as the users push the ok button of the warning message.
I prefer to leave the lines where they are at the moment. Please tell if I really should change this.

> 662 should be None not -1. None is missing -1 is a value this will confuse
> later. It may need to be handled in a different place in the code.
-1 is the value which was saved at the moment if the user choose "Global Settings" (compare line 213 respectivly 356f.) that's why I use it in line 662.
It is no problem to change it in line 662 to None. My question is:
Should I use None instead of -1 at the other points too?

1) fine you seem to get the drift.
2) you have the same if twice and should that not be optimised.

As regards to the -1 have a look at find_and_set_in_combo_box

> 1) fine you seem to get the drift.
> 2) you have the same if twice and should that not be optimised.

After disscusing a while with Raoul in irc we decided to leave "cursor busy" and the twice if.
You could show me a better solution. But at the moment I have no other solution.