Merge lp:~phill-ridout/openlp/bug-1011286 into lp:openlp

[Phill]

I have actually fixed the entering of verses with no text! I would appreciate extended testing on this, as I have made a change in the renderer code. As far as my testing goes, I have not broken any thing. But you guys have a knack of finding flaws ;-)

The validation is no longer needed, as it was just checking that each verse had some text.

Changed the save button to OK as per the reasons in my email.

[Tim Bentley (trb143)]

Do we need the log.debug?

review: Needs Fixing

[Tim Bentley (trb143)]

review: Approve

[Raoul Snyman (raoul-snyman)]

You should be able to close the dialog via OK without any text in the dialog. No text is also valid.

review: Needs Fixing

[Phill (phill-ridout)]

On Jun 12, 2012 8:06 PM, "Raoul Snyman" <
<email address hidden>> wrote:
>
> Review: Needs Fixing

Can you clarify what you want? The current behaviour is:

Click Add/Edit verse try to close the dialog box by clicking save, to save
with out any text and there is a message box telling the user that there
needs to be text.

Click edit all, and leave it blank a stack trace occurs. Add only one line
and the stack trace occurs.

Save the song with no verses, and a message appears telling you that you
can't do that.

> You should be able to close the dialog via OK without any text in the
dialog. No text is also valid.

My fix fixes the edit all version of the dialog box. The edit (single
verse) is not affected.

If you were to remove all text, then this would delete all verses. And you
wouldn't be able to save the song.

[Tim Bentley (trb143)]

Looks OK but we are in a string freeze so this cannot be merge.

review: Needs Fixing

[Phill (phill-ridout)]

On Jun 16, 2012 9:42 AM, "Tim Bentley" <email address hidden> wrote:
>
> Review: Needs Fixing
>
> Looks OK but we are in a string freeze so this cannot be merge.

I have not changed any strings. One has moved up a couple of lines but I
thought that was neither here nor there.

[Tim Bentley (trb143)]

Needs to put his glasses on!

review: Approve

[Jonathan Corwin (j-corwin)]

If I now Edit All, remove all the text from just one of the verses:
---[Verse:1]---
---[Verse:2]---
blah blah blah

I now don't get the error dialog, but verse 1 also completely disappears when I save it.

We still need that warning for the edit all scenario. We just don't want it to crash if all the text is removed.

review: Needs Fixing

[Phill (phill-ridout)]

On 16 June 2012 13:59, Jonathan Corwin <email address hidden> wrote:
> I now don't get the error dialog, but verse 1 also completely disappears when I save it.

> We still need that warning for the edit all scenario. We just don't want it to crash if all the text is removed.

So its ok that V1 disappears if we still should have a dialogue box?

[Raoul Snyman (raoul-snyman)]

So, apparently what I want is a change in functionality.

review: Approve

[Jonathan Corwin (j-corwin)]

The existing code was preventing empty verses.
---[Verse:1]---
---[Verse:2]---

We still need the code that was preventing it. So V1 won't disappear because the dialog box will prevent the above from being saved.

We don't need a dialog if there is no text at all in the edit all verses window. The user can just save that, and we'll not worry about what strange thing they are up to.

[Jonathan Corwin (j-corwin)]

(Needs Fixing still applies)

[Tim Bentley (trb143)]

review: Needs Resubmitting

[Jonathan Corwin (j-corwin)]

I think line 21 should now be the following to replicate previous behaviour?

if not value[1]:

review: Needs Fixing

[Tim Bentley (trb143)]

Sorry but the saving has become a mess.
Add a song
---[Verse:1]---
---[Verse:2]---

does not allow the song to be saved.

---[Verse:1]---

---[Verse:2]---

with a space in each verse allows the song to be saved but verse 2 disappears.

The loss of verse 2 even with a space in it is not correct as this is how I would add a blank verse are the end of the song.

review: Needs Fixing

[Jonathan Corwin (j-corwin)]

Tim, I just tried your second example, and verse 2 doesn't disappear for me. I don't see how Phill's changes could affect this behaviour.

[Phill (phill-ridout)]

Hrm, It removes the last verse if there is only one char after the new
line. It can be any char it appears!

Although, technically this has nothing to do with my code, my code is
doing what it should do. This is caused by the code that creates the
list items. I had a look at it, but its too far over my head.

I'm not sure what you want to do, but really this is a separate bug.

Phill

[Tim Bentley (trb143)]

All verses must have 1 character in them
The last verse must have 2 characters in it to be preserved.

This is strange behaviour but if this is an existing bug then that needs to be looked at under a new bug.

review: Approve

[Phill (phill-ridout)]

> All verses must have 1 character in them
> The last verse must have 2 characters in it to be preserved.
>
> This is strange behaviour but if this is an existing bug then that needs to be looked at under a new bug.

Both these issues have now been fixed in my latest resubmit

