From 435a299b19202f9f100445257b3a95a21aca6910 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Thu, 6 Nov 2014 23:08:56 +0200 Subject: [PATCH 1/6] First (failed) stab at using threads correctly. --- openlp/core/ui/firsttimeform.py | 59 ++++++++++++++++++++++++++------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 02d9e65f7..420131dbc 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -50,30 +50,48 @@ from .firsttimewizard import UiFirstTimeWizard, FirstTimePage log = logging.getLogger(__name__) -class ThemeScreenshotThread(QtCore.QThread): +class ThemeScreenshotWorker(QtCore.QObject): """ This thread downloads the theme screenshots. """ + screenshot_downloaded = QtCore.pyqtSignal(str, str) + + def __init__(self, config, themes_url): + """ + Set up the worker object + """ + self.config = config + self.themes_url = themes_url + self.was_download_cancelled = False + super(ThemeScreenshotWorker, self).__init__() + def run(self): """ Overridden method to run the thread. """ - themes = self.parent().config.get('themes', 'files') + themes = self.config.get('themes', 'files') themes = themes.split(',') - config = self.parent().config + config = self.config for theme in themes: # Stop if the wizard has been cancelled. - if self.parent().was_download_cancelled: + if self.was_download_cancelled: return title = config.get('theme_%s' % theme, 'title') filename = config.get('theme_%s' % theme, 'filename') screenshot = config.get('theme_%s' % theme, 'screenshot') - urllib.request.urlretrieve('%s%s' % (self.parent().themes_url, screenshot), + urllib.request.urlretrieve('%s%s' % (self.themes_url, screenshot), os.path.join(gettempdir(), 'openlp', screenshot)) - item = QtGui.QListWidgetItem(title, self.parent().themes_list_widget) - item.setData(QtCore.Qt.UserRole, filename) - item.setCheckState(QtCore.Qt.Unchecked) - item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) + # Signal that the screenshot has been downloaded + self.screenshot_downloaded.emit(title, filename) + + @QtCore.pyqtSlot(bool) + def set_download_canceled(self, toggle): + """ + Externally set if the download was canceled + + :param toggle: Set if the download was canceled or not + """ + self.was_download_cancelled = toggle class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): @@ -142,8 +160,8 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' self.themes_url = self.web + self.config.get('themes', 'directory') + '/' self.update_screen_list_combo() - self.was_download_cancelled = False self.theme_screenshot_thread = None + self.theme_screenshot_worker = None self.has_run_wizard = False self.downloading = translate('OpenLP.FirstTimeWizard', 'Downloading %s...') self.cancel_button.clicked.connect(self.on_cancel_button_clicked) @@ -187,7 +205,11 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) self.bibles_tree_widget.expandAll() # Download the theme screenshots. - self.theme_screenshot_thread = ThemeScreenshotThread(self) + self.theme_screenshot_worker = ThemeScreenshotWorker(self.config, self.themes_url) + self.theme_screenshot_worker.screenshot_downloaded.connect(self.on_screenshot_downloaded) + self.theme_screenshot_thread = QtCore.QThread(self) + self.theme_screenshot_thread.started.connect(self.theme_screenshot_worker.run) + self.theme_screenshot_worker.moveToThread(self.theme_screenshot_thread) self.theme_screenshot_thread.start() self.application.set_normal_cursor() @@ -255,13 +277,26 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): (self.last_id <= FirstTimePage.Plugins and not self.has_run_wizard): QtCore.QCoreApplication.exit() sys.exit() - self.was_download_cancelled = True + if self.theme_screenshot_worker: + self.theme_screenshot_worker.set_download_canceled(True) # Was the thread created. if self.theme_screenshot_thread: while self.theme_screenshot_thread.isRunning(): time.sleep(0.1) self.application.set_normal_cursor() + def on_screenshot_downloaded(self, title, filename): + """ + Add an item to the list when a theme has been downloaded + + :param title: The title of the theme + :param filename: The filename of the theme + """ + item = QtGui.QListWidgetItem(title, self.themes_list_widget) + item.setData(QtCore.Qt.UserRole, filename) + item.setCheckState(QtCore.Qt.Unchecked) + item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) + def on_no_internet_finish_button_clicked(self): """ Process the triggering of the "Finish" button on the No Internet page. From dec7390ef5c47a18c002ec8d7d4efa463c625a6c Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Sat, 8 Nov 2014 01:46:18 +0200 Subject: [PATCH 2/6] [bug 1389571] Use Qt threads properly, and kick off a separate thread for each download. [fix] Finally fixed the "configuring OpenLP" page by adding headers [refactor] Make the Wizard start up faster by only downloading the config file after the wizard has started --- openlp/core/ui/firsttimeform.py | 133 +++++++++++++++++------------- openlp/core/ui/firsttimewizard.py | 58 +++++++++---- 2 files changed, 117 insertions(+), 74 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 420131dbc..5d3b91762 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -52,37 +52,37 @@ log = logging.getLogger(__name__) class ThemeScreenshotWorker(QtCore.QObject): """ - This thread downloads the theme screenshots. + This thread downloads a theme's screenshot """ screenshot_downloaded = QtCore.pyqtSignal(str, str) + finished = QtCore.pyqtSignal() - def __init__(self, config, themes_url): + def __init__(self, themes_url, title, filename, screenshot): """ Set up the worker object """ - self.config = config - self.themes_url = themes_url self.was_download_cancelled = False + self.themes_url = themes_url + self.title = title + self.filename = filename + self.screenshot = screenshot super(ThemeScreenshotWorker, self).__init__() def run(self): """ Overridden method to run the thread. """ - themes = self.config.get('themes', 'files') - themes = themes.split(',') - config = self.config - for theme in themes: - # Stop if the wizard has been cancelled. - if self.was_download_cancelled: - return - title = config.get('theme_%s' % theme, 'title') - filename = config.get('theme_%s' % theme, 'filename') - screenshot = config.get('theme_%s' % theme, 'screenshot') - urllib.request.urlretrieve('%s%s' % (self.themes_url, screenshot), - os.path.join(gettempdir(), 'openlp', screenshot)) + if self.was_download_cancelled: + return + try: + urllib.request.urlretrieve('%s%s' % (self.themes_url, self.screenshot), + os.path.join(gettempdir(), 'openlp', self.screenshot)) # Signal that the screenshot has been downloaded - self.screenshot_downloaded.emit(title, filename) + self.screenshot_downloaded.emit(self.title, self.filename) + except: + log.exception('Unable to download screenshot') + finally: + self.finished.emit() @QtCore.pyqtSlot(bool) def set_download_canceled(self, toggle): @@ -105,6 +105,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): Create and set up the first time wizard. """ super(FirstTimeForm, self).__init__(parent) + self.web_access = True self.setup_ui(self) def nextId(self): @@ -112,18 +113,18 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): Determine the next page in the Wizard to go to. """ self.application.process_events() - if self.currentId() == FirstTimePage.Plugins: + if self.currentId() == FirstTimePage.Download: if not self.web_access: return FirstTimePage.NoInternet else: - return FirstTimePage.Songs + return FirstTimePage.Plugins elif self.currentId() == FirstTimePage.Progress: return -1 elif self.currentId() == FirstTimePage.NoInternet: return FirstTimePage.Progress elif self.currentId() == FirstTimePage.Themes: self.application.set_busy_cursor() - while not self.theme_screenshot_thread.isFinished(): + while not all([thread.isFinished() for thread in self.theme_screenshot_threads]): time.sleep(0.1) self.application.process_events() # Build the screenshot icons, as this can not be done in the thread. @@ -146,9 +147,14 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): :param screens: The screens detected by OpenLP """ + self.was_download_cancelled = False self.screens = screens + + def _download_index(self): + """ + Download the configuration file and kick off the theme screenshot download threads + """ # check to see if we have web access - self.web = 'http://openlp.org/files/frw/' self.config = ConfigParser() user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() self.web_access = get_web_page('%s%s' % (self.web, 'download.cfg'), header=('User-Agent', user_agent)) @@ -160,24 +166,10 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' self.themes_url = self.web + self.config.get('themes', 'directory') + '/' self.update_screen_list_combo() - self.theme_screenshot_thread = None - self.theme_screenshot_worker = None + self.theme_screenshot_threads = [] + self.theme_screenshot_workers = [] self.has_run_wizard = False self.downloading = translate('OpenLP.FirstTimeWizard', 'Downloading %s...') - self.cancel_button.clicked.connect(self.on_cancel_button_clicked) - self.no_internet_finish_button.clicked.connect(self.on_no_internet_finish_button_clicked) - self.currentIdChanged.connect(self.on_current_id_changed) - Registry().register_function('config_screen_changed', self.update_screen_list_combo) - - def set_defaults(self): - """ - Set up display at start of theme edit. - """ - self.restart() - check_directory_exists(os.path.join(gettempdir(), 'openlp')) - self.no_internet_finish_button.setVisible(False) - # Check if this is a re-run of the wizard. - self.has_run_wizard = Settings().value('core/has run wizard') # Sort out internet access for downloads if self.web_access: songs = self.config.get('songs', 'languages') @@ -204,13 +196,36 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): item.setCheckState(0, QtCore.Qt.Unchecked) item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) self.bibles_tree_widget.expandAll() - # Download the theme screenshots. - self.theme_screenshot_worker = ThemeScreenshotWorker(self.config, self.themes_url) - self.theme_screenshot_worker.screenshot_downloaded.connect(self.on_screenshot_downloaded) - self.theme_screenshot_thread = QtCore.QThread(self) - self.theme_screenshot_thread.started.connect(self.theme_screenshot_worker.run) - self.theme_screenshot_worker.moveToThread(self.theme_screenshot_thread) - self.theme_screenshot_thread.start() + # Download the theme screenshots + themes = self.config.get('themes', 'files').split(',') + for theme in themes: + title = self.config.get('theme_%s' % theme, 'title') + filename = self.config.get('theme_%s' % theme, 'filename') + screenshot = self.config.get('theme_%s' % theme, 'screenshot') + worker = ThemeScreenshotWorker(self.themes_url, title, filename, screenshot) + self.theme_screenshot_workers.append(worker) + worker.screenshot_downloaded.connect(self.on_screenshot_downloaded) + thread = QtCore.QThread(self) + self.theme_screenshot_threads.append(thread) + thread.started.connect(worker.run) + worker.finished.connect(thread.quit) + worker.moveToThread(thread) + thread.start() + + def set_defaults(self): + """ + Set up display at start of theme edit. + """ + self.restart() + self.web = 'http://openlp.org/files/frw/' + self.cancel_button.clicked.connect(self.on_cancel_button_clicked) + self.no_internet_finish_button.clicked.connect(self.on_no_internet_finish_button_clicked) + self.currentIdChanged.connect(self.on_current_id_changed) + Registry().register_function('config_screen_changed', self.update_screen_list_combo) + self.no_internet_finish_button.setVisible(False) + # Check if this is a re-run of the wizard. + self.has_run_wizard = Settings().value('core/has run wizard') + check_directory_exists(os.path.join(gettempdir(), 'openlp')) self.application.set_normal_cursor() def update_screen_list_combo(self): @@ -230,12 +245,20 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.application.process_events() if page_id != -1: self.last_id = page_id - if page_id == FirstTimePage.Plugins: + if page_id == FirstTimePage.Download: + self.back_button.setVisible(False) + self.next_button.setVisible(False) # Set the no internet page text. if self.has_run_wizard: self.no_internet_label.setText(self.no_internet_text) else: self.no_internet_label.setText(self.no_internet_text + self.cancel_wizard_text) + self.application.set_busy_cursor() + self._download_index() + self.application.set_normal_cursor() + self.back_button.setVisible(False) + self.next_button.setVisible(True) + self.next() elif page_id == FirstTimePage.Defaults: self.theme_combo_box.clear() for index in range(self.themes_list_widget.count()): @@ -255,15 +278,10 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): elif page_id == FirstTimePage.NoInternet: self.back_button.setVisible(False) self.next_button.setVisible(False) + self.cancel_button.setVisible(False) self.no_internet_finish_button.setVisible(True) - if self.has_run_wizard: - self.cancel_button.setVisible(False) elif page_id == FirstTimePage.Progress: self.application.set_busy_cursor() - self.repaint() - self.application.process_events() - # Try to give the wizard a chance to redraw itself - time.sleep(0.2) self._pre_wizard() self._perform_wizard() self._post_wizard() @@ -273,15 +291,16 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): """ Process the triggering of the cancel button. """ - if self.last_id == FirstTimePage.NoInternet or \ - (self.last_id <= FirstTimePage.Plugins and not self.has_run_wizard): + self.was_download_cancelled = True + if not self.has_run_wizard: QtCore.QCoreApplication.exit() sys.exit() - if self.theme_screenshot_worker: - self.theme_screenshot_worker.set_download_canceled(True) + if self.theme_screenshot_workers: + for worker in self.theme_screenshot_workers: + worker.set_download_canceled(True) # Was the thread created. - if self.theme_screenshot_thread: - while self.theme_screenshot_thread.isRunning(): + if self.theme_screenshot_threads: + while any([thread.isRunning() for thread in self.theme_screenshot_threads]): time.sleep(0.1) self.application.set_normal_cursor() diff --git a/openlp/core/ui/firsttimewizard.py b/openlp/core/ui/firsttimewizard.py index 4e83a3e74..792e36a50 100644 --- a/openlp/core/ui/firsttimewizard.py +++ b/openlp/core/ui/firsttimewizard.py @@ -41,13 +41,14 @@ class FirstTimePage(object): An enumeration class with each of the pages of the wizard. """ Welcome = 0 - Plugins = 1 + Download = 1 NoInternet = 2 - Songs = 3 - Bibles = 4 - Themes = 5 - Defaults = 6 - Progress = 7 + Plugins = 3 + Songs = 4 + Bibles = 5 + Themes = 6 + Defaults = 7 + Progress = 8 class UiFirstTimeWizard(object): @@ -78,6 +79,33 @@ class UiFirstTimeWizard(object): self.next_button = self.button(QtGui.QWizard.NextButton) self.back_button = self.button(QtGui.QWizard.BackButton) add_welcome_page(first_time_wizard, ':/wizards/wizard_firsttime.bmp') + # The download page + self.download_page = QtGui.QWizardPage() + self.download_page.setObjectName('download_page') + self.download_layout = QtGui.QVBoxLayout(self.download_page) + self.download_layout.setMargin(48) + self.download_layout.setObjectName('download_layout') + self.download_label = QtGui.QLabel(self.download_page) + self.download_label.setObjectName('download_label') + self.download_layout.addWidget(self.download_label) + self.download_progress_bar = QtGui.QProgressBar(self.download_page) + self.download_progress_bar.setMinimum(0) + self.download_progress_bar.setMaximum(0) + self.download_progress_bar.setValue(0) + self.download_progress_bar.setObjectName('download_progress_bar') + self.download_layout.addWidget(self.download_progress_bar) + first_time_wizard.setPage(FirstTimePage.Download, self.download_page) + # The "you don't have an internet connection" page. + self.no_internet_page = QtGui.QWizardPage() + self.no_internet_page.setObjectName('no_internet_page') + self.no_internet_layout = QtGui.QVBoxLayout(self.no_internet_page) + self.no_internet_layout.setContentsMargins(50, 30, 50, 40) + self.no_internet_layout.setObjectName('no_internet_layout') + self.no_internet_label = QtGui.QLabel(self.no_internet_page) + self.no_internet_label.setWordWrap(True) + self.no_internet_label.setObjectName('no_internet_label') + self.no_internet_layout.addWidget(self.no_internet_label) + first_time_wizard.setPage(FirstTimePage.NoInternet, self.no_internet_page) # The plugins page self.plugin_page = QtGui.QWizardPage() self.plugin_page.setObjectName('plugin_page') @@ -120,17 +148,6 @@ class UiFirstTimeWizard(object): self.alert_check_box.setObjectName('alert_check_box') self.plugin_layout.addWidget(self.alert_check_box) first_time_wizard.setPage(FirstTimePage.Plugins, self.plugin_page) - # The "you don't have an internet connection" page. - self.no_internet_page = QtGui.QWizardPage() - self.no_internet_page.setObjectName('no_internet_page') - self.no_internet_layout = QtGui.QVBoxLayout(self.no_internet_page) - self.no_internet_layout.setContentsMargins(50, 30, 50, 40) - self.no_internet_layout.setObjectName('no_internet_layout') - self.no_internet_label = QtGui.QLabel(self.no_internet_page) - self.no_internet_label.setWordWrap(True) - self.no_internet_label.setObjectName('no_internet_label') - self.no_internet_layout.addWidget(self.no_internet_label) - first_time_wizard.setPage(FirstTimePage.NoInternet, self.no_internet_page) # The song samples page self.songs_page = QtGui.QWizardPage() self.songs_page.setObjectName('songs_page') @@ -221,6 +238,10 @@ class UiFirstTimeWizard(object): translate('OpenLP.FirstTimeWizard', 'This wizard will help you to configure OpenLP for initial use. ' 'Click the %s button below to start.') % clean_button_text(first_time_wizard.buttonText(QtGui.QWizard.NextButton))) + self.download_page.setTitle(translate('OpenLP.FirstTimeWizard', 'Downloading Resource Index')) + self.download_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Please wait while the resource index is ' + 'downloaded.')) + self.download_label.setText(translate('OpenLP.FirstTimeWizard', 'Downloading resource index...')) self.plugin_page.setTitle(translate('OpenLP.FirstTimeWizard', 'Activate required Plugins')) self.plugin_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Select the Plugins you wish to use. ')) self.songs_check_box.setText(translate('OpenLP.FirstTimeWizard', 'Songs')) @@ -257,5 +278,8 @@ class UiFirstTimeWizard(object): 'Set up default settings to be used by OpenLP.')) self.display_label.setText(translate('OpenLP.FirstTimeWizard', 'Default output display:')) self.theme_label.setText(translate('OpenLP.FirstTimeWizard', 'Select default theme:')) + self.progress_page.setTitle(translate('OpenLP.FirstTimeWizard', 'Downloading and Configuring')) + self.progress_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Please wait while resources are downloaded ' + 'and OpenLP is configured.')) self.progress_label.setText(translate('OpenLP.FirstTimeWizard', 'Starting configuration process...')) first_time_wizard.setButtonText(QtGui.QWizard.CustomButton1, translate('OpenLP.FirstTimeWizard', 'Finish')) From 4f4c76ae8b061ed2719b5ba328ea6bb1721f16c0 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Sat, 8 Nov 2014 21:25:06 +0200 Subject: [PATCH 3/6] [fix] Don't exit OpenLP when you cancel the FRW from the Tools menu --- openlp/core/__init__.py | 3 +++ openlp/core/ui/firsttimeform.py | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index 37a713d38..9f5c69770 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -122,6 +122,9 @@ class OpenLP(OpenLPMixin, QtGui.QApplication): ftw.initialize(screens) if ftw.exec_() == QtGui.QDialog.Accepted: Settings().setValue('core/has run wizard', True) + elif ftw.was_download_cancelled: + QtCore.QCoreApplication.exit() + sys.exit() # Correct stylesheet bugs application_stylesheet = '' if not Settings().value('advanced/alternate rows'): diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 5d3b91762..06a238fed 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -292,9 +292,6 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): Process the triggering of the cancel button. """ self.was_download_cancelled = True - if not self.has_run_wizard: - QtCore.QCoreApplication.exit() - sys.exit() if self.theme_screenshot_workers: for worker in self.theme_screenshot_workers: worker.set_download_canceled(True) From 14377f332963d6066695e743aa261e0afa98cf4b Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Sat, 8 Nov 2014 23:15:25 +0200 Subject: [PATCH 4/6] [refactor] Renamed "was_download_cancelled" to "was_cancelled" [refactor] Put some more stuff back into initalise() where it belongs [refactor] Rewrote old tests, wrote new tests --- openlp/core/__init__.py | 2 +- openlp/core/ui/firsttimeform.py | 17 +-- openlp/core/ui/mainwindow.py | 2 +- .../openlp_core_ui/test_firsttimeform.py | 137 +++++++++++++----- 4 files changed, 110 insertions(+), 48 deletions(-) diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index 9f5c69770..503aadee3 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -122,7 +122,7 @@ class OpenLP(OpenLPMixin, QtGui.QApplication): ftw.initialize(screens) if ftw.exec_() == QtGui.QDialog.Accepted: Settings().setValue('core/has run wizard', True) - elif ftw.was_download_cancelled: + elif ftw.was_cancelled: QtCore.QCoreApplication.exit() sys.exit() # Correct stylesheet bugs diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 06a238fed..75f661c3a 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -31,7 +31,6 @@ This module contains the first time wizard. """ import logging import os -import sys import time import urllib.request import urllib.parse @@ -105,7 +104,6 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): Create and set up the first time wizard. """ super(FirstTimeForm, self).__init__(parent) - self.web_access = True self.setup_ui(self) def nextId(self): @@ -147,8 +145,12 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): :param screens: The screens detected by OpenLP """ - self.was_download_cancelled = False self.screens = screens + self.web_access = True + self.was_cancelled = False + self.theme_screenshot_threads = [] + self.theme_screenshot_workers = [] + self.has_run_wizard = False def _download_index(self): """ @@ -166,9 +168,6 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' self.themes_url = self.web + self.config.get('themes', 'directory') + '/' self.update_screen_list_combo() - self.theme_screenshot_threads = [] - self.theme_screenshot_workers = [] - self.has_run_wizard = False self.downloading = translate('OpenLP.FirstTimeWizard', 'Downloading %s...') # Sort out internet access for downloads if self.web_access: @@ -291,7 +290,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): """ Process the triggering of the cancel button. """ - self.was_download_cancelled = True + self.was_cancelled = True if self.theme_screenshot_workers: for worker in self.theme_screenshot_workers: worker.set_download_canceled(True) @@ -333,7 +332,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): url_file = urllib.request.urlopen(url) filename = open(f_path, "wb") # Download until finished or canceled. - while not self.was_download_cancelled: + while not self.was_cancelled: data = url_file.read(block_size) if not data: break @@ -342,7 +341,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self._download_progress(block_count, block_size) filename.close() # Delete file if cancelled, it may be a partial file. - if self.was_download_cancelled: + if self.was_cancelled: os.remove(f_path) def _build_theme_screenshots(self): diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 825a12889..48fc1fcbf 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -690,7 +690,7 @@ class MainWindow(QtGui.QMainWindow, Ui_MainWindow, RegistryProperties): first_run_wizard = FirstTimeForm(self) first_run_wizard.initialize(ScreenList()) first_run_wizard.exec_() - if first_run_wizard.was_download_cancelled: + if first_run_wizard.was_cancelled: return self.application.set_busy_cursor() self.first_time() diff --git a/tests/functional/openlp_core_ui/test_firsttimeform.py b/tests/functional/openlp_core_ui/test_firsttimeform.py index ed7a7a9e8..da5305bda 100644 --- a/tests/functional/openlp_core_ui/test_firsttimeform.py +++ b/tests/functional/openlp_core_ui/test_firsttimeform.py @@ -30,6 +30,7 @@ Package to test the openlp.core.ui.firsttimeform package. """ from configparser import ConfigParser +import os from unittest import TestCase from openlp.core.common import Registry @@ -55,52 +56,114 @@ class TestFirstTimeForm(TestCase, TestMixin): def setUp(self): self.setup_application() self.app.setApplicationVersion('0.0') + # Set up a fake "set_normal_cursor" method since we're not dealing with an actual OpenLP application object + self.app.set_normal_cursor = lambda: None Registry.create() Registry().register('application', self.app) - def basic_initialise_test(self): + def initialise_test(self): """ - Test if we can intialise the FirstTimeForm without a config file + Test if we can intialise the FirstTimeForm """ - # GIVEN: A mocked get_web_page, a First Time Wizard and an expected screen object - with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: - first_time_form = FirstTimeForm(None) - expected_screens = MagicMock() - expected_web_url = 'http://openlp.org/files/frw/' - expected_user_agent = 'OpenLP/0.0' - mocked_get_web_page.return_value = None + # GIVEN: A First Time Wizard and an expected screen object + frw = FirstTimeForm(None) + expected_screens = MagicMock() - # WHEN: The First Time Wizard is initialised - first_time_form.initialize(expected_screens) + # WHEN: The First Time Wizard is initialised + frw.initialize(expected_screens) - # THEN: The First Time Form web configuration file should be accessible and parseable - self.assertEqual(expected_screens, first_time_form.screens, 'The screens should be correct') - self.assertEqual(expected_web_url, first_time_form.web, 'The base path of the URL should be correct') - self.assertIsInstance(first_time_form.config, ConfigParser, 'The config object should be a ConfigParser') - mocked_get_web_page.assert_called_with(expected_web_url + 'download.cfg', - header=('User-Agent', expected_user_agent)) + # THEN: The screens should be set up, and the default values initialised + self.assertEqual(expected_screens, frw.screens, 'The screens should be correct') + self.assertTrue(frw.web_access, 'The default value of self.web_access should be True') + self.assertFalse(frw.was_cancelled, 'The default value of self.was_cancelled should be False') + self.assertListEqual([], frw.theme_screenshot_threads, 'The list of threads should be empty') + self.assertListEqual([], frw.theme_screenshot_workers, 'The list of workers should be empty') + self.assertFalse(frw.has_run_wizard, 'has_run_wizard should be False') - def config_initialise_test(self): + def set_defaults_test(self): """ - Test if we can intialise the FirstTimeForm with a config file + Test that the default values are set when set_defaults() is run """ - # GIVEN: A mocked get_web_page, a First Time Wizard and an expected screen object - with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: - first_time_form = FirstTimeForm(None) - expected_web_url = 'http://openlp.org/files/frw/' - expected_songs_url = 'http://example.com/frw/songs/' - expected_bibles_url = 'http://example.com/frw/bibles/' - expected_themes_url = 'http://example.com/frw/themes/' - expected_user_agent = 'OpenLP/0.0' - mocked_get_web_page.return_value.read.return_value = FAKE_CONFIG + # GIVEN: An initialised FRW and a whole lot of stuff mocked out + frw = FirstTimeForm(None) + frw.initialize(MagicMock()) + with patch.object(frw, 'restart') as mocked_restart, \ + patch.object(frw, 'cancel_button') as mocked_cancel_button, \ + patch.object(frw, 'no_internet_finish_button') as mocked_no_internet_finish_btn, \ + patch.object(frw, 'currentIdChanged') as mocked_currentIdChanged, \ + patch.object(Registry, 'register_function') as mocked_register_function, \ + patch('openlp.core.ui.firsttimeform.Settings') as MockedSettings, \ + patch('openlp.core.ui.firsttimeform.gettempdir') as mocked_gettempdir, \ + patch('openlp.core.ui.firsttimeform.check_directory_exists') as mocked_check_directory_exists, \ + patch.object(frw.application, 'set_normal_cursor') as mocked_set_normal_cursor: + mocked_settings = MagicMock() + mocked_settings.value.return_value = True + MockedSettings.return_value = mocked_settings + mocked_gettempdir.return_value = 'temp' + expected_temp_path = os.path.join('temp', 'openlp') - # WHEN: The First Time Wizard is initialised - first_time_form.initialize(MagicMock()) + # WHEN: The set_defaults() method is run + frw.set_defaults() - # THEN: The First Time Form web configuration file should be accessible and parseable - self.assertIsInstance(first_time_form.config, ConfigParser, 'The config object should be a ConfigParser') - mocked_get_web_page.assert_called_with(expected_web_url + 'download.cfg', - header=('User-Agent', expected_user_agent)) - self.assertEqual(expected_songs_url, first_time_form.songs_url, 'The songs URL should be correct') - self.assertEqual(expected_bibles_url, first_time_form.bibles_url, 'The bibles URL should be correct') - self.assertEqual(expected_themes_url, first_time_form.themes_url, 'The themes URL should be correct') + # THEN: The default values should have been set + mocked_restart.assert_called_with() + self.assertEqual('http://openlp.org/files/frw/', frw.web, 'The default URL should be set') + mocked_cancel_button.clicked.connect.assert_called_with(frw.on_cancel_button_clicked) + mocked_no_internet_finish_btn.clicked.connect.assert_called_with(frw.on_no_internet_finish_button_clicked) + mocked_currentIdChanged.connect.assert_called_with(frw.on_current_id_changed) + mocked_register_function.assert_called_with('config_screen_changed', frw.update_screen_list_combo) + mocked_no_internet_finish_btn.setVisible.assert_called_with(False) + mocked_settings.value.assert_called_with('core/has run wizard') + mocked_gettempdir.assert_called_with() + mocked_check_directory_exists.assert_called_with(expected_temp_path) + mocked_set_normal_cursor.assert_called_with() + + def update_screen_list_combo_test(self): + """ + Test that the update_screen_list_combo() method works correctly + """ + # GIVEN: A mocked Screen() object and an initialised First Run Wizard and a mocked display_combo_box + expected_screen_list = ['Screen 1', 'Screen 2'] + mocked_screens = MagicMock() + mocked_screens.get_screen_list.return_value = expected_screen_list + frw = FirstTimeForm(None) + frw.initialize(mocked_screens) + with patch.object(frw, 'display_combo_box') as mocked_display_combo_box: + mocked_display_combo_box.count.return_value = 2 + + # WHEN: update_screen_list_combo() is called + frw.update_screen_list_combo() + + # THEN: The combobox should have been updated + mocked_display_combo_box.clear.assert_called_with() + mocked_screens.get_screen_list.assert_called_with() + mocked_display_combo_box.addItems.assert_called_with(expected_screen_list) + mocked_display_combo_box.count.assert_called_with() + mocked_display_combo_box.setCurrentIndex.assert_called_with(1) + + def on_cancel_button_clicked_test(self): + """ + Test that the cancel button click slot shuts down the threads correctly + """ + # GIVEN: A FRW, some mocked threads and workers (that isn't quite done) and other mocked stuff + frw = FirstTimeForm(None) + frw.initialize(MagicMock()) + mocked_worker = MagicMock() + mocked_thread = MagicMock() + mocked_thread.isRunning.side_effect = [True, False] + frw.theme_screenshot_workers.append(mocked_worker) + frw.theme_screenshot_threads.append(mocked_thread) + with patch('openlp.core.ui.firsttimeform.time') as mocked_time, \ + patch.object(frw.application, 'set_normal_cursor') as mocked_set_normal_cursor: + + # WHEN: on_cancel_button_clicked() is called + frw.on_cancel_button_clicked() + + # THEN: The right things should be called in the right order + self.assertTrue(frw.was_cancelled, 'The was_cancelled property should have been set to True') + mocked_worker.set_download_canceled.assert_called_with(True) + mocked_thread.isRunning.assert_called_with() + self.assertEqual(2, mocked_thread.isRunning.call_count, 'isRunning() should have been called twice') + mocked_time.sleep.assert_called_with(0.1) + self.assertEqual(1, mocked_time.sleep.call_count, 'sleep() should have only been called once') + mocked_set_normal_cursor.assert_called_with() From a517ed5fea0e6e876b927cf44843477ceb0d612c Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 26 Nov 2014 22:02:48 +0200 Subject: [PATCH 5/6] [refactor] Remove the progress bar on the first download page, and update the label to be slightly more descriptive [fix] Hide the back button on the plugins page so that folks can't go back and re-download the config file --- openlp/core/ui/firsttimeform.py | 13 ++++++++++++- openlp/core/ui/firsttimewizard.py | 9 ++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 75f661c3a..148cb0f60 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -159,7 +159,10 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): # check to see if we have web access self.config = ConfigParser() user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() - self.web_access = get_web_page('%s%s' % (self.web, 'download.cfg'), header=('User-Agent', user_agent)) + self.application.process_events() + self.web_access = get_web_page('%s%s' % (self.web, 'download.cfg'), header=('User-Agent', user_agent), + update_openlp=True) + self.application.process_events() if self.web_access: files = self.web_access.read() self.config.read_string(files.decode()) @@ -168,12 +171,14 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' self.themes_url = self.web + self.config.get('themes', 'directory') + '/' self.update_screen_list_combo() + self.application.process_events() self.downloading = translate('OpenLP.FirstTimeWizard', 'Downloading %s...') # Sort out internet access for downloads if self.web_access: songs = self.config.get('songs', 'languages') songs = songs.split(',') for song in songs: + self.application.process_events() title = self.config.get('songs_%s' % song, 'title') filename = self.config.get('songs_%s' % song, 'filename') item = QtGui.QListWidgetItem(title, self.songs_list_widget) @@ -183,11 +188,13 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): bible_languages = self.config.get('bibles', 'languages') bible_languages = bible_languages.split(',') for lang in bible_languages: + self.application.process_events() language = self.config.get('bibles_%s' % lang, 'title') lang_item = QtGui.QTreeWidgetItem(self.bibles_tree_widget, [language]) bibles = self.config.get('bibles_%s' % lang, 'translations') bibles = bibles.split(',') for bible in bibles: + self.application.process_events() title = self.config.get('bible_%s' % bible, 'title') filename = self.config.get('bible_%s' % bible, 'filename') item = QtGui.QTreeWidgetItem(lang_item, [title]) @@ -195,9 +202,11 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): item.setCheckState(0, QtCore.Qt.Unchecked) item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) self.bibles_tree_widget.expandAll() + self.application.process_events() # Download the theme screenshots themes = self.config.get('themes', 'files').split(',') for theme in themes: + self.application.process_events() title = self.config.get('theme_%s' % theme, 'title') filename = self.config.get('theme_%s' % theme, 'filename') screenshot = self.config.get('theme_%s' % theme, 'screenshot') @@ -279,6 +288,8 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): self.next_button.setVisible(False) self.cancel_button.setVisible(False) self.no_internet_finish_button.setVisible(True) + elif page_id == FirstTimePage.Plugins: + self.back_button.setVisible(False) elif page_id == FirstTimePage.Progress: self.application.set_busy_cursor() self._pre_wizard() diff --git a/openlp/core/ui/firsttimewizard.py b/openlp/core/ui/firsttimewizard.py index 792e36a50..964fdd7a6 100644 --- a/openlp/core/ui/firsttimewizard.py +++ b/openlp/core/ui/firsttimewizard.py @@ -88,12 +88,6 @@ class UiFirstTimeWizard(object): self.download_label = QtGui.QLabel(self.download_page) self.download_label.setObjectName('download_label') self.download_layout.addWidget(self.download_label) - self.download_progress_bar = QtGui.QProgressBar(self.download_page) - self.download_progress_bar.setMinimum(0) - self.download_progress_bar.setMaximum(0) - self.download_progress_bar.setValue(0) - self.download_progress_bar.setObjectName('download_progress_bar') - self.download_layout.addWidget(self.download_progress_bar) first_time_wizard.setPage(FirstTimePage.Download, self.download_page) # The "you don't have an internet connection" page. self.no_internet_page = QtGui.QWizardPage() @@ -241,7 +235,8 @@ class UiFirstTimeWizard(object): self.download_page.setTitle(translate('OpenLP.FirstTimeWizard', 'Downloading Resource Index')) self.download_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Please wait while the resource index is ' 'downloaded.')) - self.download_label.setText(translate('OpenLP.FirstTimeWizard', 'Downloading resource index...')) + self.download_label.setText(translate('OpenLP.FirstTimeWizard', 'Please wait while OpenLP downloads the ' + 'resource index file...')) self.plugin_page.setTitle(translate('OpenLP.FirstTimeWizard', 'Activate required Plugins')) self.plugin_page.setSubTitle(translate('OpenLP.FirstTimeWizard', 'Select the Plugins you wish to use. ')) self.songs_check_box.setText(translate('OpenLP.FirstTimeWizard', 'Songs')) From ba8114609337b8119550bb85a14c8218ab8357f9 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Sun, 14 Dec 2014 22:37:24 +0200 Subject: [PATCH 6/6] Fix a bug inadvertently introduced in the merge conflict. --- openlp/core/ui/firsttimeform.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 33b721af4..6a77e5bc9 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -113,16 +113,17 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): """ Returns the id of the next FirstTimePage to go to based on enabled plugins """ - # The songs plugin is enabled if FirstTimePage.Welcome < self.currentId() < FirstTimePage.Songs and self.songs_check_box.isChecked(): - print('Go for songs! %r' % self.songs_check_box.isChecked()) + # If the songs plugin is enabled then go to the songs page return FirstTimePage.Songs - # The Bibles plugin is enabled elif FirstTimePage.Welcome < self.currentId() < FirstTimePage.Bibles and self.bible_check_box.isChecked(): + # Otherwise, if the Bibles plugin is enabled then go to the Bibles page return FirstTimePage.Bibles elif FirstTimePage.Welcome < self.currentId() < FirstTimePage.Themes: + # Otherwise, if the current page is somewhere between the Welcome and the Themes pages, go to the themes return FirstTimePage.Themes else: + # If all else fails, go to the next page return self.currentId() + 1 def nextId(self): @@ -134,7 +135,9 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): if not self.web_access: return FirstTimePage.NoInternet else: - return self.get_next_page_id() + return FirstTimePage.Plugins + elif self.currentId() == FirstTimePage.Plugins: + return self.get_next_page_id() elif self.currentId() == FirstTimePage.Progress: return -1 elif self.currentId() == FirstTimePage.NoInternet: @@ -414,6 +417,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties): for index, theme in enumerate(themes): screenshot = self.config.get('theme_%s' % theme, 'screenshot') item = self.themes_list_widget.item(index) + # if item: item.setIcon(build_icon(os.path.join(gettempdir(), 'openlp', screenshot))) def _get_file_size(self, url):