From 77d36b607c9a4d58f4784d83b71e6e2053e5acc7 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 1 Mar 2017 14:54:38 -0700 Subject: [PATCH 1/4] Hopefully, for one and for all, fix the off-by-one issues on all platforms Fixes: https://launchpad.net/bugs/1669007, https://launchpad.net/bugs/1668994 --- openlp/plugins/songs/forms/editsongform.py | 48 ++++++++++++---------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/openlp/plugins/songs/forms/editsongform.py b/openlp/plugins/songs/forms/editsongform.py index e9e8d6540..69d36cdfc 100644 --- a/openlp/plugins/songs/forms/editsongform.py +++ b/openlp/plugins/songs/forms/editsongform.py @@ -31,7 +31,8 @@ import shutil from PyQt5 import QtCore, QtWidgets -from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, translate +from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, \ + translate, is_macosx from openlp.core.lib import FileDialog, PluginStatus, MediaType, create_separated_list from openlp.core.lib.ui import set_case_insensitive_completer, critical_error_message_box, find_and_set_in_combo_box from openlp.plugins.songs.lib import VerseType, clean_song @@ -382,7 +383,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): self.themes = theme_list self.theme_combo_box.addItems(theme_list) set_case_insensitive_completer(self.themes, self.theme_combo_box) - self.theme_combo_box.setEditText('') + self.theme_combo_box.setCurrentIndex(-1) + self.theme_combo_box.setCurrentText('') def load_media_files(self): """ @@ -421,7 +423,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): self.load_topics() self.load_songbooks() self.load_media_files() - self.theme_combo_box.setEditText('') + self.theme_combo_box.setCurrentIndex(-1) + self.theme_combo_box.setCurrentText('') # it's a new song to preview is not possible self.preview_button.setVisible(False) @@ -446,8 +449,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): find_and_set_in_combo_box(self.theme_combo_box, str(self.song.theme_name)) else: # Clear the theme combo box in case it was previously set (bug #1212801) - self.theme_combo_box.setEditText('') - self.theme_combo_box.setCurrentIndex(0) + self.theme_combo_box.setCurrentIndex(-1) + self.theme_combo_box.setCurrentText('') self.copyright_edit.setText(self.song.copyright if self.song.copyright else '') self.comments_edit.setPlainText(self.song.comments if self.song.comments else '') self.ccli_number_edit.setText(self.song.ccli_number if self.song.ccli_number else '') @@ -550,12 +553,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): item = int(self.authors_combo_box.currentIndex()) text = self.authors_combo_box.currentText().strip(' \r\n\t') author_type = self.author_types_combo_box.itemData(self.author_types_combo_box.currentIndex()) - # This if statement is for OS X, which doesn't seem to work well with - # the QCompleter auto-completion class. See bug #812628. - if text in self.authors: - # Index 0 is a blank string, so add 1 - item = self.authors.index(text) + 1 - if item == 0 and text: + if item == -1 and text: if QtWidgets.QMessageBox.question( self, translate('SongsPlugin.EditSongForm', 'Add Author'), @@ -570,10 +568,11 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): self.manager.save_object(author) self._add_author_to_list(author, author_type) self.load_authors() - self.authors_combo_box.setEditText('') + self.authors_combo_box.setCurrentIndex(-1) + self.authors_combo_box.setCurrentText('') else: return - elif item > 0: + elif item >= 0: item_id = (self.authors_combo_box.itemData(item)) author = self.manager.get_object(Author, item_id) if self.authors_list_view.findItems(author.get_display_name(author_type), QtCore.Qt.MatchExactly): @@ -581,7 +580,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): message=translate('SongsPlugin.EditSongForm', 'This author is already in the list.')) else: self._add_author_to_list(author, author_type) - self.authors_combo_box.setEditText('') + self.authors_combo_box.setCurrentIndex(-1) + self.authors_combo_box.setCurrentText('') else: QtWidgets.QMessageBox.warning( self, UiStrings().NISs, @@ -633,7 +633,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): def on_topic_add_button_clicked(self): item = int(self.topics_combo_box.currentIndex()) text = self.topics_combo_box.currentText() - if item == 0 and text: + if item == -1 and text: if QtWidgets.QMessageBox.question( self, translate('SongsPlugin.EditSongForm', 'Add Topic'), translate('SongsPlugin.EditSongForm', 'This topic does not exist, do you want to add it?'), @@ -645,10 +645,11 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): topic_item.setData(QtCore.Qt.UserRole, topic.id) self.topics_list_view.addItem(topic_item) self.load_topics() - self.topics_combo_box.setEditText('') + self.topics_combo_box.setCurrentIndex(-1) + self.topics_combo_box.setCurrentText('') else: return - elif item > 0: + elif item >= 0: item_id = (self.topics_combo_box.itemData(item)) topic = self.manager.get_object(Topic, item_id) if self.topics_list_view.findItems(str(topic.name), QtCore.Qt.MatchExactly): @@ -658,7 +659,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): topic_item = QtWidgets.QListWidgetItem(str(topic.name)) topic_item.setData(QtCore.Qt.UserRole, topic.id) self.topics_list_view.addItem(topic_item) - self.topics_combo_box.setEditText('') + self.topics_combo_box.setCurrentIndex(-1) + self.topics_combo_box.setCurrentText('') else: QtWidgets.QMessageBox.warning( self, UiStrings().NISs, @@ -678,7 +680,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): def on_songbook_add_button_clicked(self): item = int(self.songbooks_combo_box.currentIndex()) text = self.songbooks_combo_box.currentText() - if item == 0 and text: + if item == -1 and text: if QtWidgets.QMessageBox.question( self, translate('SongsPlugin.EditSongForm', 'Add Songbook'), translate('SongsPlugin.EditSongForm', 'This Songbook does not exist, do you want to add it?'), @@ -688,11 +690,12 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): self.manager.save_object(songbook) self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text()) self.load_songbooks() - self.songbooks_combo_box.setEditText('') + self.songbooks_combo_box.setCurrentIndex(-1) + self.songbooks_combo_box.setCurrentText('') self.songbook_entry_edit.clear() else: return - elif item > 0: + elif item >= 0: item_id = (self.songbooks_combo_box.itemData(item)) songbook = self.manager.get_object(Book, item_id) if self.songbooks_list_view.findItems(str(songbook.name), QtCore.Qt.MatchExactly): @@ -700,7 +703,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): message=translate('SongsPlugin.EditSongForm', 'This Songbook is already in the list.')) else: self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text()) - self.songbooks_combo_box.setEditText('') + self.songbooks_combo_box.setCurrentIndex(-1) + self.songbooks_combo_box.setCurrentText('') self.songbook_entry_edit.clear() else: QtWidgets.QMessageBox.warning( From 35af0af21167279d3e8fa04a0fb73219388dae1f Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 1 Mar 2017 19:35:04 -0700 Subject: [PATCH 2/4] Some tests to go with it --- .../songs/forms/test_authorsform.py | 215 ++++++++++++++++++ 1 file changed, 215 insertions(+) diff --git a/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py b/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py index 701f37d94..3ff5c8baa 100644 --- a/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py +++ b/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py @@ -23,6 +23,7 @@ Package to test the openlp.plugins.songs.forms.authorsform package. """ from unittest import TestCase +from unittest.mock import patch from PyQt5 import QtWidgets @@ -138,3 +139,217 @@ class TestAuthorsForm(TestCase, TestMixin): # THEN: The display_name_edit should have the correct value self.assertEqual(self.form.display_edit.text(), display_name, 'The display name should be set correctly') + + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.exec') + def test_exec(self, mocked_exec): + """ + Test the exec() method + """ + # GIVEN: An authors for and various mocked objects + with patch.object(self.form.first_name_edit, 'clear') as mocked_first_name_edit_clear, \ + patch.object(self.form.last_name_edit, 'clear') as mocked_last_name_edit_clear, \ + patch.object(self.form.display_edit, 'clear') as mocked_display_edit_clear, \ + patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus: + # WHEN: The exec() method is called + self.form.exec(clear=True) + + # THEN: The clear and exec() methods should have been called + mocked_first_name_edit_clear.assert_called_once_with() + mocked_last_name_edit_clear.assert_called_once_with() + mocked_display_edit_clear.assert_called_once_with() + mocked_first_name_edit_setFocus.assert_called_once_with() + mocked_exec.assert_called_once_with(self.form) + + def test_first_name_edited(self): + """ + Test the on_first_name_edited() method + """ + # GIVEN: An author form + self.form.auto_display_name = True + + with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: + mocked_last_name_edit_text.return_value = 'Newton' + + # WHEN: on_first_name_edited() is called + self.form.on_first_name_edited('John') + + # THEN: The display name should be updated + assert mocked_last_name_edit_text.call_count == 2 + mocked_display_edit_setText.assert_called_once_with('John Newton') + + def test_first_name_edited_no_auto(self): + """ + Test the on_first_name_edited() method without auto_display_name + """ + # GIVEN: An author form + self.form.auto_display_name = False + + with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: + + # WHEN: on_first_name_edited() is called + self.form.on_first_name_edited('John') + + # THEN: The display name should not be updated + assert mocked_last_name_edit_text.call_count == 0 + assert mocked_display_edit_setText.call_count == 0 + + def test_last_name_edited(self): + """ + Test the on_last_name_edited() method + """ + # GIVEN: An author form + self.form.auto_display_name = True + + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: + mocked_first_name_edit_text.return_value = 'John' + + # WHEN: on_last_name_edited() is called + self.form.on_last_name_edited('Newton') + + # THEN: The display name should be updated + assert mocked_first_name_edit_text.call_count == 2 + mocked_display_edit_setText.assert_called_once_with('John Newton') + + def test_last_name_edited_no_auto(self): + """ + Test the on_last_name_edited() method without auto_display_name + """ + # GIVEN: An author form + self.form.auto_display_name = False + + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: + + # WHEN: on_last_name_edited() is called + self.form.on_last_name_edited('Newton') + + # THEN: The display name should not be updated + assert mocked_first_name_edit_text.call_count == 0 + assert mocked_display_edit_setText.call_count == 0 + + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') + def test_accept_no_first_name(self, mocked_critical_error): + """ + Test the accept() method with no first name + """ + # GIVEN: A form and no text in thefirst name edit + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus: + mocked_first_name_edit_text.return_value = '' + + # WHEN: accept() is called + result = self.form.accept() + + # THEN: The result should be false and a critical error displayed + assert result is False + mocked_critical_error.assert_called_once_with(message='You need to type in the first name of the author.') + mocked_first_name_edit_text.assert_called_once_with() + mocked_first_name_edit_setFocus.assert_called_once_with() + + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') + def test_accept_no_last_name(self, mocked_critical_error): + """ + Test the accept() method with no last name + """ + # GIVEN: A form and no text in the last name edit + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.last_name_edit, 'setFocus') as mocked_last_name_edit_setFocus: + mocked_first_name_edit_text.return_value = 'John' + mocked_last_name_edit_text.return_value = '' + + # WHEN: accept() is called + result = self.form.accept() + + # THEN: The result should be false and a critical error displayed + assert result is False + mocked_critical_error.assert_called_once_with(message='You need to type in the last name of the author.') + mocked_first_name_edit_text.assert_called_once_with() + mocked_last_name_edit_text.assert_called_once_with() + mocked_last_name_edit_setFocus.assert_called_once_with() + + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') + def test_accept_no_display_name_no_combine(self, mocked_critical_error): + """ + Test the accept() method with no display name and no combining + """ + # GIVEN: A form and no text in the display name edit + mocked_critical_error.return_value = QtWidgets.QMessageBox.No + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \ + patch.object(self.form.display_edit, 'setFocus') as mocked_display_edit_setFocus: + mocked_first_name_edit_text.return_value = 'John' + mocked_last_name_edit_text.return_value = 'Newton' + mocked_display_edit_text.return_value = '' + + # WHEN: accept() is called + result = self.form.accept() + + # THEN: The result should be false and a critical error displayed + assert result is False + mocked_critical_error.assert_called_once_with( + message='You have not set a display name for the author, combine the first and last names?', + parent=self.form, question=True) + mocked_first_name_edit_text.assert_called_once_with() + mocked_last_name_edit_text.assert_called_once_with() + mocked_display_edit_text.assert_called_once_with() + mocked_display_edit_setFocus.assert_called_once_with() + + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept') + def test_accept_no_display_name(self, mocked_accept, mocked_critical_error): + """ + Test the accept() method with no display name and auto-combine + """ + # GIVEN: A form and no text in the display name edit + mocked_accept.return_value = True + mocked_critical_error.return_value = QtWidgets.QMessageBox.Yes + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \ + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: + mocked_first_name_edit_text.return_value = 'John' + mocked_last_name_edit_text.return_value = 'Newton' + mocked_display_edit_text.return_value = '' + + # WHEN: accept() is called + result = self.form.accept() + + # THEN: The result should be false and a critical error displayed + assert result is True + mocked_critical_error.assert_called_once_with( + message='You have not set a display name for the author, combine the first and last names?', + parent=self.form, question=True) + mocked_first_name_edit_text.assert_called_once_with() + mocked_last_name_edit_text.assert_called_once_with() + mocked_display_edit_text.assert_called_once_with() + mocked_display_edit_setText.assert_called_once_with('John Newton') + mocked_accept.assert_called_once_with(self.form) + + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept') + def test_accept(self, mocked_accept): + """ + Test the accept() method + """ + # GIVEN: A form and text in the right places + mocked_accept.return_value = True + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text: + mocked_first_name_edit_text.return_value = 'John' + mocked_last_name_edit_text.return_value = 'Newton' + mocked_display_edit_text.return_value = 'John Newton' + + # WHEN: accept() is called + result = self.form.accept() + + # THEN: The result should be false and a critical error displayed + assert result is True + mocked_first_name_edit_text.assert_called_once_with() + mocked_last_name_edit_text.assert_called_once_with() + mocked_display_edit_text.assert_called_once_with() + mocked_accept.assert_called_once_with(self.form) From 678a42263d274110916eab059f60fe5d3d415e06 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 1 Mar 2017 19:51:15 -0700 Subject: [PATCH 3/4] Fixed a test --- .../interfaces/openlp_plugins/songs/forms/test_authorsform.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py b/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py index 3ff5c8baa..587d38bd8 100644 --- a/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py +++ b/tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py @@ -324,8 +324,8 @@ class TestAuthorsForm(TestCase, TestMixin): mocked_critical_error.assert_called_once_with( message='You have not set a display name for the author, combine the first and last names?', parent=self.form, question=True) - mocked_first_name_edit_text.assert_called_once_with() - mocked_last_name_edit_text.assert_called_once_with() + assert mocked_first_name_edit_text.call_count == 2 + assert mocked_last_name_edit_text.call_count == 2 mocked_display_edit_text.assert_called_once_with() mocked_display_edit_setText.assert_called_once_with('John Newton') mocked_accept.assert_called_once_with(self.form) From bef7c9df13b526d155bed50d1feaa0403292c37d Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 1 Mar 2017 22:02:21 -0700 Subject: [PATCH 4/4] Forgot a few things --- openlp/plugins/songs/forms/editsongform.py | 6 ++++-- tests/functional/openlp_plugins/songs/test_editsongform.py | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/openlp/plugins/songs/forms/editsongform.py b/openlp/plugins/songs/forms/editsongform.py index 69d36cdfc..fce073818 100644 --- a/openlp/plugins/songs/forms/editsongform.py +++ b/openlp/plugins/songs/forms/editsongform.py @@ -119,7 +119,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): cache.append(obj.name) combo.setItemData(row, obj.id) set_case_insensitive_completer(cache, combo) - combo.setEditText('') + combo.setCurrentIndex(-1) + combo.setCurrentText('') def _add_author_to_list(self, author, author_type): """ @@ -353,7 +354,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties): self.authors_combo_box.setItemData(row, author.id) self.authors.append(author.display_name) set_case_insensitive_completer(self.authors, self.authors_combo_box) - self.authors_combo_box.setEditText('') + self.authors_combo_box.setCurrentIndex(-1) + self.authors_combo_box.setCurrentText('') # Types self.author_types_combo_box.clear() diff --git a/tests/functional/openlp_plugins/songs/test_editsongform.py b/tests/functional/openlp_plugins/songs/test_editsongform.py index 460383365..9cdd2c935 100644 --- a/tests/functional/openlp_plugins/songs/test_editsongform.py +++ b/tests/functional/openlp_plugins/songs/test_editsongform.py @@ -106,4 +106,5 @@ class TestEditSongForm(TestCase, TestMixin): mocked_cache.append.assert_called_once_with('Charles') mocked_combo.setItemData.assert_called_once_with(0, 1) mocked_set_case_insensitive_completer.assert_called_once_with(mocked_cache, mocked_combo) - mocked_combo.setEditText.assert_called_once_with('') + mocked_combo.setCurrentIndex.assert_called_once_with(-1) + mocked_combo.setCurrentText.assert_called_once_with('')