← Back to team overview

openlp-core team mailing list archive

[Merge] lp:~googol/openlp/last-version-fix into lp:openlp

 

Andreas Preikschat has proposed merging lp:~googol/openlp/last-version-fix into lp:openlp.

Requested reviews:
  Andreas Preikschat (googol)

For more details, see:
https://code.launchpad.net/~googol/openlp/last-version-fix/+merge/157126

Hello,

- Use unicode instead of QDateTime for "last version test" setting
- Added three tests
- Do not subclass QThread
- Instead of using a timer to show the version update dialog I just start the Thread when everything is ready (MainWindow)

Python datetime objects are not converted by PyQt to Qt QDateTime objects. This script shows the error: http://pastebin.com/qbpKFknx (Just run the script twice.)

Now we do not subclass QThread as recommended (http://qt-project.org/doc/note_revisions/5/8/view). I removed the timer (etc) because I do not like the idea to use attributes (in such a central class) just to preserve information just for 1000ms.

http://ci.openlp.org/view/Specific%20Branch/job/OpenLP-Pull_and_Run_Interface_Tests/22/console
http://ci.openlp.org/view/Specific%20Branch/job/OpenLP-Pull_and_Run_Functional_Tests/76/console
-- 
https://code.launchpad.net/~googol/openlp/last-version-fix/+merge/157126
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/__init__.py'
--- openlp/core/__init__.py	2013-03-23 07:07:06 +0000
+++ openlp/core/__init__.py	2013-04-04 14:37:28 +0000
@@ -50,7 +50,7 @@
 from openlp.core.ui.firsttimeform import FirstTimeForm
 from openlp.core.ui.exceptionform import ExceptionForm
 from openlp.core.ui import SplashScreen
-from openlp.core.utils import AppLocation, LanguageManager, VersionThread, get_application_version
+from openlp.core.utils import AppLocation, LanguageManager, get_application_version
 
 
 __all__ = [u'OpenLP', u'main']
@@ -147,9 +147,6 @@
         self.processEvents()
         if not has_run_wizard:
             self.main_window.first_time()
-        update_check = Settings().value(u'general/update check')
-        if update_check:
-            VersionThread(self.main_window).start()
         self.main_window.is_display_blank()
         self.main_window.app_startup()
         return self.exec_()

=== modified file 'openlp/core/lib/settings.py'
--- openlp/core/lib/settings.py	2013-03-28 20:15:59 +0000
+++ openlp/core/lib/settings.py	2013-04-04 14:37:28 +0000
@@ -125,8 +125,7 @@
         u'general/ccli number': u'',
         u'general/has run wizard': False,
         u'general/language': u'[en]',
-        # This defaults to yesterday in order to force the update check to run when you've never run it before.
-        u'general/last version test': datetime.datetime.now().date() - datetime.timedelta(days=1),
+        u'general/last version test': u'',
         u'general/loop delay': 5,
         u'general/recent files': [],
         u'general/save prompt': False,

=== modified file 'openlp/core/ui/mainwindow.py'
--- openlp/core/ui/mainwindow.py	2013-03-28 20:05:46 +0000
+++ openlp/core/ui/mainwindow.py	2013-04-04 14:37:28 +0000
@@ -48,7 +48,7 @@
     MediaDockManager, ShortcutListForm, FormattingTagForm
 from openlp.core.ui.media import MediaController
 from openlp.core.utils import AppLocation, LanguageManager, add_actions, get_application_version, \
-    get_filesystem_encoding
+    get_filesystem_encoding, start_version_thread
 from openlp.core.utils.actions import ActionList, CategoryOrder
 from openlp.core.ui.firsttimeform import FirstTimeForm
 
@@ -487,7 +487,6 @@
         self.header_section = u'SettingsImport'
         self.recent_files = []
         self.timer_id = 0
-        self.timer_version_id = 0
         self.new_data_path = None
         self.copy_data = False
         Settings().set_up_default_values()
@@ -535,7 +534,7 @@
         self.application.set_busy_cursor()
         # Simple message boxes
         Registry().register_function(u'theme_update_global', self.default_theme_changed)
-        Registry().register_function(u'openlp_version_check', self.version_notice)
+        Registry().register_function(u'show_version_notice_dialog', self.show_version_notice_dialog)
         Registry().register_function(u'config_screen_changed', self.screen_changed)
         self.renderer = Renderer()
         log.info(u'Load data from Settings')
@@ -562,15 +561,17 @@
         if widget:
             widget.onFocus()
 
-    def version_notice(self, version):
+    def show_version_notice_dialog(self, version):
         """
         Notifies the user that a newer version of OpenLP is available.
         Triggered by delay thread and cannot display popup.
         """
-        log.debug(u'version_notice')
+        log.debug(u'show_version_notice_dialog')
         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/.')
-        self.version_text = version_text % (version, get_application_version()[u'full'])
+        version_text = version_text % (version, get_application_version()[u'full'])
+        QtGui.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'), version_text)
+        self.application.process_events()
 
     def show(self):
         """
@@ -590,7 +591,6 @@
             self.service_manager_contents.load_file(filename)
         elif Settings().value(self.general_settings_section + u'/auto open'):
             self.service_manager_contents.load_Last_file()
-        self.timer_version_id = self.startTimer(1000)
         view_mode = Settings().value(u'%s/view mode' % self.general_settings_section)
         if view_mode == u'default':
             self.mode_default_Item.setChecked(True)
@@ -600,6 +600,9 @@
         elif view_mode == u'live':
             self.setViewMode(False, True, False, False, True)
             self.mode_live_item.setChecked(True)
+        # Now as everything is shown, start the version check.
+        if Settings().value(u'general/update check'):
+            start_version_thread()
 
     def app_startup(self):
         """
@@ -1315,17 +1318,6 @@
             self.timer_id = 0
             self.load_progress_bar.hide()
             self.application.process_events()
-        if event.timerId() == self.timer_version_id:
-            self.timer_version_id = 0
-            # Has the thread passed some data to be displayed so display it and stop all waiting
-            if hasattr(self, u'version_text'):
-                QtGui.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'),
-                    self.version_text)
-            else:
-                # the thread has not confirmed it is running or it has not yet sent any data so lets keep waiting
-                if not hasattr(self, u'version_update_running') or self.version_update_running:
-                    self.timer_version_id = self.startTimer(1000)
-            self.application.process_events()
 
     def set_new_data_path(self, new_data_path):
         """

=== modified file 'openlp/core/utils/__init__.py'
--- openlp/core/utils/__init__.py	2013-03-04 10:55:02 +0000
+++ openlp/core/utils/__init__.py	2013-04-04 14:37:28 +0000
@@ -62,21 +62,43 @@
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]', re.UNICODE)
 
 
-class VersionThread(QtCore.QThread):
+class Worker(QtCore.QObject):
     """
