Merge lp:~suutari-olli/openlp/force-split into lp:openlp

[Azaziah]

- Added "Split" button for Song editor:
  This adds the currently selected Verses tag to
  the given position, thus creating a "Force split"
- Increased Optional Split limit from < 5 to < 51
- Increased test coverage

lp:~suutari-olli/openlp/force-split (revision 2665)
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/1543/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-02-Functional-Tests/1454/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-03-Interface-Tests/1392/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1175/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/765/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05a-Code_Analysis/833/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05b-Test_Coverage/701/

[Tim Bentley (trb143)]

Cannot test as the UI is not visible.
There will be more comments when it works

review: Needs Fixing

[Azaziah (suutari-olli)]

Could you explain "Cannot test as the UI is not visible." ?

I replied to the other comments, thank you for the review.

[Tim Bentley (trb143)]

You have and Insert button which is not visible - simple.

[Azaziah (suutari-olli)]

> You have and Insert button which is not visible - simple.

Did you read my replies to code comments?

"What do you mean? The UI is the new "Split" button found in song editor,
in both "Edit all" and "Edit" modes."

This is visible for me, if it is not visible for you, then something has gone horribly wrong.

[Tim Bentley (trb143)]

Ok found it now as the button is only visible on Edit All.

All the button does is add a ---[Verse X]--- tag which is the same as the splt button does except that the split button has more options.

Why is this request needed because it would seem we have the required functionality already.

[Tomas Groth (tomasgroth)]

I think the idea is to force s split inside a verse - bot correct me if I'm wrong.
If that is the idea, then the button should also be available in single verse edit.

[Azaziah (suutari-olli)]

> Ok found it now as the button is only visible on Edit All.
>
> All the button does is add a ---[Verse X]--- tag which is the same as the splt
> button does except that the split button has more options.
>
> Why is this request needed because it would seem we have the required
> functionality already.

----
> All the button does is add a ---[Verse X]--- tag which is the same as the splt
> button does except that the split button has more options.

Not sure if I understand this sentence, but:

With the "Split" button, the verse will be force splitted by using the verse tag.
This is easier than selecting the verse manually and then inserting it to the required position.
A lot of people are confusing the meaning of Optional split and try to perform force splits with it.
A lot of people have requested easier way to force split verses.

> Ok found it now as the button is only visible on Edit All.

Again: This is visible for me in: Edit, Edit all & Add,
are you sure this button is not visible for you as well?

[Azaziah (suutari-olli)]

> I think the idea is to force s split inside a verse - bot correct me if I'm
> wrong.
> If that is the idea, then the button should also be available in single verse
> edit.

This button is available in single verse edit, as well it is available in "Add" verse.
I believe TRB143 may has missed it or something very weird is happening.

[Tim Bentley (trb143)]

OK now have this working a bit.
The insert only works on the Edit All screen and is not visible on Add or Edit.

Insert adds a ---[Verse-4]--- type tag

We already have split which does the same thing but with greater functionality as it allows one to choose what they want to add.

If Insert adds based on the thing which was last used and increments the count by one.

This is now very confusing.
Split needs to be enhanced with a new option to split the current verse type not add a 2nd implementation.

Looking at the defects they were old and could predate some of the current functionality like virtual split (only used if required).

review: Needs Fixing

[Azaziah (suutari-olli)]

> OK now have this working a bit.
> The insert only works on the Edit All screen and is not visible on Add or
> Edit.
>
> Insert adds a ---[Verse-4]--- type tag
>
> We already have split which does the same thing but with greater functionality
> as it allows one to choose what they want to add.
>
> If Insert adds based on the thing which was last used and increments the count
> by one.
>
> This is now very confusing.
> Split needs to be enhanced with a new option to split the current verse type
> not add a 2nd implementation.
>
>
> Looking at the defects they were old and could predate some of the current
> functionality like virtual split (only used if required).

I think you are confusing "Insert" and "Split"

"Insert" is old functionality, "Split" is new functionality.

"Insert" is available only in "Edit all", "Split" is also available in "Edit" & "Add"

"Insert" adds +1 to the current verse, as it is used to add more verses.
"Insert" allows you to change the verse type you want to insert.

"Split" duplicates the active verses tag, thus creating a force split

[lp:~suutari-olli/openlp/force-split]

2666. By Azaziah on 2016-05-29

- Fixed the issue whre verse current number is not updated properly on edit single verse mode.

