Bible media item refactors.

Added the ability to sort the 'select' aka 'advanced' bible books combo box alphabetically. (note resources will need regen to show the button icon, I'll submit this in another merge proposal to keep this one a tiny bit cleaner.
Lots of Tests!

bzr-revno: 2722

openlp/core/common/settings.py

@@ -217,7 +217,9 @@ class Settings(QtCore.QSettings):
         ('advanced/default image', 'core/logo file', []),  # Default image renamed + moved to general after 2.4.
         ('shortcuts/escapeItem', 'shortcuts/desktopScreenEnable', []),  # Escape item was removed in 2.6.
         ('shortcuts/offlineHelpItem', 'shortcuts/userManualItem', []),  # Online and Offline help were combined in 2.6.
-        ('shortcuts/onlineHelpItem', 'shortcuts/userManualItem', [])  # Online and Offline help were combined in 2.6.
+        ('shortcuts/onlineHelpItem', 'shortcuts/userManualItem', []),  # Online and Offline help were combined in 2.6.
+        ('bibles/advanced bible', '', []),  # Common bible search widgets combined in 2.6
+        ('bibles/quick bible', 'bibles/primary bible', [])  # Common bible search widgets combined in 2.6
     ]
 
     @staticmethod

openlp/core/lib/mediamanageritem.py

@@ -197,14 +197,9 @@ class MediaManagerItem(QtWidgets.QWidget, RegistryProperties):
         """
         # Add the List widget
         self.list_view = ListWidgetWithDnD(self, self.plugin.name)
-        self.list_view.setSpacing(1)
-        self.list_view.setSelectionMode(QtWidgets.QAbstractItemView.ExtendedSelection)
-        self.list_view.setAlternatingRowColors(True)
         self.list_view.setObjectName('{name}ListView'.format(name=self.plugin.name))
         # Add to page_layout
         self.page_layout.addWidget(self.list_view)
-        # define and add the context menu
-        self.list_view.setContextMenuPolicy(QtCore.Qt.CustomContextMenu)
         if self.has_edit_icon:
             create_widget_action(self.list_view,
                                  text=self.plugin.get_string(StringContent.Edit)['title'],

openlp/core/ui/lib/listwidgetwithdnd.py

@@ -40,6 +40,11 @@ class ListWidgetWithDnD(QtWidgets.QListWidget):
         super().__init__(parent)
         self.mime_data_text = name
         self.no_results_text = UiStrings().NoResults
+        self.setSpacing(1)
+        self.setSelectionMode(QtWidgets.QAbstractItemView.ExtendedSelection)
+        self.setAlternatingRowColors(True)
+        self.setContextMenuPolicy(QtCore.Qt.CustomContextMenu)
+        self.locked = False
 
     def activateDnD(self):
         """
@@ -49,13 +54,15 @@ class ListWidgetWithDnD(QtWidgets.QListWidget):
         self.setDragDropMode(QtWidgets.QAbstractItemView.DragDrop)
         Registry().register_function(('%s_dnd' % self.mime_data_text), self.parent().load_file)
 
-    def clear(self, search_while_typing=False):
+    def clear(self, search_while_typing=False, override_lock=False):
         """
         Re-implement clear, so that we can customise feedback when using 'Search as you type'
 
         :param search_while_typing: True if we want to display the customised message
         :return: None
         """
+        if self.locked and not override_lock:
+            return
         if search_while_typing:
             self.no_results_text = UiStrings().ShortResults
         else:

openlp/plugins/bibles/bibleplugin.py

