← Back to team overview

openlp-core team mailing list archive

[Merge] lp:~raoul-snyman/openlp/bug-1389571 into lp:openlp

 

Raoul Snyman has proposed merging lp:~raoul-snyman/openlp/bug-1389571 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1389571 in OpenLP: "Crash while downloading thumbnails"
  https://bugs.launchpad.net/openlp/+bug/1389571

For more details, see:
https://code.launchpad.net/~raoul-snyman/openlp/bug-1389571/+merge/241189

[bug #1389571] Fixed by using threads the right way.
- Refactor FRW to show up faster by downloading config file after show
- Fix a long-standing UI bug in FRW by supplying title and subtitle to config page

Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/bug-1389571 (revision 2443)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/761/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/699/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/643/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/582/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/191/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/396/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/270/
-- 
https://code.launchpad.net/~raoul-snyman/openlp/bug-1389571/+merge/241189
Your team OpenLP Core is requested to review the proposed merge of lp:~raoul-snyman/openlp/bug-1389571 into lp:openlp.
=== modified file 'openlp/core/__init__.py'
--- openlp/core/__init__.py	2014-10-22 20:47:47 +0000
+++ openlp/core/__init__.py	2014-11-08 21:29:24 +0000
@@ -122,6 +122,9 @@
             ftw.initialize(screens)
             if ftw.exec_() == QtGui.QDialog.Accepted:
                 Settings().setValue('core/has run wizard', True)
+            elif ftw.was_cancelled:
+                QtCore.QCoreApplication.exit()
+                sys.exit()
         # Correct stylesheet bugs
         application_stylesheet = ''
         if not Settings().value('advanced/alternate rows'):

=== modified file 'openlp/core/ui/firsttimeform.py'
--- openlp/core/ui/firsttimeform.py	2014-11-01 10:38:33 +0000
+++ openlp/core/ui/firsttimeform.py	2014-11-08 21:29:24 +0000
@@ -31,7 +31,6 @@
 """
 import logging
 import os
-import sys
 import time
 import urllib.request
 import urllib.parse
@@ -50,30 +49,48 @@
 log = logging.getLogger(__name__)
 
 
-class ThemeScreenshotThread(QtCore.QThread):
-    """
-    This thread downloads the theme screenshots.
-    """
+class ThemeScreenshotWorker(QtCore.QObject):
+    """
+    This thread downloads a theme's screenshot
+    """
+    screenshot_downloaded = QtCore.pyqtSignal(str, str)
+    finished = QtCore.pyqtSignal()
+
+    def __init__(self, themes_url, title, filename, screenshot):
+        """
+        Set up the worker object
+        """
+        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.parent().config.get('themes', 'files')
-        themes = themes.split(',')
-        config = self.parent().config
-        for theme in themes:
-            # Stop if the wizard has been cancelled.
-            if self.parent().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),
-                                       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)
+        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(self.title, self.filename)
+        except:
+            log.exception('Unable to download screenshot')
+        finally:
+            self.finished.emit()
+
+    @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):
@@ -94,18 +111,18 @@
         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.
@@ -129,8 +146,17 @@
         :param screens: The screens detected by OpenLP
         """
         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):
+        """
+        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))
@@ -142,24 +168,7 @@
             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.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')
@@ -186,9 +195,36 @@
                     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_thread = ThemeScreenshotThread(self)
-            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):
@@ -208,12 +244,20 @@
         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()):
@@ -233,15 +277,10 @@
         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()
@@ -251,17 +290,28 @@
         """
         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):
-            QtCore.QCoreApplication.exit()
-            sys.exit()
-        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)
         # 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()
 
+    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.
@@ -282,7 +332,7 @@
         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
@@ -291,7 +341,7 @@
             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):

=== modified file 'openlp/core/ui/firsttimewizard.py'
--- openlp/core/ui/firsttimewizard.py	2014-10-31 19:47:36 +0000
+++ openlp/core/ui/firsttimewizard.py	2014-11-08 21:29:24 +0000
@@ -41,13 +41,14 @@
     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 @@
         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 @@
         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 @@
             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 @@
                                                  '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'))

=== modified file 'openlp/core/ui/mainwindow.py'
--- openlp/core/ui/mainwindow.py	2014-10-23 22:16:38 +0000
+++ openlp/core/ui/mainwindow.py	2014-11-08 21:29:24 +0000
@@ -690,7 +690,7 @@
         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()

=== modified file 'tests/functional/openlp_core_ui/test_firsttimeform.py'
--- tests/functional/openlp_core_ui/test_firsttimeform.py	2014-10-22 20:43:05 +0000
+++ tests/functional/openlp_core_ui/test_firsttimeform.py	2014-11-08 21:29:24 +0000
@@ -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 @@
     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):
-        """
-        Test if we can intialise the FirstTimeForm without a config file
-        """
-        # 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
-
-            # WHEN: The First Time Wizard is initialised
-            first_time_form.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))
-
-    def config_initialise_test(self):
-        """
-        Test if we can intialise the FirstTimeForm with a config file
-        """
-        # 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
-
-            # WHEN: The First Time Wizard is initialised
-            first_time_form.initialize(MagicMock())
-
-            # 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')
+    def initialise_test(self):
+        """
+        Test if we can intialise the FirstTimeForm
+        """
+        # GIVEN: A First Time Wizard and an expected screen object
+        frw = FirstTimeForm(None)
+        expected_screens = MagicMock()
+
+        # WHEN: The First Time Wizard is initialised
+        frw.initialize(expected_screens)
+
+        # 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 set_defaults_test(self):
+        """
+        Test that the default values are set when set_defaults() is run
+        """
+        # 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 set_defaults() method is run
+            frw.set_defaults()
+
+            # 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()


References