2667. By Azaziah on 2016-06-16

Merged trunk?

2668. By Azaziah on 2016-06-16

Merged trunk?

2669. By Azaziah on 2016-06-16

Reverted the perfectly functioning Verse split technigue and replaced it with insert [--].
This still needs to be turned into force split in renderer.py

2670. By Azaziah on 2016-07-02

- This is broken / Going to revert to older solution after this.

2671. By Azaziah on 2016-09-07

- Reverted to the working solution.

Unmerged revisions

2671. By Azaziah on 2016-09-07

- Reverted to the working solution.

2670. By Azaziah on 2016-07-02

- This is broken / Going to revert to older solution after this.

2669. By Azaziah on 2016-06-16

Reverted the perfectly functioning Verse split technigue and replaced it with insert [--].
This still needs to be turned into force split in renderer.py

2668. By Azaziah on 2016-06-16

Merged trunk?

2667. By Azaziah on 2016-06-16

Merged trunk?

2666. By Azaziah on 2016-05-29

- Fixed the issue whre verse current number is not updated properly on edit single verse mode.

2665. By Azaziah on 2016-05-10

Merged trunk (Noticed tests appear to be fixed)

2664. By Azaziah on 2016-05-10

- Improved this on Edit single verse (This now detects if the verse type is changed)

2663. By Azaziah on 2016-05-09

Added a test for def on_insert_button_clicked(self):

2662. By Azaziah on 2016-05-07

Fixed a test

=== modified file 'openlp/core/lib/renderer.py'
--- openlp/core/lib/renderer.py	2016-05-15 17:33:42 +0000
+++ openlp/core/lib/renderer.py	2016-05-29 14:01:07 +0000
@@ -241,7 +241,7 @@
         elif item.is_capable(ItemCapabilities.CanSoftBreak):
             pages = []
             if '[---]' in text:
-                # Remove two or more option slide breaks next to each other (causing infinite loop).
+                # Remove two or more optional splits next to each other (causing infinite loop).
                 while '\n[---]\n[---]\n' in text:
                     text = text.replace('\n[---]\n[---]\n', '\n[---]\n')
                 while ' [---]' in text:
@@ -249,8 +249,8 @@
                 while '[---] ' in text:
                     text = text.replace('[---] ', '[---]')
                 count = 0
-                # only loop 5 times as there will never be more than 5 incorrect logical splits on a single slide.
-                while True and count < 5:
+                # only loop 50 times as there will never be more than 50 optional splits on a single slide.
+                while True and count < 51:
                     slides = text.split('\n[---]\n', 2)
                     # If there are (at least) two occurrences of [---] we use the first two slides (and neglect the last
                     # for now).

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2016-04-25 11:01:32 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2016-05-29 14:01:07 +0000
@@ -776,6 +776,55 @@
                         item = QtWidgets.QTableWidgetItem(entry, 0)
                         item.setData(QtCore.Qt.UserRole, temp_ids[row])
                         self.verse_list_widget.setItem(row, 0, item)
+        # This code converts force splits aka. Verse tags inserted during edit to new verses from text.
+        # It is largely copied from def on_verse_edit_all_button_clicked
+        verse_list = ''
+        if self.verse_list_widget.rowCount() > 0:
+            for row in range(self.verse_list_widget.rowCount()):
+                item = self.verse_list_widget.item(row, 0)
+                field = item.data(QtCore.Qt.UserRole)
+                verse_tag = VerseType.translated_name(field[0])
+                verse_num = field[1:]
+                verse_list += '---[%s:%s]---\n' % (verse_tag, verse_num)
+                verse_list += item.text()
+                verse_list += '\n'
+            self.verse_form.set_verse(verse_list)
+        verse_list = self.verse_form.get_all_verses()
+        verse_list = str(verse_list.replace('\r\n', '\n'))
+        self.verse_list_widget.clear()
+        self.verse_list_widget.setRowCount(0)
+        for row in self.find_verse_split.split(verse_list):
+            for match in row.split('---['):
+                for count, parts in enumerate(match.split(']---\n')):
+                    if count == 0:
+                        if len(parts) == 0:
+                            continue
+                        # handling carefully user inputted versetags
+                        separator = parts.find(':')
+                        if separator >= 0:
+                            verse_name = parts[0:separator].strip()
+                            verse_num = parts[separator + 1:].strip()
+                        else:
+                            verse_name = parts
+                            verse_num = '1'
+                        verse_index = VerseType.from_loose_input(verse_name)
+                        verse_tag = VerseType.tags[verse_index]
+                        # Later we need to handle v1a as well.
+                        regex = re.compile(r'\D*(\d+)\D*')
+                        match = regex.match(verse_num)
+                        if match:
+                            verse_num = match.group(1)
+                        else:
+                            verse_num = '1'
+                        verse_def = '%s%s' % (verse_tag, verse_num)
+                    else:
+                        if parts.endswith('\n'):
+                            parts = parts.rstrip('\n')
+                        item = QtWidgets.QTableWidgetItem(parts)
+                        item.setData(QtCore.Qt.UserRole, verse_def)
+                        self.verse_list_widget.setRowCount(self.verse_list_widget.rowCount() + 1)
+                        self.verse_list_widget.setItem(self.verse_list_widget.rowCount() - 1, 0, item)
+        # Finalize edit
         self.tag_rows()
         # Check if all verse tags are used.
         self.on_verse_order_text_changed(self.verse_order_edit.text())