@@ -46,8 +46,7 @@ __default_settings__ = {
     'bibles/is verse number visible': True,
     'bibles/display new chapter': False,
     'bibles/second bibles': True,
-    'bibles/advanced bible': '',
-    'bibles/quick bible': '',
+    'bibles/primary bible': '',
     'bibles/proxy name': '',
     'bibles/proxy address': '',
     'bibles/proxy username': '',

openlp/plugins/bibles/lib/__init__.py

@@ -230,7 +230,7 @@ def update_reference_separators():
     REFERENCE_MATCHES['range_separator'] = re.compile(REFERENCE_SEPARATORS['sep_l'], re.UNICODE)
     # full reference match: <book>(<range>(,(?!$)|(?=$)))+
     REFERENCE_MATCHES['full'] = \
-        re.compile('^\s*(?!\s)(?P<book>[\d]*[^\d]+)(?<!\s)\s*'
+        re.compile('^\s*(?!\s)(?P<book>[\d]*[^\d\.]+)\.*(?<!\s)\s*'
                    '(?P<ranges>(?:%(range_regex)s(?:%(sep_l)s(?!\s*$)|(?=\s*$)))+)\s*$'
                    % dict(list(REFERENCE_SEPARATORS.items()) + [('range_regex', range_regex)]), re.UNICODE)
 
@@ -326,7 +326,7 @@ def parse_reference(reference, bible, language_selection, book_ref_id=False):
 
     ``^\s*(?!\s)(?P<book>[\d]*[^\d]+)(?<!\s)\s*``
         The ``book`` group starts with the first non-whitespace character. There are optional leading digits followed by
-        non-digits. The group ends before the whitspace in front of the next digit.
+        non-digits. The group ends before the whitspace, or a full stop in front of the next digit.
 
     ``(?P<ranges>(?:%(range_regex)s(?:%(sep_l)s(?!\s*$)|(?=\s*$)))+)\s*$``
         The second group contains all ``ranges``. This can be multiple declarations of range_regex separated by a list

openlp/plugins/bibles/lib/db.py

@@ -36,7 +36,7 @@ from sqlalchemy.orm.exc import UnmappedClassError
 from openlp.core.common import AppLocation, translate, clean_filename
 from openlp.core.lib.db import BaseModel, init_db, Manager
 from openlp.core.lib.ui import critical_error_message_box
-from openlp.plugins.bibles.lib import upgrade
+from openlp.plugins.bibles.lib import BibleStrings, LanguageSelection, upgrade
 
 log = logging.getLogger(__name__)
 
@@ -52,9 +52,15 @@ class BibleMeta(BaseModel):
 
 class Book(BaseModel):
     """
-    Song model
+    Bible Book model
     """
-    pass
+    def get_name(self, language_selection=LanguageSelection.Bible):
+        if language_selection == LanguageSelection.Bible:
+            return self.name
+        elif language_selection == LanguageSelection.Application:
+            return BibleStrings().BookNames[BiblesResourcesDB.get_book_by_id(self.book_reference_id)['abbreviation']]
+        elif language_selection == LanguageSelection.English:
+            return BiblesResourcesDB.get_book_by_id(self.book_reference_id)['name']
 
 
 class Verse(BaseModel):
@@ -380,13 +386,12 @@ class BibleDB(Manager):
         """
         log.debug('BibleDB.verse_search("{text}")'.format(text=text))
         verses = self.session.query(Verse)
-        # TODO: Find out what this is doing before converting to format()
         if text.find(',') > -1:
-            keywords = ['%%%s%%' % keyword.strip() for keyword in text.split(',')]
+            keywords = ['%{keyword}%'.format(keyword=keyword.strip()) for keyword in text.split(',') if keyword.strip()]
             or_clause = [Verse.text.like(keyword) for keyword in keywords]
             verses = verses.filter(or_(*or_clause))
         else:
-            keywords = ['%%%s%%' % keyword.strip() for keyword in text.split(' ')]
+            keywords = ['%{keyword}%'.format(keyword=keyword.strip()) for keyword in text.split(' ') if keyword.strip()]
             for keyword in keywords:
                 verses = verses.filter(Verse.text.like(keyword))
         verses = verses.all()

openlp/plugins/bibles/lib/manager.py

@@ -250,14 +250,20 @@ class BibleManager(OpenLPMixin, RegistryProperties):
                   '"{book}", "{chapter}")'.format(bible=bible, book=book_ref_id, chapter=chapter))
         return self.db_cache[bible].get_verse_count(book_ref_id, chapter)
 
-    def get_verses(self, bible, verse_text, book_ref_id=False, show_error=True):
+    def parse_ref(self, bible, reference_text, book_ref_id=False):
+        if not bible:
+            return
+        language_selection = self.get_language_selection(bible)
+        return parse_reference(reference_text, self.db_cache[bible], language_selection, book_ref_id)
+
+    def get_verses(self, bible, ref_list, show_error=True):
         """
         Parses a scripture reference, fetches the verses from the Bible
         specified, and returns a list of ``Verse`` objects.
 
         :param bible: Unicode. The Bible to use.
         :param verse_text:
-             Unicode. The scripture reference. Valid scripture references are:
+             String. The scripture reference. Valid scripture references are:
 
                 - Genesis 1
                 - Genesis 1-2
@@ -271,22 +277,9 @@ class BibleManager(OpenLPMixin, RegistryProperties):
             For second bible this is necessary.
         :param show_error:
         """
-        # If no bibles are installed, message is given.
-        log.debug('BibleManager.get_verses("{bible}", "{verse}")'.format(bible=bible, verse=verse_text))
-        if not bible:
-            if show_error:
-                self.main_window.information_message(
-                    UiStrings().BibleNoBiblesTitle,
-                    UiStrings().BibleNoBibles)
-            return None
-        # Get the language for books.
-        language_selection = self.get_language_selection(bible)
-        ref_list = parse_reference(verse_text, self.db_cache[bible], language_selection, book_ref_id)
-        if ref_list:
-            return self.db_cache[bible].get_verses(ref_list, show_error)
-        # If nothing is found. Message is given if this is not combined search. (defined in mediaitem.py)
-        else:
+        if not bible or not ref_list:
             return None
+        return self.db_cache[bible].get_verses(ref_list, show_error)
 
     def get_language_selection(self, bible):
         """
@@ -308,7 +301,7 @@ class BibleManager(OpenLPMixin, RegistryProperties):
             language_selection = LanguageSelection.Application
         return language_selection
 
-    def verse_search(self, bible, second_bible, text):
+    def verse_search(self, bible, text):
         """
         Does a verse search for the given bible and text.
 
@@ -325,20 +318,14 @@ class BibleManager(OpenLPMixin, RegistryProperties):
             return None
         # Check if the bible or second_bible is a web bible.
         web_bible = self.db_cache[bible].get_object(BibleMeta, 'download_source')
-        second_web_bible = ''
-        if second_bible:
-            second_web_bible = self.db_cache[second_bible].get_object(BibleMeta, 'download_source')
-        if web_bible or second_web_bible:
+        if web_bible:
             # If either Bible is Web, cursor is reset to normal and message is given.
             self.application.set_normal_cursor()
             self.main_window.information_message(
                 translate('BiblesPlugin.BibleManager', 'Web Bible cannot be used in Text Search'),
                 translate('BiblesPlugin.BibleManager', 'Text Search is not available with Web Bibles.\n'
                                                        'Please use the Scripture Reference Search instead.\n\n'
-                                                       'This means that the currently used Bible\nor Second Bible '
-                                                       'is installed as Web Bible.\n\n'
-                                                       'If you were trying to perform a Reference search\nin Combined '
-                                                       'Search, your reference is invalid.')
+                                                       'This means that the currently selected Bible is a Web Bible.')
             )
             return None
         # Shorter than 3 char searches break OpenLP with very long search times, thus they are blocked.
@@ -380,6 +367,20 @@ class BibleManager(OpenLPMixin, RegistryProperties):
         else:
             return None
 
+    def process_verse_range(self, book_ref_id, chapter_from, verse_from, chapter_to, verse_to):
+        verse_ranges = []
+        for chapter in range(chapter_from, chapter_to + 1):
+            if chapter == chapter_from:
+                start_verse = verse_from
+            else:
+                start_verse = 1
+            if chapter == chapter_to:
+                end_verse = verse_to
+            else:
+                end_verse = -1
+            verse_ranges.append((book_ref_id, chapter, start_verse, end_verse))
+        return verse_ranges
+
     def save_meta_data(self, bible, version, copyright, permissions, full_license, book_name_language=None):
         """
         Saves the bibles meta data.

resources/images/openlp-2.qrc

@@ -29,6 +29,7 @@
     <file>image_new_group.png</file>
   </qresource>
   <qresource prefix="bibles">
+    <file>bibles_book_sort.png</file>
     <file>bibles_search_combined.png</file>
     <file>bibles_search_text.png</file>
     <file>bibles_search_reference.png</file>

tests/functional/openlp_core_ui_lib/test_listwidgetwithdnd.py

@@ -33,6 +33,37 @@ class TestListWidgetWithDnD(TestCase):
     """
     Test the :class:`~openlp.core.lib.listwidgetwithdnd.ListWidgetWithDnD` class
     """
+    def test_clear_locked(self):
+        """
+        Test the clear method the list is 'locked'
+        """
+        with patch('openlp.core.ui.lib.listwidgetwithdnd.QtWidgets.QListWidget.clear') as mocked_clear_super_method:
+            # GIVEN: An instance of ListWidgetWithDnD
+            widget = ListWidgetWithDnD()
+
+            # WHEN: The list is 'locked' and clear has been called
+            widget.locked = True
+            widget.clear()
+
+            # THEN: The super method should not have been called (i.e. The list not cleared)
+            self.assertFalse(mocked_clear_super_method.called)
+
+    def test_clear_overide_locked(self):
+        """
+        Test the clear method the list is 'locked', but clear is called with 'override_lock' set to True
+        """
+        with patch('openlp.core.ui.lib.listwidgetwithdnd.QtWidgets.QListWidget.clear') as mocked_clear_super_method:
+            # GIVEN: An instance of ListWidgetWithDnD
+            widget = ListWidgetWithDnD()
+
+            # WHEN: The list is 'locked' and clear has been called with override_lock se to True
+            widget.locked = True
+            widget.clear(override_lock=True)
+
+            # THEN: The super method should have been called (i.e. The list is cleared regardless whether it is locked
+            #       or not)
+            mocked_clear_super_method.assert_called_once_with()
+
     def test_clear(self):
         """
         Test the clear method when called without any arguments.

tests/functional/openlp_plugins/bibles/test_lib.py

@@ -25,11 +25,12 @@ This module contains tests for the lib submodule of the Bibles plugin.
 from unittest import TestCase
 
 from openlp.plugins.bibles import lib
-from openlp.plugins.bibles.lib import SearchResults
-from tests.functional import patch
+from openlp.plugins.bibles.lib import SearchResults, get_reference_match
+from tests.functional import MagicMock, patch
+from tests.helpers.testmixin import TestMixin
 
 
-class TestLib(TestCase):
+class TestLib(TestCase, TestMixin):
     """
     Test the functions in the :mod:`lib` module.
     """
@@ -60,6 +61,142 @@ class TestLib(TestCase):
             self.assertEqual(separators[key], value)
         mocked_update_reference_separators.assert_called_once_with()
 
+    def test_reference_matched_full(self):
+        """
+        Test that the 'full' regex parses bible verse references correctly.
+        """
+        # GIVEN: Some test data which contains different references to parse, with the expected results.
+        with patch('openlp.plugins.bibles.lib.Settings', return_value=MagicMock(**{'value.return_value': ''})):
+            # The following test data tests with 222 variants when using the default 'separators'
+            test_data = [
+                # Input reference, book name, chapter + verse reference
+                ('Psalm 23', 'Psalm', '23'),
+                ('Psalm. 23', 'Psalm', '23'),
+                ('Psalm 23{to}24', 'Psalm', '23-24'),
+                ('Psalm 23{verse}1{to}2', 'Psalm', '23:1-2'),
+                ('Psalm 23{verse}1{to}{end}', 'Psalm', '23:1-end'),
+                ('Psalm 23{verse}1{to}2{_and}5{to}6', 'Psalm', '23:1-2,5-6'),
+                ('Psalm 23{verse}1{to}2{_and}5{to}{end}', 'Psalm', '23:1-2,5-end'),
+                ('Psalm 23{verse}1{to}2{_and}24{verse}1{to}3', 'Psalm', '23:1-2,24:1-3'),
+                ('Psalm 23{verse}1{to}{end}{_and}24{verse}1{to}{end}', 'Psalm', '23:1-end,24:1-end'),
+                ('Psalm 23{verse}1{to}24{verse}1', 'Psalm', '23:1-24:1'),
+                ('Psalm 23{_and}24', 'Psalm', '23,24'),
+                ('1 John 23', '1 John', '23'),
+                ('1 John. 23', '1 John', '23'),
+                ('1 John 23{to}24', '1 John', '23-24'),
+                ('1 John 23{verse}1{to}2', '1 John', '23:1-2'),
+                ('1 John 23{verse}1{to}{end}', '1 John', '23:1-end'),
+                ('1 John 23{verse}1{to}2{_and}5{to}6', '1 John', '23:1-2,5-6'),
+                ('1 John 23{verse}1{to}2{_and}5{to}{end}', '1 John', '23:1-2,5-end'),
+                ('1 John 23{verse}1{to}2{_and}24{verse}1{to}3', '1 John', '23:1-2,24:1-3'),
+                ('1 John 23{verse}1{to}{end}{_and}24{verse}1{to}{end}', '1 John', '23:1-end,24:1-end'),
+                ('1 John 23{verse}1{to}24{verse}1', '1 John', '23:1-24:1'),
+                ('1 John 23{_and}24', '1 John', '23,24')]
+
+            full_reference_match = get_reference_match('full')
+            for reference_text, book_result, ranges_result in test_data:
+                to_separators = ['-', ' - ', 'to', ' to '] if '{to}' in reference_text else ['']
+                verse_separators = [':', ' : ', 'v', ' v ', 'V', ' V ', 'verse', ' verse ', 'verses', ' verses '] \
+                    if '{verse}' in reference_text else ['']
+                and_separators = [',', ' , ', 'and', ' and '] if '{_and}' in reference_text else ['']
+                end_separators = ['end', ' end '] if '{end}' in reference_text else ['']
+
+                for to in to_separators:
+                    for verse in verse_separators:
+                        for _and in and_separators:
+                            for end in end_separators:
+                                reference_text = reference_text.format(to=to, verse=verse, _and=_and, end=end)
+
+                                # WHEN: Attempting to parse the input string
+                                match = full_reference_match.match(reference_text)
+
+                                # THEN: A match should be returned, and the book and reference should match the
+                                #       expected result
+                                self.assertIsNotNone(match, '{text} should provide a match'.format(text=reference_text))
+                                self.assertEqual(book_result, match.group('book'),
+                                                 '{text} does not provide the expected result for the book group.'
+                                                 .format(text=reference_text))
+                                self.assertEqual(ranges_result, match.group('ranges'),
+                                                 '{text} does not provide the expected result for the ranges group.'
+                                                 .format(text=reference_text))
+
+    def test_reference_matched_range(self):
+        """
+        Test that the 'range' regex parses bible verse references correctly.
+        Note: This test takes in to account that the regex does not work quite as expected!
+        see https://bugs.launchpad.net/openlp/+bug/1638620
+        """
+        # GIVEN: Some test data which contains different references to parse, with the expected results.
+        with patch('openlp.plugins.bibles.lib.Settings', return_value=MagicMock(**{'value.return_value': ''})):
+            # The following test data tests with 45 variants when using the default 'separators'
+            test_data = [
+                ('23', None, '23', None, None, None),
+                ('23{to}24', None, '23', '-24', None, '24'),
+                ('23{verse}1{to}2', '23', '1', '-2', None, '2'),
+                ('23{verse}1{to}{end}', '23', '1', '-end', None, None),
+                ('23{verse}1{to}24{verse}1', '23', '1', '-24:1', '24', '1')]
+            full_reference_match = get_reference_match('range')
+            for reference_text, from_chapter, from_verse, range_to, to_chapter, to_verse in test_data:
+                to_separators = ['-', ' - ', 'to', ' to '] if '{to}' in reference_text else ['']
+                verse_separators = [':', ' : ', 'v', ' v ', 'V', ' V ', 'verse', ' verse ', 'verses', ' verses '] \
+                    if '{verse}' in reference_text else ['']
+                and_separators = [',', ' , ', 'and', ' and '] if '{_and}' in reference_text else ['']
+                end_separators = ['end', ' end '] if '{end}' in reference_text else ['']
+
+                for to in to_separators:
+                    for verse in verse_separators:
+                        for _and in and_separators:
+                            for end in end_separators:
+                                reference_text = reference_text.format(to=to, verse=verse, _and=_and, end=end)
+
+                                # WHEN: Attempting to parse the input string
+                                match = full_reference_match.match(reference_text)
+
+                                # THEN: A match should be returned, and the to/from chapter/verses should match as
+                                #       expected
+                                self.assertIsNotNone(match, '{text} should provide a match'.format(text=reference_text))
+                                self.assertEqual(match.group('from_chapter'), from_chapter)
+                                self.assertEqual(match.group('from_verse'), from_verse)
+                                self.assertEqual(match.group('range_to'), range_to)
+                                self.assertEqual(match.group('to_chapter'), to_chapter)
+                                self.assertEqual(match.group('to_verse'), to_verse)
+
+    def test_reference_matched_range_separator(self):
+        # GIVEN: Some test data which contains different references to parse, with the expected results.
+        with patch('openlp.plugins.bibles.lib.Settings', return_value=MagicMock(**{'value.return_value': ''})):
+            # The following test data tests with 111 variants when using the default 'separators'
+            # The regex for handling ranges is a bit screwy, see https://bugs.launchpad.net/openlp/+bug/1638620
+            test_data = [
+                ('23', ['23']),
+                ('23{to}24', ['23-24']),
+                ('23{verse}1{to}2', ['23:1-2']),
+                ('23{verse}1{to}{end}', ['23:1-end']),
+                ('23{verse}1{to}2{_and}5{to}6', ['23:1-2', '5-6']),
+                ('23{verse}1{to}2{_and}5{to}{end}', ['23:1-2', '5-end']),
+                ('23{verse}1{to}2{_and}24{verse}1{to}3', ['23:1-2', '24:1-3']),
+                ('23{verse}1{to}{end}{_and}24{verse}1{to}{end}', ['23:1-end', '24:1-end']),
+                ('23{verse}1{to}24{verse}1', ['23:1-24:1']),
+                ('23,24', ['23', '24'])]
+            full_reference_match = get_reference_match('range_separator')
+            for reference_text, ranges in test_data:
+                to_separators = ['-', ' - ', 'to', ' to '] if '{to}' in reference_text else ['']
+                verse_separators = [':', ' : ', 'v', ' v ', 'V', ' V ', 'verse', ' verse ', 'verses', ' verses '] \
+                    if '{verse}' in reference_text else ['']
+                and_separators = [',', ' , ', 'and', ' and '] if '{_and}' in reference_text else ['']
+                end_separators = ['end', ' end '] if '{end}' in reference_text else ['']
+
+                for to in to_separators:
+                    for verse in verse_separators:
+                        for _and in and_separators:
+                            for end in end_separators:
+                                reference_text = reference_text.format(to=to, verse=verse, _and=_and, end=end)
+
+                                # WHEN: Attempting to parse the input string
+                                references = full_reference_match.split(reference_text)
+
+                                # THEN: The list of references should be as the expected results
+                                self.assertEqual(references, ranges)
+
     def test_search_results_creation(self):
         """
         Test the creation and construction of the SearchResults class