openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

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

Diff comments:

> === modified file 'openlp/core/lib/renderer.py'
> --- openlp/core/lib/renderer.py	2016-01-23 12:38:08 +0000
> +++ openlp/core/lib/renderer.py	2016-05-09 19:57:28 +0000
> @@ -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:

No this was set to balance sense and usability but moving from 5 to 50 is not a valid change.

>                      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-09 19:57:28 +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

Copying code is not good practice so it needs to be refactored to common code if possible.
Do not understand what it is trying to do and the lack of UI does not help.

> +        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-09 19:57:28 +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)

No visible on UI so cannot test.

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


-- 
https://code.launchpad.net/~suutari-olli/openlp/force-split/+merge/294159
Your team OpenLP Core is subscribed to branch lp:openlp.