=== modified file 'openlp/plugins/songs/forms/editversedialog.py'
--- openlp/plugins/songs/forms/editversedialog.py	2015-12-31 22:46:06 +0000
+++ openlp/plugins/songs/forms/editversedialog.py	2016-05-29 14:01:07 +0000
@@ -44,6 +44,10 @@
         self.split_button.setIcon(build_icon(':/general/general_add.png'))
         self.split_button.setObjectName('split_button')
         self.verse_type_layout.addWidget(self.split_button)
+        self.force_split_button = QtWidgets.QPushButton(edit_verse_dialog)
+        self.force_split_button.setIcon(build_icon(':/general/general_add.png'))
+        self.force_split_button.setObjectName('force_split_button')
+        self.verse_type_layout.addWidget(self.force_split_button)
         self.verse_type_label = QtWidgets.QLabel(edit_verse_dialog)
         self.verse_type_label.setObjectName('verse_type_label')
         self.verse_type_layout.addWidget(self.verse_type_label)
@@ -78,6 +82,8 @@
         self.verse_type_combo_box.setItemText(VerseType.Other, VerseType.translated_names[VerseType.Other])
         self.split_button.setText(UiStrings().Split)
         self.split_button.setToolTip(UiStrings().SplitToolTip)
+        self.force_split_button.setText(translate('SongsPlugin.EditVerseForm', '&Split'))
+        self.force_split_button.setToolTip(translate('SongsPlugin.EditVerseForm', 'Split the verse.'))
         self.insert_button.setText(translate('SongsPlugin.EditVerseForm', '&Insert'))
         self.insert_button.setToolTip(translate('SongsPlugin.EditVerseForm',
                                       'Split a slide into two by inserting a verse splitter.'))

=== modified file 'openlp/plugins/songs/forms/editverseform.py'
--- openlp/plugins/songs/forms/editverseform.py	2016-01-09 16:26:14 +0000
+++ openlp/plugins/songs/forms/editverseform.py	2016-05-29 14:01:07 +0000
@@ -46,6 +46,7 @@
         self.has_single_verse = False
         self.insert_button.clicked.connect(self.on_insert_button_clicked)
         self.split_button.clicked.connect(self.on_split_button_clicked)
+        self.force_split_button.clicked.connect(self.on_force_split_button_clicked)
         self.verse_text_edit.cursorPositionChanged.connect(self.on_cursor_position_changed)
         self.verse_type_combo_box.currentIndexChanged.connect(self.on_verse_type_combo_box_changed)
 
@@ -64,7 +65,7 @@
 
     def on_split_button_clicked(self):
         """
-        The split button has been pressed
+        The (optional) split button has been pressed
         """
         text = self.verse_text_edit.toPlainText()
         position = self.verse_text_edit.textCursor().position()
@@ -76,6 +77,39 @@
         self.verse_text_edit.insertPlainText(insert_string)
         self.verse_text_edit.setFocus()
 