-    A special Qt thread class to fetch the version of OpenLP from the website.
-    This is threaded so that it doesn't affect the loading time of OpenLP.
+    This class fetches the version of OpenLP from the website. Do not use this class. The check should be triggered by
+    calling the  :func:`~start_version_thread` function.
     """
     def run(self):
         """
-        Run the thread.
+        Run the check.
         """
-        self.sleep(1)
+        import time
+        print(u'start')
+        time.sleep(10)
+        print(u'still running')
+        time.sleep(10)
+        print(u'finished')
         log.debug(u'Version thread - run')
         app_version = get_application_version()
         version = check_latest_version(app_version)
         if LooseVersion(str(version)) > LooseVersion(str(app_version[u'full'])):
-            Registry().execute(u'openlp_version_check', u'%s' % version)
+            Registry().execute(u'show_version_notice_dialog', u'%s' % version)
+
+
+def start_version_thread():
+    """
+    This can be called to trigger the version check. This starts a thread and checks if the current version is the
+    latest.
+    """
+    # Create the thread.
+    q_thread = QtCore.QThread(Registry().get(u'main_window'))
+    worker = Worker()
+    # Create a QObject which then emits the startWork() signal.
+    q_object = QtCore.QObject()
+    QtCore.QObject.connect(q_object, QtCore.SIGNAL(u'startWork()'), worker.run)
+    worker.moveToThread(q_thread)
+    # Now start the thread.
+    q_thread.start()
+    q_object.emit(QtCore.SIGNAL(u'startWork()'))
 
 
 def _get_frozen_path(frozen_option, non_frozen_option):
@@ -184,11 +206,9 @@
     settings = Settings()
     settings.beginGroup(u'general')
     last_test = settings.value(u'last version test')
-    this_test = datetime.now().date()
+    this_test = unicode(datetime.now().date())
     settings.setValue(u'last version test', this_test)
     settings.endGroup()
-    # Tell the main window whether there will ever be data to display
-    Registry().get(u'main_window').version_update_running = last_test != this_test
     if last_test != this_test:
         if current_version[u'build']:
             req = urllib2.Request(u'http://www.openlp.org/files/nightly_version.txt')
@@ -244,8 +264,7 @@
     global IMAGES_FILTER
     if not IMAGES_FILTER:
         log.debug(u'Generating images filter.')
-        formats = [unicode(fmt)
-            for fmt in QtGui.QImageReader.supportedImageFormats()]
+        formats = map(unicode, QtGui.QImageReader.supportedImageFormats())
         visible_formats = u'(*.%s)' % u'; *.'.join(formats)
         actual_formats = u'(*.%s)' % u' *.'.join(formats)
         IMAGES_FILTER = u'%s %s %s' % (translate('OpenLP', 'Image Files'), visible_formats, actual_formats)
@@ -403,4 +422,5 @@
 
 __all__ = [u'AppLocation', u'ActionList', u'LanguageManager', u'get_application_version', u'check_latest_version',
     u'add_actions', u'get_filesystem_encoding', u'get_web_page', u'get_uno_command', u'get_uno_instance',
-    u'delete_file', u'clean_filename', u'format_time', u'locale_compare', u'locale_direct_compare']
+    u'delete_file', u'clean_filename', u'format_time', u'locale_compare', u'locale_direct_compare',
+    u'start_version_thread']

=== modified file 'tests/functional/openlp_core_utils/test_utils.py'
--- tests/functional/openlp_core_utils/test_utils.py	2012-12-07 21:38:02 +0000
+++ tests/functional/openlp_core_utils/test_utils.py	2013-04-04 14:37:28 +0000
@@ -5,7 +5,7 @@
 
 from mock import patch
 
-from openlp.core.utils import get_filesystem_encoding, _get_frozen_path
+from openlp.core.utils import get_filesystem_encoding, _get_frozen_path, clean_filename, split_filename
 
 class TestUtils(TestCase):
     """