[Jonathan Corwin (j-corwin)]

Line 9, change to:
   if previous_raw is not None:

Line 22:
   Rather than have a if count == 0 /and/ len(... just move the len() part of the test into the if on line 24. Also we can't have negative lengths, so change the len() test to == 0, not <= 0.

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

I just want to check, is this proposal waiting for post-RC2?

[Phill (phill-ridout)]

No, not if you are happy with it.
On Jul 27, 2012 12:10 AM, "Raoul Snyman" <
<email address hidden>> wrote:

> I just want to check, is this proposal waiting for post-RC2?
> --
> https://code.launchpad.net/~phill-ridout/openlp/bug-1011286/+merge/116168
> You are the owner of lp:~phill-ridout/openlp/bug-1011286.
>

[Tim Bentley (trb143)]

I have not had time this week to review this due to pressure.
It is too late to get this in and tested before the freeze.
This needs to be reviewed and signed off by two Core's !

[Phill (phill-ridout)]

> I have not had time this week to review this due to pressure.
> It is too late to get this in and tested before the freeze.
> This needs to be reviewed and signed off by two Core's !

Thats fine, although I've done my testing, I would rather it was thourghly
tested. Due to the change to the render it has the potential to screw the
main display up.

[Raoul Snyman (raoul-snyman)]

Yes, let's play it safe.

[Raoul Snyman (raoul-snyman)]

I am happy with this, let's get it in.

review: Approve

[Tim Bentley (trb143)]

looks fine to me to.

review: Approve

[Andreas Preikschat (googol-deactivatedaccount)]

I am not sure about line 9. I do not see a place where previous_raw becomes None.

My question: Why did you change the line? In what circumstances does this fix or prevent something?

Currently I believe that this check is not needed any longer (because it is always true). Just check the code yourself: previous_raw is set to u'' or is build from strings, but never set to None.

review: Needs Information

[Phill (phill-ridout)]

Any chance of getting this in before the next freeze?

[Raoul Snyman (raoul-snyman)]

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/core/lib/renderer.py'
--- openlp/core/lib/renderer.py	2012-07-02 18:45:03 +0000
+++ openlp/core/lib/renderer.py	2012-08-13 19:34:21 +0000
@@ -450,8 +450,7 @@
                 previous_html, previous_raw, html_lines, lines, separator, u'')
         else:
             previous_raw = separator.join(lines)
-        if previous_raw:
-            formatted.append(previous_raw)
+        formatted.append(previous_raw)
         log.debug(u'_paginate_slide - End')
         return formatted
 

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2012-07-07 14:54:14 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2012-08-13 19:34:21 +0000
@@ -528,9 +528,9 @@
         for row in self.findVerseSplit.split(verse_list):
             for match in row.split(u'---['):
                 for count, parts in enumerate(match.split(u']---\n')):
-                    if len(parts) <= 1:
-                        continue
                     if count == 0:
+                        if len(parts) == 0:
+                            continue
                         # handling carefully user inputted versetags
                         separator = parts.find(u':')
                         if separator >= 0:

=== modified file 'openlp/plugins/songs/forms/editversedialog.py'
--- openlp/plugins/songs/forms/editversedialog.py	2012-06-22 14:14:53 +0000
+++ openlp/plugins/songs/forms/editversedialog.py	2012-08-13 19:34:21 +0000
@@ -67,7 +67,7 @@
         self.verseTypeLayout.addStretch()
         self.dialogLayout.addLayout(self.verseTypeLayout)
         self.buttonBox = create_button_box(editVerseDialog, u'buttonBox',
-            [u'cancel', u'save'])
+            [u'cancel', u'ok'])
         self.dialogLayout.addWidget(self.buttonBox)
         self.retranslateUi(editVerseDialog)
 

=== modified file 'openlp/plugins/songs/forms/editverseform.py'
--- openlp/plugins/songs/forms/editverseform.py	2012-06-22 14:14:53 +0000
+++ openlp/plugins/songs/forms/editverseform.py	2012-08-13 19:34:21 +0000
@@ -185,22 +185,3 @@
             text = u'---[%s:1]---\n%s' % \
                 (VerseType.TranslatedNames[VerseType.Verse], text)
         return text
-
-    def accept(self):
-        if self.hasSingleVerse:
-            value = unicode(self.getVerse()[0])
-        else:
-            log.debug(unicode(self.getVerse()[0]).split(u'\n'))
-            value = unicode(self.getVerse()[0]).split(u'\n')[1]
-            if not value:
-                lines = unicode(self.getVerse()[0]).split(u'\n')
-                index = 2
-                while index < len(lines) and not value:
-                    value = lines[index]
-                    index += 1
-        if not value:
-            critical_error_message_box(
-                message=translate('SongsPlugin.EditSongForm',
-                'You need to type some text in to the verse.'))
-            return False
-        QtGui.QDialog.accept(self)