+    def on_force_split_button_clicked(self, verse_tag, verse_num=1):
+        """
+        The force split button has been pressed
+        Find the last used verse tag by using given position and regex.
+        Then insert the tag and create a new line if required.
+        """
+        position = self.verse_text_edit.textCursor().position()
+        text = self.verse_text_edit.toPlainText()
+        position = text.rfind('---[', 0, position)
+        # If  editing single verse or ---[ not found, get value of the current verse and insert it.
+        # This will insert the verse tag if editing a single verse or all  verses and no verse tag is present.
+        if self.has_single_verse or position == -1:
+            verse_type_index = self.verse_type_combo_box.currentIndex()
+            self.insert_verse(VerseType.tags[verse_type_index], self.verse_number_box.value())
+        # Find & Duplicate the currently selected verses tag and insert it.
+        else:
+            text = text[position:]
+            position = text.find(']---')
+            if position == -1:
+                return
+            text = text[:position + 4]
+            match = re.match(('---\[.*\]---'), text)
+            insert_string = match.group()
+            # Reset text & position data for checking the proper inserting place & is newline required.
+            text = self.verse_text_edit.toPlainText()
+            position = self.verse_text_edit.textCursor().position()
+            if position and text[position - 1] != '\n':
+                insert_string = '\n' + insert_string
+            if position == len(text) or text[position] != '\n':
+                insert_string += '\n'
+            self.verse_text_edit.insertPlainText(insert_string)
+            self.verse_text_edit.setFocus()
+
     def on_insert_button_clicked(self):
         """
         The insert button has been pressed
@@ -99,13 +133,11 @@
         """
         Adjusts the verse number SpinBox in regard to the selected verse type and the cursor's position.
         """
-        if self.has_single_verse:
-            return
         position = self.verse_text_edit.textCursor().position()
         text = self.verse_text_edit.toPlainText()
         verse_name = VerseType.translated_names[
             self.verse_type_combo_box.currentIndex()]
-        if not text:
+        if not text or self.has_single_verse:
             return
         position = text.rfind('---[%s' % verse_name, 0, position)
         if position == -1:
@@ -115,7 +147,12 @@
         position = text.find(']---')
         if position == -1:
             return
-        text = text[:position + 4]
+        # If editing a Single verse, set the text to match current verse, otherwise increase the suggested verse.
+        # This seems to only take effect when adding a split in middle of the sentence.
+        if self.has_single_verse:
+            text = text[:position]
+        else:
+            text = text[:position + 4]
         match = VERSE_REGEX.match(text)
         if match:
             try:
@@ -153,6 +190,7 @@
     def get_verse(self):
         """
         Extract the verse text
+        If you are looking for the save method, check out songs\forms\editsongform.py def on_verse_edit_button_clicked
 
         :return: The text
         """
@@ -162,6 +200,7 @@
     def get_all_verses(self):
         """
         Extract all the verses
+        If you are looking for the save method, check out forms\editsongform.py def on_verse_edit_all_button_clicked
 
         :return: The text
         """

=== modified file 'tests/functional/openlp_plugins/songs/test_editverseform.py'
--- tests/functional/openlp_plugins/songs/test_editverseform.py	2015-12-31 22:46:06 +0000
+++ tests/functional/openlp_plugins/songs/test_editverseform.py	2016-05-29 14:01:07 +0000
@@ -55,16 +55,29 @@
 
     def update_suggested_verse_number_test(self):
         """
-        Test that update_suggested_verse_number() has no effect when editing a single verse
+        Test that update_suggested_verse_number() does not change the verse number for force split in edit single verse.
         """
         # GIVEN some input values
         self.edit_verse_form.has_single_verse = True
         self.edit_verse_form.verse_type_combo_box.currentIndex = MagicMock(return_value=0)
         self.edit_verse_form.verse_text_edit.toPlainText = MagicMock(return_value='Text')
-        self.edit_verse_form.verse_number_box.setValue(3)
+        self.edit_verse_form.verse_number_box.setValue(1)
 
         # WHEN the method is called
         self.edit_verse_form.update_suggested_verse_number()
 
         # THEN the verse number must not be changed
-        self.assertEqual(3, self.edit_verse_form.verse_number_box.value(), 'The verse number should be 3')
+        self.assertEqual(1, self.edit_verse_form.verse_number_box.value(), 'The verse number should be 1')
+
+    def on_insert_button_clicked_test(self):
+        """
+        Test that insert_verse is called on def on_insert_button_clicked
+        """
+        # GIVEN required functions
+        self.edit_verse_form.insert_verse = MagicMock()
+
+        # WHEN the method is called
+        self.edit_verse_form.on_insert_button_clicked()
+
+        # THEN the verse number must not be changed
+        self.assertEqual(1, self.edit_verse_form.insert_verse.call_count, 'The insert_verse should had been called')