[Song select] Stop search on viewing a song.

[Song select] Add a stop button to the SongSelect importer to stop searching.
[Song select] Fix a potential bug where the song author only has 1 name (most commonly seen when the name is 'Unknown')

Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/song-select-fixes (revision 2582)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1227/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1152/
[SUCC...

bzr-revno: 2598

openlp/plugins/songs/forms/songselectdialog.py

@@ -114,12 +114,19 @@ class Ui_SongSelectDialog(object):
         self.search_button.setObjectName('search_button')
         self.search_input_layout.addWidget(self.search_button)
         self.search_layout.addLayout(self.search_input_layout)
+        self.search_progress_layout = QtWidgets.QHBoxLayout()
+        self.search_progress_layout.setSpacing(8)
+        self.search_progress_layout.setObjectName('search_progress_layout')
         self.search_progress_bar = QtWidgets.QProgressBar(self.search_page)
         self.search_progress_bar.setMinimum(0)
         self.search_progress_bar.setMaximum(3)
         self.search_progress_bar.setValue(0)
-        self.search_progress_bar.setVisible(False)
+        self.search_progress_layout.addWidget(self.search_progress_bar)
-        self.search_layout.addWidget(self.search_progress_bar)
+        self.stop_button = QtWidgets.QPushButton(self.search_page)
+        self.stop_button.setIcon(build_icon(':/songs/song_search_stop.png'))
+        self.stop_button.setObjectName('stop_button')
+        self.search_progress_layout.addWidget(self.stop_button)
+        self.search_layout.addLayout(self.search_progress_layout)
         self.search_results_widget = QtWidgets.QListWidget(self.search_page)
         self.search_results_widget.setProperty("showDropIndicator", False)
         self.search_results_widget.setAlternatingRowColors(True)
@@ -234,6 +241,7 @@ class Ui_SongSelectDialog(object):
         self.login_button.setText(translate('SongsPlugin.SongSelectForm', 'Login'))
         self.search_label.setText(translate('SongsPlugin.SongSelectForm', 'Search Text:'))
         self.search_button.setText(translate('SongsPlugin.SongSelectForm', 'Search'))
+        self.stop_button.setText(translate('SongsPlugin.SongSelectForm', 'Stop'))
         self.result_count_label.setText(translate('SongsPlugin.SongSelectForm', 'Found %s song(s)') % 0)
         self.logout_button.setText(translate('SongsPlugin.SongSelectForm', 'Logout'))
         self.view_button.setText(translate('SongsPlugin.SongSelectForm', 'View'))

openlp/plugins/songs/forms/songselectform.py

@@ -95,11 +95,13 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
         self.worker = None
         self.song_count = 0
         self.song = None
+        self.set_progress_visible(False)
         self.song_select_importer = SongSelectImport(self.db_manager)
         self.save_password_checkbox.toggled.connect(self.on_save_password_checkbox_toggled)
         self.login_button.clicked.connect(self.on_login_button_clicked)
         self.search_button.clicked.connect(self.on_search_button_clicked)
         self.search_combobox.returnPressed.connect(self.on_search_button_clicked)
+        self.stop_button.clicked.connect(self.on_stop_button_clicked)
         self.logout_button.clicked.connect(self.done)
         self.search_results_widget.itemDoubleClicked.connect(self.on_search_results_widget_double_clicked)
         self.search_results_widget.itemSelectionChanged.connect(self.on_search_results_widget_selection_changed)
@@ -154,18 +156,30 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
         return QtWidgets.QDialog.done(self, r)
 
     def _update_login_progress(self):
+        """
+        Update the progress bar as the user logs in.
+        """
         self.login_progress_bar.setValue(self.login_progress_bar.value() + 1)
         self.application.process_events()
 
     def _update_song_progress(self):
+        """
+        Update the progress bar as the song is being downloaded.
+        """
         self.song_progress_bar.setValue(self.song_progress_bar.value() + 1)
         self.application.process_events()
 
     def _view_song(self, current_item):
+        """
+        Load a song into the song view.
+        """
         if not current_item:
             return
         else:
             current_item = current_item.data(QtCore.Qt.UserRole)
+        # Stop the current search, if it's running
+        self.song_select_importer.stop()
+        # Clear up the UI
         self.song_progress_bar.setVisible(True)
         self.import_button.setEnabled(False)
         self.back_button.setEnabled(False)
@@ -289,7 +303,7 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
         self.search_progress_bar.setMinimum(0)
         self.search_progress_bar.setMaximum(0)
         self.search_progress_bar.setValue(0)
-        self.search_progress_bar.setVisible(True)
+        self.set_progress_visible(True)
         self.search_results_widget.clear()
         self.result_count_label.setText(translate('SongsPlugin.SongSelectForm', 'Found %s song(s)') % self.song_count)
         self.application.process_events()
@@ -309,6 +323,12 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
         self.thread.finished.connect(self.thread.deleteLater)
         self.thread.start()
 
+    def on_stop_button_clicked(self):
+        """
+        Stop the search when the stop button is clicked.
+        """
+        self.song_select_importer.stop()
+
     def on_search_show_info(self, title, message):
         """
         Show an informational message from the search thread
@@ -333,7 +353,7 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
         Slot which is called when the search is completed.
         """
         self.application.process_events()
-        self.search_progress_bar.setVisible(False)
+        self.set_progress_visible(False)
         self.search_button.setEnabled(True)
         self.application.process_events()
 
@@ -381,6 +401,13 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog):
             self.application.process_events()
             self.done(QtWidgets.QDialog.Accepted)
 