@@ -56,3 +56,50 @@
             # THEN: The frozen parameter is returned
             assert _get_frozen_path(u'frozen', u'not frozen') == u'frozen', u'Should return "frozen"'
 
+    def split_filename_with_file_path_test(self):
+        """
+        Test the split_filename() function with a path to a file
+        """
+        # GIVEN: A path to a file.
+        file_path = u'/home/user/myfile.txt'
+        wanted_result = (u'/home/user', u'myfile.txt')
+        with patch(u'openlp.core.utils.os.path.isfile') as mocked_is_file:
+            mocked_is_file.return_value = True
+
+            # WHEN: Split the file name.
+            result = split_filename(file_path)
+
+            # THEN: A tuple should be returned.
+            assert result == wanted_result, u'A tuple with the directory and file name should have been returned.'
+
+    def split_filename_with_dir_path_test(self):
+        """
+        Test the split_filename() function with a path to a directory
+        """
+        # GIVEN: A path to a dir.
+        file_path = u'/home/user/mydir'
+        wanted_result = (u'/home/user/mydir', u'')
+        with patch(u'openlp.core.utils.os.path.isfile') as mocked_is_file:
+            mocked_is_file.return_value = False
+
+            # WHEN: Split the file name.
+            result = split_filename(file_path)
+
+            # THEN: A tuple should be returned.
+            assert result == wanted_result, \
+                u'A two-entry tuple with the directory and file name (empty) should have been returned.'
+
+
+    def clean_filename_test(self):
+        """
+        Test the clean_filename() function
+        """
+        # GIVEN: A invalid file name and the valid file name.
+        invalid_name = u'A_file_with_invalid_characters_[\\/:\*\?"<>\|\+\[\]%].py'
+        wanted_name = u'A_file_with_invalid_characters______________________.py'
+
+        # WHEN: Clean the name.
+        result = clean_filename(invalid_name)
+
+        # THEN: The file name should be cleaned.
+        assert result == wanted_name, u'The file name should not contain any special characters.'


Follow ups