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

Proposed by Azaziah
Status: Superseded
Proposed branch: lp:~suutari-olli/openlp/force-split
Merge into: lp:openlp
Diff against target: 250 lines (+118/-11)
5 files modified
openlp/core/lib/renderer.py (+3/-3)
openlp/plugins/songs/forms/editsongform.py (+49/-0)
openlp/plugins/songs/forms/editversedialog.py (+6/-0)
openlp/plugins/songs/forms/editverseform.py (+44/-5)
tests/functional/openlp_plugins/songs/test_editverseform.py (+16/-3)
To merge this branch: bzr merge lp:~suutari-olli/openlp/force-split
Reviewer Review Type Date Requested Status
Tim Bentley Needs Fixing
Review via email: mp+294228@code.launchpad.net

This proposal supersedes a proposal from 2016-05-10.

Description of the change

- 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/

To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

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

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

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> 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.

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> 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?

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> 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.

Revision history for this message
Tim Bentley (trb143) wrote :

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
Revision history for this message
Azaziah (suutari-olli) wrote :

> 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 updated
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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openlp/core/lib/renderer.py'
2--- openlp/core/lib/renderer.py 2016-05-15 17:33:42 +0000
3+++ openlp/core/lib/renderer.py 2016-05-29 14:01:07 +0000
4@@ -241,7 +241,7 @@
5 elif item.is_capable(ItemCapabilities.CanSoftBreak):
6 pages = []
7 if '[---]' in text:
8- # Remove two or more option slide breaks next to each other (causing infinite loop).
9+ # Remove two or more optional splits next to each other (causing infinite loop).
10 while '\n[---]\n[---]\n' in text:
11 text = text.replace('\n[---]\n[---]\n', '\n[---]\n')
12 while ' [---]' in text:
13@@ -249,8 +249,8 @@
14 while '[---] ' in text:
15 text = text.replace('[---] ', '[---]')
16 count = 0
17- # only loop 5 times as there will never be more than 5 incorrect logical splits on a single slide.
18- while True and count < 5:
19+ # only loop 50 times as there will never be more than 50 optional splits on a single slide.
20+ while True and count < 51:
21 slides = text.split('\n[---]\n', 2)
22 # If there are (at least) two occurrences of [---] we use the first two slides (and neglect the last
23 # for now).
24
25=== modified file 'openlp/plugins/songs/forms/editsongform.py'
26--- openlp/plugins/songs/forms/editsongform.py 2016-04-25 11:01:32 +0000
27+++ openlp/plugins/songs/forms/editsongform.py 2016-05-29 14:01:07 +0000
28@@ -776,6 +776,55 @@
29 item = QtWidgets.QTableWidgetItem(entry, 0)
30 item.setData(QtCore.Qt.UserRole, temp_ids[row])
31 self.verse_list_widget.setItem(row, 0, item)
32+ # This code converts force splits aka. Verse tags inserted during edit to new verses from text.
33+ # It is largely copied from def on_verse_edit_all_button_clicked
34+ verse_list = ''
35+ if self.verse_list_widget.rowCount() > 0:
36+ for row in range(self.verse_list_widget.rowCount()):
37+ item = self.verse_list_widget.item(row, 0)
38+ field = item.data(QtCore.Qt.UserRole)
39+ verse_tag = VerseType.translated_name(field[0])
40+ verse_num = field[1:]
41+ verse_list += '---[%s:%s]---\n' % (verse_tag, verse_num)
42+ verse_list += item.text()
43+ verse_list += '\n'
44+ self.verse_form.set_verse(verse_list)
45+ verse_list = self.verse_form.get_all_verses()
46+ verse_list = str(verse_list.replace('\r\n', '\n'))
47+ self.verse_list_widget.clear()
48+ self.verse_list_widget.setRowCount(0)
49+ for row in self.find_verse_split.split(verse_list):
50+ for match in row.split('---['):
51+ for count, parts in enumerate(match.split(']---\n')):
52+ if count == 0:
53+ if len(parts) == 0:
54+ continue
55+ # handling carefully user inputted versetags
56+ separator = parts.find(':')
57+ if separator >= 0:
58+ verse_name = parts[0:separator].strip()
59+ verse_num = parts[separator + 1:].strip()
60+ else:
61+ verse_name = parts
62+ verse_num = '1'
63+ verse_index = VerseType.from_loose_input(verse_name)
64+ verse_tag = VerseType.tags[verse_index]
65+ # Later we need to handle v1a as well.
66+ regex = re.compile(r'\D*(\d+)\D*')
67+ match = regex.match(verse_num)
68+ if match:
69+ verse_num = match.group(1)
70+ else:
71+ verse_num = '1'
72+ verse_def = '%s%s' % (verse_tag, verse_num)
73+ else:
74+ if parts.endswith('\n'):
75+ parts = parts.rstrip('\n')
76+ item = QtWidgets.QTableWidgetItem(parts)
77+ item.setData(QtCore.Qt.UserRole, verse_def)
78+ self.verse_list_widget.setRowCount(self.verse_list_widget.rowCount() + 1)
79+ self.verse_list_widget.setItem(self.verse_list_widget.rowCount() - 1, 0, item)
80+ # Finalize edit
81 self.tag_rows()
82 # Check if all verse tags are used.
83 self.on_verse_order_text_changed(self.verse_order_edit.text())
84
85=== modified file 'openlp/plugins/songs/forms/editversedialog.py'
86--- openlp/plugins/songs/forms/editversedialog.py 2015-12-31 22:46:06 +0000
87+++ openlp/plugins/songs/forms/editversedialog.py 2016-05-29 14:01:07 +0000
88@@ -44,6 +44,10 @@
89 self.split_button.setIcon(build_icon(':/general/general_add.png'))
90 self.split_button.setObjectName('split_button')
91 self.verse_type_layout.addWidget(self.split_button)
92+ self.force_split_button = QtWidgets.QPushButton(edit_verse_dialog)
93+ self.force_split_button.setIcon(build_icon(':/general/general_add.png'))
94+ self.force_split_button.setObjectName('force_split_button')
95+ self.verse_type_layout.addWidget(self.force_split_button)
96 self.verse_type_label = QtWidgets.QLabel(edit_verse_dialog)
97 self.verse_type_label.setObjectName('verse_type_label')
98 self.verse_type_layout.addWidget(self.verse_type_label)
99@@ -78,6 +82,8 @@
100 self.verse_type_combo_box.setItemText(VerseType.Other, VerseType.translated_names[VerseType.Other])
101 self.split_button.setText(UiStrings().Split)
102 self.split_button.setToolTip(UiStrings().SplitToolTip)
103+ self.force_split_button.setText(translate('SongsPlugin.EditVerseForm', '&Split'))
104+ self.force_split_button.setToolTip(translate('SongsPlugin.EditVerseForm', 'Split the verse.'))
105 self.insert_button.setText(translate('SongsPlugin.EditVerseForm', '&Insert'))
106 self.insert_button.setToolTip(translate('SongsPlugin.EditVerseForm',
107 'Split a slide into two by inserting a verse splitter.'))
108
109=== modified file 'openlp/plugins/songs/forms/editverseform.py'
110--- openlp/plugins/songs/forms/editverseform.py 2016-01-09 16:26:14 +0000
111+++ openlp/plugins/songs/forms/editverseform.py 2016-05-29 14:01:07 +0000
112@@ -46,6 +46,7 @@
113 self.has_single_verse = False
114 self.insert_button.clicked.connect(self.on_insert_button_clicked)
115 self.split_button.clicked.connect(self.on_split_button_clicked)
116+ self.force_split_button.clicked.connect(self.on_force_split_button_clicked)
117 self.verse_text_edit.cursorPositionChanged.connect(self.on_cursor_position_changed)
118 self.verse_type_combo_box.currentIndexChanged.connect(self.on_verse_type_combo_box_changed)
119
120@@ -64,7 +65,7 @@
121
122 def on_split_button_clicked(self):
123 """
124- The split button has been pressed
125+ The (optional) split button has been pressed
126 """
127 text = self.verse_text_edit.toPlainText()
128 position = self.verse_text_edit.textCursor().position()
129@@ -76,6 +77,39 @@
130 self.verse_text_edit.insertPlainText(insert_string)
131 self.verse_text_edit.setFocus()
132
133+ def on_force_split_button_clicked(self, verse_tag, verse_num=1):
134+ """
135+ The force split button has been pressed
136+ Find the last used verse tag by using given position and regex.
137+ Then insert the tag and create a new line if required.
138+ """
139+ position = self.verse_text_edit.textCursor().position()
140+ text = self.verse_text_edit.toPlainText()
141+ position = text.rfind('---[', 0, position)
142+ # If editing single verse or ---[ not found, get value of the current verse and insert it.
143+ # This will insert the verse tag if editing a single verse or all verses and no verse tag is present.
144+ if self.has_single_verse or position == -1:
145+ verse_type_index = self.verse_type_combo_box.currentIndex()
146+ self.insert_verse(VerseType.tags[verse_type_index], self.verse_number_box.value())
147+ # Find & Duplicate the currently selected verses tag and insert it.
148+ else:
149+ text = text[position:]
150+ position = text.find(']---')
151+ if position == -1:
152+ return
153+ text = text[:position + 4]
154+ match = re.match(('---\[.*\]---'), text)
155+ insert_string = match.group()
156+ # Reset text & position data for checking the proper inserting place & is newline required.
157+ text = self.verse_text_edit.toPlainText()
158+ position = self.verse_text_edit.textCursor().position()
159+ if position and text[position - 1] != '\n':
160+ insert_string = '\n' + insert_string
161+ if position == len(text) or text[position] != '\n':
162+ insert_string += '\n'
163+ self.verse_text_edit.insertPlainText(insert_string)
164+ self.verse_text_edit.setFocus()
165+
166 def on_insert_button_clicked(self):
167 """
168 The insert button has been pressed
169@@ -99,13 +133,11 @@
170 """
171 Adjusts the verse number SpinBox in regard to the selected verse type and the cursor's position.
172 """
173- if self.has_single_verse:
174- return
175 position = self.verse_text_edit.textCursor().position()
176 text = self.verse_text_edit.toPlainText()
177 verse_name = VerseType.translated_names[
178 self.verse_type_combo_box.currentIndex()]
179- if not text:
180+ if not text or self.has_single_verse:
181 return
182 position = text.rfind('---[%s' % verse_name, 0, position)
183 if position == -1:
184@@ -115,7 +147,12 @@
185 position = text.find(']---')
186 if position == -1:
187 return
188- text = text[:position + 4]
189+ # If editing a Single verse, set the text to match current verse, otherwise increase the suggested verse.
190+ # This seems to only take effect when adding a split in middle of the sentence.
191+ if self.has_single_verse:
192+ text = text[:position]
193+ else:
194+ text = text[:position + 4]
195 match = VERSE_REGEX.match(text)
196 if match:
197 try:
198@@ -153,6 +190,7 @@
199 def get_verse(self):
200 """
201 Extract the verse text
202+ If you are looking for the save method, check out songs\forms\editsongform.py def on_verse_edit_button_clicked
203
204 :return: The text
205 """
206@@ -162,6 +200,7 @@
207 def get_all_verses(self):
208 """
209 Extract all the verses
210+ If you are looking for the save method, check out forms\editsongform.py def on_verse_edit_all_button_clicked
211
212 :return: The text
213 """
214
215=== modified file 'tests/functional/openlp_plugins/songs/test_editverseform.py'
216--- tests/functional/openlp_plugins/songs/test_editverseform.py 2015-12-31 22:46:06 +0000
217+++ tests/functional/openlp_plugins/songs/test_editverseform.py 2016-05-29 14:01:07 +0000
218@@ -55,16 +55,29 @@
219
220 def update_suggested_verse_number_test(self):
221 """
222- Test that update_suggested_verse_number() has no effect when editing a single verse
223+ Test that update_suggested_verse_number() does not change the verse number for force split in edit single verse.
224 """
225 # GIVEN some input values
226 self.edit_verse_form.has_single_verse = True
227 self.edit_verse_form.verse_type_combo_box.currentIndex = MagicMock(return_value=0)
228 self.edit_verse_form.verse_text_edit.toPlainText = MagicMock(return_value='Text')
229- self.edit_verse_form.verse_number_box.setValue(3)
230+ self.edit_verse_form.verse_number_box.setValue(1)
231
232 # WHEN the method is called
233 self.edit_verse_form.update_suggested_verse_number()
234
235 # THEN the verse number must not be changed
236- self.assertEqual(3, self.edit_verse_form.verse_number_box.value(), 'The verse number should be 3')
237+ self.assertEqual(1, self.edit_verse_form.verse_number_box.value(), 'The verse number should be 1')
238+
239+ def on_insert_button_clicked_test(self):
240+ """
241+ Test that insert_verse is called on def on_insert_button_clicked
242+ """
243+ # GIVEN required functions
244+ self.edit_verse_form.insert_verse = MagicMock()
245+
246+ # WHEN the method is called
247+ self.edit_verse_form.on_insert_button_clicked()
248+
249+ # THEN the verse number must not be changed
250+ self.assertEqual(1, self.edit_verse_form.insert_verse.call_count, 'The insert_verse should had been called')