+    def set_progress_visible(self, is_visible):
+        """
+        Show or hide the search progress, including the stop button.
+        """
+        self.search_progress_bar.setVisible(is_visible)
+        self.stop_button.setVisible(is_visible)
+
     @property
     def application(self):
         """

openlp/plugins/songs/lib/songselect.py

@@ -61,6 +61,7 @@ class SongSelectImport(object):
         self.html_parser = HTMLParser()
         self.opener = build_opener(HTTPCookieProcessor(CookieJar()))
         self.opener.addheaders = [('User-Agent', USER_AGENT)]
+        self.run_search = True
 
     def login(self, username, password, callback=None):
         """
@@ -115,10 +116,11 @@ class SongSelectImport(object):
         :param callback: A method which is called when each song is found, with the song as a parameter.
         :return: List of songs
         """
+        self.run_search = True
         params = {'allowredirect': 'false', 'SearchTerm': search_text}
         current_page = 1
         songs = []
-        while True:
+        while self.run_search:
             if current_page > 1:
                 params['page'] = current_page
             try:
@@ -220,3 +222,9 @@ class SongSelectImport(object):
             db_song.add_author(author)
         self.db_manager.save_object(db_song)
         return db_song
+
+    def stop(self):
+        """
+        Stop the search.
+        """
+        self.run_search = False

resources/images/openlp-2.qrc

@@ -1,5 +1,6 @@
 <RCC>
   <qresource prefix="songs">
+    <file>song_search_stop.png</file>
     <file>song_search_all.png</file>
     <file>song_search_author.png</file>
     <file>song_search_lyrics.png</file>

tests/functional/openlp_plugins/songs/test_songselect.py

@@ -192,7 +192,7 @@ class TestSongSelectImport(TestCase, TestMixin):
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
-        # WHEN: The login method is called after being rigged to fail
+        # WHEN: The search method is called
         results = importer.search('text', 1000, mock_callback)
 
         # THEN: callback was never called, open was called once, find_all was called once, an empty list returned
@@ -234,10 +234,10 @@ class TestSongSelectImport(TestCase, TestMixin):
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
-        # WHEN: The login method is called after being rigged to fail
+        # WHEN: The search method is called
         results = importer.search('text', 2, mock_callback)
 
-        # THEN: callback was never called, open was called once, find_all was called once, an empty list returned
+        # THEN: callback was called twice, open was called twice, find_all was called twice, max results returned
         self.assertEqual(2, mock_callback.call_count, 'callback should have been called twice')
         self.assertEqual(2, mocked_opener.open.call_count, 'open should have been called twice')
         self.assertEqual(2, mocked_results_page.find_all.call_count, 'find_all should have been called twice')
@@ -246,6 +246,22 @@ class TestSongSelectImport(TestCase, TestMixin):
                          {'title': 'Title 2', 'authors': ['Author 2-1', 'Author 2-2'], 'link': BASE_URL + '/url2'}]
         self.assertListEqual(expected_list, results, 'The search method should have returned two songs')
 
+    @patch('openlp.plugins.songs.lib.songselect.build_opener')
+    @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup')
+    def stop_called_test(self, MockedBeautifulSoup, mocked_build_opener):
+        """
+        Test that the search is stopped with stop() is called
+        """
+        # GIVEN: An importer object that is currently "searching"
+        importer = SongSelectImport(None)
+        importer.run_search = True
+
+        # WHEN: The stop method is called
+        results = importer.stop()
+
+        # THEN: Searching should have stopped
+        self.assertFalse(importer.run_search, 'Searching should have been stopped')
+
     @patch('openlp.plugins.songs.lib.songselect.build_opener')
     def get_song_page_raises_exception_test(self, mocked_build_opener):
         """
@@ -686,6 +702,23 @@ class TestSongSelectForm(TestCase, TestMixin):
         # THEN: The view button should be enabled
         mocked_view_button.setEnabled.assert_called_with(True)
 
+    @patch('openlp.plugins.songs.forms.songselectform.SongSelectImport')
+    def on_stop_button_clicked_test(self, MockedSongSelectImport):
+        """
+        Test that the search is stopped when the stop button is clicked
+        """
+        # GIVEN: A mocked SongSelectImporter and a SongSelect form
+        mocked_song_select_importer = MagicMock()
+        MockedSongSelectImport.return_value = mocked_song_select_importer
+        ssform = SongSelectForm(None, MagicMock(), MagicMock())
+        ssform.initialise()
+
+        # WHEN: The stop button is clicked
+        ssform.on_stop_button_clicked()
+
+        # THEN: The view button should be enabled
+        mocked_song_select_importer.stop.assert_called_with()
+
 
 class TestSongSelectFileImport(SongImportTestHelper):