openlp-core team mailing list archive

[Samuel Mehrbrodt <s.mehrbrodt@xxxxxxxxx>]

Review: Needs Fixing

The theme issue is fixed indeed.

But there are two minor issues, see inline comments.

Diff comments:

> === modified file 'openlp/core/lib/renderer.py'
> --- openlp/core/lib/renderer.py	2014-04-29 11:04:19 +0000
> +++ openlp/core/lib/renderer.py	2014-07-02 19:57:02 +0000
> @@ -59,7 +59,6 @@
>          """
>          super(Renderer, self).__init__(None)
>          # Need live behaviour if this is also working as a pseudo MainDisplay.
> -        self.is_live = True
>          self.screens = ScreenList()
>          self.theme_level = ThemeLevel.Global
>          self.global_theme_name = ''
> 
> === modified file 'openlp/core/ui/maindisplay.py'
> --- openlp/core/ui/maindisplay.py	2014-04-16 04:49:45 +0000
> +++ openlp/core/ui/maindisplay.py	2014-07-02 19:57:02 +0000
> @@ -66,11 +66,8 @@
>          if hasattr(parent, 'is_live') and parent.is_live:
>              self.is_live = True
>          if self.is_live:
> -            super(Display, self).__init__()
> -            # Overwrite the parent() method.
>              self.parent = lambda: parent
> -        else:
> -            super(Display, self).__init__(parent)
> +        super(Display, self).__init__()
>          self.controller = parent
>          self.screen = {}
>          # FIXME: On Mac OS X (tested on 10.7) the display screen is corrupt with
> 
> === modified file 'openlp/core/utils/__init__.py'
> --- openlp/core/utils/__init__.py	2014-05-02 06:42:17 +0000
> +++ openlp/core/utils/__init__.py	2014-07-02 19:57:02 +0000
> @@ -149,9 +149,9 @@
>          # If they are equal, then this tree is tarball with the source for the release. We do not want the revision
>          # number in the full version.
>          if tree_revision == tag_revision:
> -            full_version = tag_version
> +            full_version = tag_version.decode('utf-8')
>          else:
> -            full_version = '%s-bzr%s' % (tag_version, tree_revision)
> +            full_version = '%s-bzr%s' % (tag_version.decode('utf-8'), tree_revision.decode('utf-8'))
>      else:
>          # We're not running the development version, let's use the file.
>          filepath = AppLocation.get_directory(AppLocation.VersionDir)
> 
> === modified file 'resources/openlp.desktop'
> --- resources/openlp.desktop	2011-06-11 07:07:32 +0000
> +++ resources/openlp.desktop	2014-07-02 19:57:02 +0000
> @@ -1,7 +1,5 @@
>  [Desktop Entry]
>  Categories=AudioVideo;
> -Comment[de]=
> -Comment=
>  Exec=openlp %F
>  GenericName[de]=Church lyrics projection

This can also be removed I guess. That's not German.

>  GenericName=Church lyrics projection
> @@ -9,11 +7,7 @@
>  MimeType=application/x-openlp-service;
>  Name[de]=OpenLP

I would also remove this since it's no difference from the english string.
If we want to translate these things, we should put the strings in transifex also.

>  Name=OpenLP
> -Path=
>  StartupNotify=true
>  Terminal=false
>  Type=Application
> -X-DBUS-ServiceName=
> -X-DBUS-StartupType=
>  X-KDE-SubstituteUID=false
> -X-KDE-Username=
> 
> === modified file 'setup.py'
> --- setup.py	2014-04-02 18:51:21 +0000
> +++ setup.py	2014-07-02 19:57:02 +0000
> @@ -105,10 +105,12 @@
>          tag_version, tag_revision = tags[-1].split()
>      # If they are equal, then this tree is tarball with the source for the release. We do not want the revision number
>      # in the version string.
> +    tree_revision = tree_revision.strip()
> +    tag_revision = tag_revision.strip()
>      if tree_revision == tag_revision:
> -        version_string = tag_version
> +        version_string = tag_version.decode('utf-8')
>      else:
> -        version_string = '%s-bzr%s' % (tag_version, tree_revision)
> +        version_string = '%s-bzr%s' % (tag_version.decode('utf-8'), tree_revision.decode('utf-8'))
>      ver_file = open(VERSION_FILE, 'w')
>      ver_file.write(version_string)
>  except:
> @@ -152,7 +154,7 @@
>          'Operating System :: POSIX :: BSD :: FreeBSD',
>          'Operating System :: POSIX :: Linux',
>          'Programming Language :: Python',
> -        'Programming Language :: Python :: 2',
> +        'Programming Language :: Python :: 3',
>          'Topic :: Desktop Environment :: Gnome',
>          'Topic :: Desktop Environment :: K Desktop Environment (KDE)',
>          'Topic :: Multimedia',
> 
> === added file 'tests/functional/openlp_core_common/test_registrymixin.py'
> --- tests/functional/openlp_core_common/test_registrymixin.py	1970-01-01 00:00:00 +0000
> +++ tests/functional/openlp_core_common/test_registrymixin.py	2014-07-02 19:57:02 +0000
> @@ -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-2014 Raoul Snyman                                        #
> +# Portions copyright (c) 2008-2014 Tim Bentley, Gerald Britton, Jonathan      #
> +# Corwin, Samuel Findlay, Michael Gorven, Scott Guerrieri, Matthias Hub,      #
> +# Meinert Jordan, Armin Köhler, Erik Lundin, Edwin Lunando, Brian T. Meyer.   #
> +# Joshua Miller, Stevan Pettit, Andreas Preikschat, Mattias Põldaru,          #
> +# Christian Richter, Philip Ridout, Simon Scudder, Jeffrey Smith,             #
> +# Maikel Stuivenberg, Martin Thompson, Jon Tibble, Dave Warnock,              #
> +# Frode Woldsund, Martin Zibricky, Patrick Zimmermann                         #
> +# --------------------------------------------------------------------------- #
> +# 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                          #
> +###############################################################################
> +"""
> +Package to test the openlp.core.common package.
> +"""
> +import os
> +from unittest import TestCase
> +
> +from openlp.core.common import RegistryMixin, Registry
> +
> +TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '../', '..', 'resources'))
> +
> +
> +class TestRegistryMixin(TestCase):
> +
> +    def registry_mixin_missing_test(self):
> +        """
> +        Test the registry creation and its usage
> +        """
> +        # GIVEN: A new registry
> +        Registry.create()
> +
> +        # WHEN: I create a new class
> +        mock_1 = Test1()
> +
> +        # THEN: The following methods are missing
> +        self.assertEqual(len(Registry().functions_list), 0), 'The function should not be in the dict anymore.'
> +
> +    def registry_mixin_present_test(self):
> +        """
> +        Test the registry creation and its usage
> +        """
> +        # GIVEN: A new registry
> +        Registry.create()
> +
> +        # WHEN: I create a new class
> +        mock_2 = Test2()
> +
> +        # THEN: The following bootstrap methods should be present
> +        self.assertEqual(len(Registry().functions_list), 2), 'The bootstrap functions should be in the dict.'
> +
> +
> +class Test1(object):
> +    def __init__(self):
> +        pass
> +
> +
> +class Test2(RegistryMixin):
> +    def __init__(self):
> +        super(Test2, self).__init__(None)
> 
> === modified file 'tests/functional/openlp_core_lib/test_renderer.py'
> --- tests/functional/openlp_core_lib/test_renderer.py	2014-04-14 18:59:28 +0000
> +++ tests/functional/openlp_core_lib/test_renderer.py	2014-07-02 19:57:02 +0000
> @@ -65,16 +65,6 @@
>          """
>          del self.screens
>  
> -    def initial_renderer_test(self):
> -        """
> -        Test the initial renderer state
> -        """
> -        # GIVEN: A new renderer instance.
> -        renderer = Renderer()
> -        # WHEN: the default renderer is built.
> -        # THEN: The renderer should be a live controller.
> -        self.assertEqual(renderer.is_live, True, 'The base renderer should be a live controller')
> -
>      def default_screen_layout_test(self):
>          """
>          Test the default layout calculations
> 


-- 
https://code.launchpad.net/~trb143/openlp/gen1/+merge/225389
Your team OpenLP Core is requested to review the proposed merge of lp:~trb143/openlp/gen1 into lp:openlp.