From 726234591294d20198a6020ff4162c653dcbb1b2 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 27 Apr 2016 20:58:35 +0200 Subject: [PATCH 1/9] Fix bug #1557514 by auto-detecting the columns of the tables in the songs database Fixes: https://launchpad.net/bugs/1557514 --- .bzrignore | 1 + openlp/plugins/songs/lib/db.py | 2 +- openlp/plugins/songs/lib/importers/openlp.py | 88 ++++++++++++++------ 3 files changed, 63 insertions(+), 28 deletions(-) diff --git a/.bzrignore b/.bzrignore index b7dffe4fb..97af7bea6 100644 --- a/.bzrignore +++ b/.bzrignore @@ -45,3 +45,4 @@ cover *.kdev4 coverage tags +output diff --git a/openlp/plugins/songs/lib/db.py b/openlp/plugins/songs/lib/db.py index 5ea35d6b6..3026915e4 100644 --- a/openlp/plugins/songs/lib/db.py +++ b/openlp/plugins/songs/lib/db.py @@ -383,7 +383,7 @@ def init_schema(url): # Use lazy='joined' to always load authors when the song is fetched from the database (bug 1366198) 'authors': relation(Author, secondary=authors_songs_table, viewonly=True, lazy='joined'), 'media_files': relation(MediaFile, backref='songs', order_by=media_files_table.c.weight), - 'songbook_entries': relation(SongBookEntry, backref='song', cascade="all, delete-orphan"), + 'songbook_entries': relation(SongBookEntry, backref='song', cascade='all, delete-orphan'), 'topics': relation(Topic, backref='songs', secondary=songs_topics_table) }) mapper(Topic, topics_table) diff --git a/openlp/plugins/songs/lib/importers/openlp.py b/openlp/plugins/songs/lib/importers/openlp.py index e17fe138f..20c603e28 100644 --- a/openlp/plugins/songs/lib/importers/openlp.py +++ b/openlp/plugins/songs/lib/importers/openlp.py @@ -51,7 +51,7 @@ class OpenLPSongImport(SongImport): :param manager: The song manager for the running OpenLP installation. :param kwargs: The database providing the data to import. """ - SongImport.__init__(self, manager, **kwargs) + super(OpenLPSongImport, self).__init__(manager, **kwargs) self.source_session = None def do_import(self, progress_dialog=None): @@ -63,49 +63,61 @@ class OpenLPSongImport(SongImport): class OldAuthor(BaseModel): """ - Author model + Maps to the authors table """ pass class OldBook(BaseModel): """ - Book model + Maps to the songbooks table """ pass class OldMediaFile(BaseModel): """ - MediaFile model + Maps to the media_files table """ pass class OldSong(BaseModel): """ - Song model + Maps to the songs table """ pass class OldTopic(BaseModel): """ - Topic model + Maps to the topics table + """ + pass + + class OldSongBookEntry(BaseModel): + """ + Maps to the songs_songbooks table """ pass # Check the file type - if not self.import_source.endswith('.sqlite'): + if not isinstance(self.import_source, str) or not self.import_source.endswith('.sqlite'): self.log_error(self.import_source, translate('SongsPlugin.OpenLPSongImport', 'Not a valid OpenLP 2 song database.')) return self.import_source = 'sqlite:///%s' % self.import_source - # Load the db file + # Load the db file and reflect it engine = create_engine(self.import_source) source_meta = MetaData() source_meta.reflect(engine) self.source_session = scoped_session(sessionmaker(bind=engine)) + # Run some checks to see which version of the database we have if 'media_files' in list(source_meta.tables.keys()): has_media_files = True else: has_media_files = False + if 'songs_songbooks' in list(source_meta.tables.keys()): + has_songs_books = True + else: + has_songs_books = False + # Load up the tabls and map them out source_authors_table = source_meta.tables['authors'] source_song_books_table = source_meta.tables['song_books'] source_songs_table = source_meta.tables['songs'] @@ -113,6 +125,7 @@ class OpenLPSongImport(SongImport): source_authors_songs_table = source_meta.tables['authors_songs'] source_songs_topics_table = source_meta.tables['songs_topics'] source_media_files_songs_table = None + # Set up media_files relations if has_media_files: source_media_files_table = source_meta.tables['media_files'] source_media_files_songs_table = source_meta.tables.get('media_files_songs') @@ -120,9 +133,15 @@ class OpenLPSongImport(SongImport): class_mapper(OldMediaFile) except UnmappedClassError: mapper(OldMediaFile, source_media_files_table) + if has_songs_books: + source_songs_songbooks_table = source_meta.tables['songs_songbooks'] + try: + class_mapper(OldSongBookEntry) + except UnmappedClassError: + mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)}) + # Set up the songs relationships song_props = { 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), - 'book': relation(OldBook, backref='songs'), 'topics': relation(OldTopic, backref='songs', secondary=source_songs_topics_table) } if has_media_files: @@ -134,6 +153,11 @@ class OpenLPSongImport(SongImport): relation(OldMediaFile, backref='songs', foreign_keys=[source_media_files_table.c.song_id], primaryjoin=source_songs_table.c.id == source_media_files_table.c.song_id) + if has_songs_books: + song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan') + else: + song_props['book'] = relation(OldBook, backref='songs') + # Map the rest of the tables try: class_mapper(OldAuthor) except UnmappedClassError: @@ -163,44 +187,54 @@ class OpenLPSongImport(SongImport): old_titles = song.search_title.split('@') if len(old_titles) > 1: new_song.alternate_title = old_titles[1] - # Values will be set when cleaning the song. + # Transfer the values to the new song object new_song.search_title = '' new_song.search_lyrics = '' - new_song.song_number = song.song_number new_song.lyrics = song.lyrics new_song.verse_order = song.verse_order new_song.copyright = song.copyright new_song.comments = song.comments new_song.theme_name = song.theme_name new_song.ccli_number = song.ccli_number + if hasattr(song, 'song_number') and song.song_number: + new_song.song_number = song.song_number + # Find or create all the authors and add them to the new song object for author in song.authors: existing_author = self.manager.get_object_filtered(Author, Author.display_name == author.display_name) - if existing_author is None: + if not existing_author: existing_author = Author.populate( first_name=author.first_name, last_name=author.last_name, display_name=author.display_name) new_song.add_author(existing_author) - if song.book: - existing_song_book = self.manager.get_object_filtered(Book, Book.name == song.book.name) - if existing_song_book is None: - existing_song_book = Book.populate(name=song.book.name, publisher=song.book.publisher) - new_song.book = existing_song_book + # Find or create all the topics and add them to the new song object if song.topics: for topic in song.topics: existing_topic = self.manager.get_object_filtered(Topic, Topic.name == topic.name) - if existing_topic is None: + if not existing_topic: existing_topic = Topic.populate(name=topic.name) new_song.topics.append(existing_topic) - if has_media_files: - if song.media_files: - for media_file in song.media_files: - existing_media_file = self.manager.get_object_filtered( - MediaFile, MediaFile.file_name == media_file.file_name) - if existing_media_file: - new_song.media_files.append(existing_media_file) - else: - new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name)) + # Find or create all the songbooks and add them to the new song object + if has_songs_books and song.songbook_entries: + for entry in song.songbook_entries: + existing_book = self.manager.get_object_filtered(Book, Book.name == entry.songbook.name) + if not existing_book: + existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher) + new_song.add_songbook_entry(existing_book, entry.entry) + elif song.book: + existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name) + if not existing_book: + existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher) + new_song.add_songbook_entry(existing_book, '') + # Find or create all the media files and add them to the new song object + if has_media_files and song.media_files: + for media_file in song.media_files: + existing_media_file = self.manager.get_object_filtered( + MediaFile, MediaFile.file_name == media_file.file_name) + if existing_media_file: + new_song.media_files.append(existing_media_file) + else: + new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name)) clean_song(self.manager, new_song) self.manager.save_object(new_song) if progress_dialog: From 60f3ae195cc130c196d6b823531193ec05ff462a Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 27 Apr 2016 23:23:16 +0200 Subject: [PATCH 2/9] Forgot to add the test file --- .../songs/test_openlpimporter.py | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 tests/functional/openlp_plugins/songs/test_openlpimporter.py diff --git a/tests/functional/openlp_plugins/songs/test_openlpimporter.py b/tests/functional/openlp_plugins/songs/test_openlpimporter.py new file mode 100644 index 000000000..113db16e0 --- /dev/null +++ b/tests/functional/openlp_plugins/songs/test_openlpimporter.py @@ -0,0 +1,76 @@ +# -*- coding: utf-8 -*- +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 + +############################################################################### +# OpenLP - Open Source Lyrics Projection # +# --------------------------------------------------------------------------- # +# Copyright (c) 2008-2016 OpenLP Developers # +# --------------------------------------------------------------------------- # +# This program is free software; you can redistribute it and/or modify it # +# under the terms of the GNU General Public License as published by the Free # +# Software Foundation; version 2 of the License. # +# # +# This program is distributed in the hope that it will be useful, but WITHOUT # +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for # +# more details. # +# # +# You should have received a copy of the GNU General Public License along # +# with this program; if not, write to the Free Software Foundation, Inc., 59 # +# Temple Place, Suite 330, Boston, MA 02111-1307 USA # +############################################################################### +""" +This module contains tests for the OpenLP song importer. +""" +from unittest import TestCase + +from openlp.plugins.songs.lib.importers.openlp import OpenLPSongImport +from openlp.core.common import Registry +from tests.functional import patch, MagicMock + + +class TestOpenLPImport(TestCase): + """ + Test the functions in the :mod:`openlp` importer module. + """ + def setUp(self): + """ + Create the registry + """ + Registry.create() + + def create_importer_test(self): + """ + Test creating an instance of the OpenLP database importer + """ + # GIVEN: A mocked out SongImport class, and a mocked out "manager" + with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'): + mocked_manager = MagicMock() + + # WHEN: An importer object is created + importer = OpenLPSongImport(mocked_manager, filenames=[]) + + # THEN: The importer object should not be None + self.assertIsNotNone(importer, 'Import should not be none') + + def invalid_import_source_test(self): + """ + Test OpenLPSongImport.do_import handles different invalid import_source values + """ + # GIVEN: A mocked out SongImport class, and a mocked out "manager" + with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'): + mocked_manager = MagicMock() + mocked_import_wizard = MagicMock() + importer = OpenLPSongImport(mocked_manager, filenames=[]) + importer.import_wizard = mocked_import_wizard + importer.stop_import_flag = True + + # WHEN: Import source is not a list + for source in ['not a list', 0]: + importer.import_source = source + + # THEN: do_import should return none and the progress bar maximum should not be set. + self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list') + self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False, + 'setMaximum on import_wizard.progress_bar should not have been called') + From d636461cbba5b6e68e9ea0b53dfede0dd74f09f5 Mon Sep 17 00:00:00 2001 From: Samuel Mehrbrodt Date: Fri, 29 Apr 2016 18:30:17 +0200 Subject: [PATCH 3/9] Fix performance in songbook search Speedup by ~500% Fixes: https://launchpad.net/bugs/1552563 --- openlp/plugins/songs/lib/mediaitem.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/openlp/plugins/songs/lib/mediaitem.py b/openlp/plugins/songs/lib/mediaitem.py index 8edac2877..d08be8422 100644 --- a/openlp/plugins/songs/lib/mediaitem.py +++ b/openlp/plugins/songs/lib/mediaitem.py @@ -207,9 +207,11 @@ class SongMediaItem(MediaManagerItem): search_keywords = search_keywords.rpartition(' ') search_book = search_keywords[0] + '%' search_entry = search_keywords[2] + '%' - search_results = (self.plugin.manager.session.query(SongBookEntry) + search_results = (self.plugin.manager.session.query(SongBookEntry.entry, Book.name, Song.title, Song.id) + .join(Song) .join(Book) - .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry)).all()) + .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry), + Song.temporary.is_(False)).all()) self.display_results_book(search_results) elif search_type == SongSearch.Themes: log.debug('Theme Search') @@ -316,20 +318,17 @@ class SongMediaItem(MediaManagerItem): :param search_results: A list of db SongBookEntry objects :return: None """ - def get_songbook_key(songbook_entry): + def get_songbook_key(result): """Get the key to sort by""" - return (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry)) + return (get_natural_key(result[1]), get_natural_key(result[0]), get_natural_key(result[2])) log.debug('display results Book') self.list_view.clear() search_results.sort(key=get_songbook_key) - for songbook_entry in search_results: - # Do not display temporary songs - if songbook_entry.song.temporary: - continue - song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title) + for result in search_results: + song_detail = '%s #%s: %s' % (result[1], result[0], result[2]) song_name = QtWidgets.QListWidgetItem(song_detail) - song_name.setData(QtCore.Qt.UserRole, songbook_entry.song.id) + song_name.setData(QtCore.Qt.UserRole, result[3]) self.list_view.addItem(song_name) def display_results_topic(self, search_results): From 48c5737bd1eee1dc26ee00d2ab289d0fe8c56baa Mon Sep 17 00:00:00 2001 From: Samuel Mehrbrodt Date: Fri, 29 Apr 2016 18:43:14 +0200 Subject: [PATCH 4/9] Fix comment --- openlp/plugins/songs/lib/mediaitem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openlp/plugins/songs/lib/mediaitem.py b/openlp/plugins/songs/lib/mediaitem.py index d08be8422..9cc1bc558 100644 --- a/openlp/plugins/songs/lib/mediaitem.py +++ b/openlp/plugins/songs/lib/mediaitem.py @@ -315,7 +315,7 @@ class SongMediaItem(MediaManagerItem): """ Display the song search results in the media manager list, grouped by book and entry - :param search_results: A list of db SongBookEntry objects + :param search_results: A tuple containing (songbook entry, book name, song title, song id) :return: None """ def get_songbook_key(result): From 30dc1bd2ebf64a2ca73e3aa62587bd17617099b7 Mon Sep 17 00:00:00 2001 From: Samuel Mehrbrodt Date: Fri, 29 Apr 2016 18:44:24 +0200 Subject: [PATCH 5/9] Unused import --- openlp/plugins/songs/lib/mediaitem.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openlp/plugins/songs/lib/mediaitem.py b/openlp/plugins/songs/lib/mediaitem.py index 9cc1bc558..11deeb31d 100644 --- a/openlp/plugins/songs/lib/mediaitem.py +++ b/openlp/plugins/songs/lib/mediaitem.py @@ -21,7 +21,6 @@ ############################################################################### import logging -import re import os import shutil From 868b538dd2625caeb77ffa485c0aaf477654a329 Mon Sep 17 00:00:00 2001 From: Samuel Mehrbrodt Date: Fri, 29 Apr 2016 19:10:50 +0200 Subject: [PATCH 6/9] Fix test and add new one --- .../openlp_plugins/songs/test_mediaitem.py | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/tests/functional/openlp_plugins/songs/test_mediaitem.py b/tests/functional/openlp_plugins/songs/test_mediaitem.py index 4b9fd50ee..85911e640 100644 --- a/tests/functional/openlp_plugins/songs/test_mediaitem.py +++ b/tests/functional/openlp_plugins/songs/test_mediaitem.py @@ -23,6 +23,7 @@ This module contains tests for the lib submodule of the Songs plugin. """ from unittest import TestCase +from unittest.mock import call from PyQt5 import QtCore @@ -151,29 +152,7 @@ class TestMediaItem(TestCase, TestMixin): # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem, \ patch('openlp.core.lib.QtCore.Qt.UserRole') as MockedUserRole: - mock_search_results = [] - mock_songbook_entry = MagicMock() - mock_songbook_entry_temp = MagicMock() - mock_songbook = MagicMock() - mock_song = MagicMock() - mock_song_temp = MagicMock() - mock_songbook_entry.entry = '1' - mock_songbook_entry_temp.entry = '2' - mock_songbook.name = 'My Book' - mock_song.id = 1 - mock_song.title = 'My Song' - mock_song.sort_key = 'My Song' - mock_song.temporary = False - mock_song_temp.id = 2 - mock_song_temp.title = 'My Temporary' - mock_song_temp.sort_key = 'My Temporary' - mock_song_temp.temporary = True - mock_songbook_entry.song = mock_song - mock_songbook_entry.songbook = mock_songbook - mock_songbook_entry_temp.song = mock_song_temp - mock_songbook_entry_temp.songbook = mock_songbook - mock_search_results.append(mock_songbook_entry) - mock_search_results.append(mock_songbook_entry_temp) + mock_search_results = [('1', 'My Book', 'My Song', 1)] mock_qlist_widget = MagicMock() MockedQListWidgetItem.return_value = mock_qlist_widget @@ -183,9 +162,35 @@ class TestMediaItem(TestCase, TestMixin): # THEN: The current list view is cleared, the widget is created, and the relevant attributes set self.media_item.list_view.clear.assert_called_with() MockedQListWidgetItem.assert_called_once_with('My Book #1: My Song') - mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_songbook_entry.song.id) + mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, 1) self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget) + def songbook_natural_sorting_test(self): + """ + Test that songbooks are sorted naturally + """ + # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem + with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem: + mock_search_results = [('2', 'Thy Book', 'Thy Song', 50), + ('2', 'My Book', 'Your Song', 7), + ('10', 'My Book', 'Our Song', 12), + ('1', 'My Book', 'My Song', 1), + ('2', 'Thy Book', 'A Song', 8)] + mock_qlist_widget = MagicMock() + MockedQListWidgetItem.return_value = mock_qlist_widget + + # WHEN: I display song search results grouped by book + self.media_item.display_results_book(mock_search_results) + + # THEN: The songbooks are inserted in the right (natural) order, + # grouped first by book, then by number, then by song title + calls = [call('My Book #1: My Song'), call().setData(QtCore.Qt.UserRole, 1), + call('My Book #2: Your Song'), call().setData(QtCore.Qt.UserRole, 7), + call('My Book #10: Our Song'), call().setData(QtCore.Qt.UserRole, 12), + call('Thy Book #2: A Song'), call().setData(QtCore.Qt.UserRole, 8), + call('Thy Book #2: Thy Song'), call().setData(QtCore.Qt.UserRole, 50)] + MockedQListWidgetItem.assert_has_calls(calls) + def display_results_topic_test(self): """ Test displaying song search results grouped by topic with basic song From 9cd49f194ff16ef4a0a1b38c53efbdb7499c2425 Mon Sep 17 00:00:00 2001 From: Samuel Mehrbrodt Date: Fri, 29 Apr 2016 19:32:09 +0200 Subject: [PATCH 7/9] PEP8 --- tests/functional/openlp_plugins/songs/test_mediaitem.py | 4 ++-- tests/functional/openlp_plugins/songs/test_openlpimporter.py | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/functional/openlp_plugins/songs/test_mediaitem.py b/tests/functional/openlp_plugins/songs/test_mediaitem.py index 85911e640..12447368b 100644 --- a/tests/functional/openlp_plugins/songs/test_mediaitem.py +++ b/tests/functional/openlp_plugins/songs/test_mediaitem.py @@ -187,8 +187,8 @@ class TestMediaItem(TestCase, TestMixin): calls = [call('My Book #1: My Song'), call().setData(QtCore.Qt.UserRole, 1), call('My Book #2: Your Song'), call().setData(QtCore.Qt.UserRole, 7), call('My Book #10: Our Song'), call().setData(QtCore.Qt.UserRole, 12), - call('Thy Book #2: A Song'), call().setData(QtCore.Qt.UserRole, 8), - call('Thy Book #2: Thy Song'), call().setData(QtCore.Qt.UserRole, 50)] + call('Thy Book #2: A Song'), call().setData(QtCore.Qt.UserRole, 8), + call('Thy Book #2: Thy Song'), call().setData(QtCore.Qt.UserRole, 50)] MockedQListWidgetItem.assert_has_calls(calls) def display_results_topic_test(self): diff --git a/tests/functional/openlp_plugins/songs/test_openlpimporter.py b/tests/functional/openlp_plugins/songs/test_openlpimporter.py index 113db16e0..b78d5c43b 100644 --- a/tests/functional/openlp_plugins/songs/test_openlpimporter.py +++ b/tests/functional/openlp_plugins/songs/test_openlpimporter.py @@ -73,4 +73,3 @@ class TestOpenLPImport(TestCase): self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list') self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False, 'setMaximum on import_wizard.progress_bar should not have been called') - From 5601e61c0fbeae169773c0fcdfdc3fd50f833f41 Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Fri, 29 Apr 2016 13:25:12 -0700 Subject: [PATCH 8/9] Convert strings to python3 in mainwindow --- openlp/core/ui/maindisplay.py | 14 +++-- openlp/core/ui/mainwindow.py | 56 ++++++++++--------- .../openlp_core_lib/test_projectordb.py | 24 ++++++-- 3 files changed, 56 insertions(+), 38 deletions(-) diff --git a/openlp/core/ui/maindisplay.py b/openlp/core/ui/maindisplay.py index 079235c2d..b803df205 100644 --- a/openlp/core/ui/maindisplay.py +++ b/openlp/core/ui/maindisplay.py @@ -247,7 +247,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): """ Set up and build the output screen """ - self.log_debug('Start MainDisplay setup (live = %s)' % self.is_live) + self.log_debug('Start MainDisplay setup (live = {islive})'.format(islive=self.is_live)) self.screen = self.screens.current self.setVisible(False) Display.setup(self) @@ -288,7 +288,9 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): self.application.process_events() self.setGeometry(self.screen['size']) if animate: - self.frame.evaluateJavaScript('show_text("%s")' % slide.replace('\\', '\\\\').replace('\"', '\\\"')) + # NOTE: Verify this works with ''.format() + _text = slide.replace('\\', '\\\\').replace('\"', '\\\"') + self.frame.evaluateJavaScript('show_text("{text}")'.format(text=_text)) else: # This exists for https://bugs.launchpad.net/openlp/+bug/1016843 # For unknown reasons if evaluateJavaScript is called @@ -309,10 +311,10 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): text_prepared = expand_tags(html.escape(text)).replace('\\', '\\\\').replace('\"', '\\\"') if self.height() != self.screen['size'].height() or not self.isVisible(): shrink = True - js = 'show_alert("%s", "%s")' % (text_prepared, 'top') + js = 'show_alert("{text}", "{top}")'.format(text=text_prepared, top='top') else: shrink = False - js = 'show_alert("%s", "")' % text_prepared + js = 'show_alert("{text}", "")'.format(text=text_prepared) height = self.frame.evaluateJavaScript(js) if shrink: if text: @@ -368,7 +370,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): """ self.setGeometry(self.screen['size']) if image: - js = 'show_image("data:image/png;base64,%s");' % image + js = 'show_image("data:image/png;base64,{image}");'.format(image=image) else: js = 'show_image("");' self.frame.evaluateJavaScript(js) @@ -492,7 +494,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): :param mode: How the screen is to be hidden """ - self.log_debug('hide_display mode = %d' % mode) + self.log_debug('hide_display mode = {mode:d}'.format(mode=mode)) if self.screens.display_count == 1: # Only make visible if setting enabled. if not Settings().value('core/display on monitor'): diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 39e0ac518..a0235bb9b 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -622,11 +622,10 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): :param version: The Version to be displayed. """ log.debug('version_notice') - version_text = translate('OpenLP.MainWindow', 'Version %s of OpenLP is now available for download (you are ' - 'currently running version %s). \n\nYou can download the latest version from ' - 'http://openlp.org/.') - QtWidgets.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'), - version_text % (version, get_application_version()[u'full'])) + version_text = translate('OpenLP.MainWindow', 'Version {new} of OpenLP is now available for download (you are ' + 'currently running version {current}). \n\nYou can download the latest version from ' + 'http://openlp.org/.').format(new=version, current=get_application_version()[u'full']) + QtWidgets.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'), version_text) def show(self): """ @@ -642,7 +641,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): self.service_manager_contents.load_last_file() # This will store currently used layout preset so it remains enabled on next startup. # If any panel is enabled/disabled after preset is set, this setting is not saved. - view_mode = Settings().value('%s/view mode' % self.general_settings_section) + view_mode = Settings().value('{section}/view mode'.format(section=self.general_settings_section)) if view_mode == 'default' and Settings().value('user interface/is preset layout'): self.mode_default_item.setChecked(True) elif view_mode == 'setup' and Settings().value('user interface/is preset layout'): @@ -731,8 +730,8 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ settings = Settings() self.live_controller.main_display_set_background() - if settings.value('%s/screen blank' % self.general_settings_section): - if settings.value('%s/blank warning' % self.general_settings_section): + if settings.value('{section}/screen blank'.format(section=self.general_settings_section)): + if settings.value('{section}/blank warning'.format(section=self.general_settings_section)): QtWidgets.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Main Display Blanked'), translate('OpenLP.MainWindow', 'The Main Display has been blanked out')) @@ -924,9 +923,9 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): try: value = import_settings.value(section_key) except KeyError: - log.warning('The key "%s" does not exist (anymore), so it will be skipped.' % section_key) + log.warning('The key "{key}" does not exist (anymore), so it will be skipped.'.format(key=section_key)) if value is not None: - settings.setValue('%s' % (section_key), value) + settings.setValue('{key}'.format(key=section_key), value) now = datetime.now() settings.beginGroup(self.header_section) settings.setValue('file_imported', import_file_name) @@ -1003,9 +1002,9 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): key_value = settings.value(section_key) except KeyError: QtWidgets.QMessageBox.critical(self, translate('OpenLP.MainWindow', 'Export setting error'), - translate('OpenLP.MainWindow', 'The key "%s" does not have a default ' + translate('OpenLP.MainWindow', 'The key "{key}" does not have a default ' 'value so it will be skipped in this ' - 'export.') % section_key, + 'export.').format(key=section_key), QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Ok)) key_value = None if key_value is not None: @@ -1027,8 +1026,9 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): os.remove(temp_file) except OSError as ose: QtWidgets.QMessageBox.critical(self, translate('OpenLP.MainWindow', 'Export setting error'), - translate('OpenLP.MainWindow', 'An error occurred while exporting the ' - 'settings: %s') % ose.strerror, + translate('OpenLP.MainWindow', + 'An error occurred while exporting the ' + 'settings: {err}').format(err=ose.strerror), QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Ok)) def on_mode_default_item_clicked(self): @@ -1061,7 +1061,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ if mode: settings = Settings() - settings.setValue('%s/view mode' % self.general_settings_section, mode) + settings.setValue('{section}/view mode'.format(section=self.general_settings_section), mode) self.media_manager_dock.setVisible(media) self.service_manager_dock.setVisible(service) self.theme_manager_dock.setVisible(theme) @@ -1168,9 +1168,9 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): :param file_name: The file name of the service file. """ if modified: - title = '%s - %s*' % (UiStrings().OLPV2x, file_name) + title = '{title} - {name}*'.format(title=UiStrings().OLPV2x, name=file_name) else: - title = '%s - %s' % (UiStrings().OLPV2x, file_name) + title = '{title} - {name}'.format(title=UiStrings().OLPV2x, name=file_name) self.setWindowTitle(title) def show_status_message(self, message): @@ -1183,8 +1183,9 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ Update the default theme indicator in the status bar """ - self.default_theme_label.setText(translate('OpenLP.MainWindow', 'Default Theme: %s') % - Settings().value('themes/global theme')) + theme_name = Settings().value('themes/global theme') + self.default_theme_label.setText(translate('OpenLP.MainWindow', + 'Default Theme: {theme}').format(theme=theme_name)) def toggle_media_manager(self): """ @@ -1331,7 +1332,8 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): recent_files_to_display = existing_recent_files[0:recent_file_count] self.recent_files_menu.clear() for file_id, filename in enumerate(recent_files_to_display): - log.debug('Recent file name: %s', filename) + log.debug('Recent file name: {name}'.format(name=filename)) + # TODO: Verify ''.format() before committing action = create_action(self, '', text='&%d %s' % (file_id + 1, os.path.splitext(os.path.basename(str(filename)))[0]), data=filename, triggers=self.service_manager_contents.on_recent_service_clicked) @@ -1424,7 +1426,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ Change the data directory. """ - log.info('Changing data path to %s' % self.new_data_path) + log.info('Changing data path to {newpath}'.format(newpath=self.new_data_path)) old_data_path = str(AppLocation.get_data_path()) # Copy OpenLP data to new location if requested. self.application.set_busy_cursor() @@ -1432,17 +1434,17 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): log.info('Copying data to new path') try: self.show_status_message( - translate('OpenLP.MainWindow', 'Copying OpenLP data to new data directory location - %s ' - '- Please wait for copy to finish').replace('%s', self.new_data_path)) + translate('OpenLP.MainWindow', 'Copying OpenLP data to new data directory location - {path} ' + '- Please wait for copy to finish').format(path=self.new_data_path)) dir_util.copy_tree(old_data_path, self.new_data_path) log.info('Copy successful') except (IOError, os.error, DistutilsFileError) as why: self.application.set_normal_cursor() - log.exception('Data copy failed %s' % str(why)) + log.exception('Data copy failed {err}'.format(err=str(why))) + err_text = translate('OpenLP.MainWindow', + 'OpenLP Data directory copy failed\n\n{err}').format(err=str(why)), QtWidgets.QMessageBox.critical(self, translate('OpenLP.MainWindow', 'New Data Directory Error'), - translate('OpenLP.MainWindow', - 'OpenLP Data directory copy failed\n\n%s'). - replace('%s', str(why)), + err_text, QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Ok)) return False else: diff --git a/tests/functional/openlp_core_lib/test_projectordb.py b/tests/functional/openlp_core_lib/test_projectordb.py index 7a8a57d19..f6d52e476 100644 --- a/tests/functional/openlp_core_lib/test_projectordb.py +++ b/tests/functional/openlp_core_lib/test_projectordb.py @@ -28,7 +28,7 @@ PREREQUISITE: add_record() and get_all() functions validated. import os from unittest import TestCase -from openlp.core.lib.projector.db import Projector, ProjectorDB, ProjectorSource +from openlp.core.lib.projector.db import Manufacturer, Model, Projector, ProjectorDB, ProjectorSource from tests.functional import MagicMock, patch from tests.resources.projector.data import TEST_DB, TEST1_DATA, TEST2_DATA, TEST3_DATA @@ -82,13 +82,13 @@ class TestProjectorDB(TestCase): """ Test case for ProjectorDB """ - def setUp(self): + @patch('openlp.core.lib.projector.db.init_url') + def setUp(self, mocked_init_url): """ Set up anything necessary for all tests """ - with patch('openlp.core.lib.projector.db.init_url') as mocked_init_url: - mocked_init_url.return_value = 'sqlite:///%s' % TEST_DB - self.projector = ProjectorDB() + mocked_init_url.return_value = 'sqlite:///{db}'.format(db=TEST_DB) + self.projector = ProjectorDB() def tearDown(self): """ @@ -192,3 +192,17 @@ class TestProjectorDB(TestCase): # THEN: Projector should have the same source entry item = self.projector.get_projector_by_id(item_id) self.assertTrue(compare_source(item.source_list[0], source)) + + def manufacturer_repr_test(self): + """ + Test manufacturer class __repr__ text + """ + # GIVEN: Test object + manufacturer = Manufacturer() + + # WHEN: Name is set + manufacturer.name = 'OpenLP Test' + + # THEN: __repr__ should return a proper string + self.assertEqual(str(manufacturer), '', + 'Manufacturer.__repr__() should have returned a proper representation string') From 956c9d1653d845811aac31ad5613a0a6b51abe46 Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Fri, 29 Apr 2016 13:35:54 -0700 Subject: [PATCH 9/9] pep8 in test_openlpimporter.py --- tests/functional/openlp_plugins/songs/test_openlpimporter.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/functional/openlp_plugins/songs/test_openlpimporter.py b/tests/functional/openlp_plugins/songs/test_openlpimporter.py index 113db16e0..b78d5c43b 100644 --- a/tests/functional/openlp_plugins/songs/test_openlpimporter.py +++ b/tests/functional/openlp_plugins/songs/test_openlpimporter.py @@ -73,4 +73,3 @@ class TestOpenLPImport(TestCase): self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list') self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False, 'setMaximum on import_wizard.progress_bar should not have been called') -