openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'openlp/plugins/remotes/html/stage.js'
> --- openlp/plugins/remotes/html/stage.js	2015-01-18 13:39:21 +0000
> +++ openlp/plugins/remotes/html/stage.js	2015-02-27 19:43:59 +0000
> @@ -96,6 +96,10 @@
>      $("#tag" + OpenLP.currentTags[OpenLP.currentSlide]).addClass("currenttag");
>      var slide = OpenLP.currentSlides[OpenLP.currentSlide];
>      var text = "";
> +    if (!slide) {
> +        // This will happen if no live view ist started
> +        return;
> +    }
>      // use title if available
>      if (slide["title"]) {
>          text = slide["title"];
> @@ -137,14 +141,26 @@
>    },
>    updateClock: function(data) {
>      var div = $("#clock");
> -    var t = new Date();
> -    var h = t.getHours();
> -    if (data.results.twelve && h > 12)
> -      h = h - 12;
> -    var m = t.getMinutes();
> -    if (m < 10)
> -      m = '0' + m + '';
> -    div.html(h + ":" + m);
> +    switch (data.results.timeSource) {
> +        case 0: // use client's time
> +            var t = new Date();
> +            var h = t.getHours();
> +            if (data.results.twelve && h > 12)
> +            h = h - 12;
> +            var m = t.getMinutes();
> +            if (m < 10)
> +            m = '0' + m + '';
> +            div.html(h + ":" + m);
> +            div.show();
> +            break;
> +        case 1: // use server time
> +            div.html(data.results.serverTime);
> +            div.show();
> +            break;
> +        default: // no time display
> +            div.html("");
> +            div.hide();
> +    }
>    },
>    pollServer: function () {
>      $.getJSON(
> 
> === modified file 'openlp/plugins/remotes/lib/__init__.py'
> --- openlp/plugins/remotes/lib/__init__.py	2015-01-18 13:39:21 +0000
> +++ openlp/plugins/remotes/lib/__init__.py	2015-02-27 19:43:59 +0000
> @@ -20,8 +20,17 @@
>  # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
>  ###############################################################################
>  
> +class TimeSourceType(object):
> +    """
> +    An enumeration for the time source
> +    """
> +    Client = 0
> +    Server = 1
> +    NoTime = 2
> +
>  from .remotetab import RemoteTab
>  from .httprouter import HttpRouter
>  from .httpserver import OpenLPServer
>  
>  __all__ = ['RemoteTab', 'OpenLPServer', 'HttpRouter']
> +
> 
> === modified file 'openlp/plugins/remotes/lib/httprouter.py'
> --- openlp/plugins/remotes/lib/httprouter.py	2015-02-14 09:12:35 +0000
> +++ openlp/plugins/remotes/lib/httprouter.py	2015-02-27 19:43:59 +0000
> @@ -112,6 +112,7 @@
>  import re
>  import urllib.request
>  import urllib.error
> +import time
>  from urllib.parse import urlparse, parse_qs
>  
>  from mako.template import Template
> @@ -426,20 +427,28 @@
>          """
>          Poll OpenLP to determine the current slide number and item name.
>          """
> +        twelve = Settings().value('remotes/twelve hour')
> +        if twelve:
> +            time_string = time.strftime("%I:%M")
> +        else:
> +            time_string = time.strftime("%H:%M")
>          result = {
>              'service': self.service_manager.service_id,
>              'slide': self.live_controller.selected_row or 0,
>              'item': self.live_controller.service_item.unique_identifier if self.live_controller.service_item else '',
> -            'twelve': Settings().value('remotes/twelve hour'),
> +            'twelve': twelve,
> +            'timeSource': Settings().value('remotes/time source'),
> +            'serverTime': time_string,
>              'blank': self.live_controller.blank_screen.isChecked(),
>              'theme': self.live_controller.theme_screen.isChecked(),
>              'display': self.live_controller.desktop_screen.isChecked(),
> -            'version': 2,
> +            'version': 3,
>              'isSecure': Settings().value(self.settings_section + '/authentication enabled'),
>              'isAuthorised': self.authorised
>          }
>          self.do_json_header()
> -        return json.dumps({'results': result}).encode()
> +        # need sort_keys=True for the unit test
> +        return json.dumps({'results': result}, sort_keys=True).encode()

If you just need it for the tests, do something better:

Load the JSON back into a dictionary, and use self.assertDictEqual()

>  
>      def main_poll(self):
>          """
> 
> === modified file 'openlp/plugins/remotes/lib/remotetab.py'
> --- openlp/plugins/remotes/lib/remotetab.py	2015-02-14 12:35:37 +0000
> +++ openlp/plugins/remotes/lib/remotetab.py	2015-02-27 19:43:59 +0000
> @@ -26,6 +26,7 @@
>  
>  from openlp.core.common import AppLocation, Settings, translate
>  from openlp.core.lib import SettingsTab, build_icon
> +from openlp.plugins.remotes.lib import TimeSourceType
>  
>  ZERO_URL = '0.0.0.0'
>  
> @@ -52,6 +53,12 @@
>                                         self))
>          self.address_edit.setObjectName('address_edit')
>          self.server_settings_layout.addRow(self.address_label, self.address_edit)
> +        self.time_source_label = QtGui.QLabel(self.server_settings_group_box)
> +        self.time_source_label.setObjectName('time_source_label')
> +        self.time_source_combo_box = QtGui.QComboBox(self.server_settings_group_box)
> +        self.time_source_combo_box.addItems(['', '', ''])
> +        self.time_source_combo_box.setObjectName('time_source_combo_box')
> +        self.server_settings_layout.addRow(self.time_source_label, self.time_source_combo_box)
>          self.twelve_hour_check_box = QtGui.QCheckBox(self.server_settings_group_box)
>          self.twelve_hour_check_box.setObjectName('twelve_hour_check_box')
>          self.server_settings_layout.addRow(self.twelve_hour_check_box)
> @@ -158,6 +165,7 @@
>          self.qr_layout.addWidget(self.qr_description_label)
>          self.left_layout.addStretch()
>          self.right_layout.addStretch()
> +        self.time_source_combo_box.activated.connect(self.on_time_source_combo_box_changed)
>          self.twelve_hour_check_box.stateChanged.connect(self.on_twelve_hour_check_box_changed)
>          self.thumbnails_check_box.stateChanged.connect(self.on_thumbnails_check_box_changed)
>          self.address_edit.textChanged.connect(self.set_urls)
> @@ -172,6 +180,10 @@
>          self.remote_url_label.setText(translate('RemotePlugin.RemoteTab', 'Remote URL:'))
>          self.stage_url_label.setText(translate('RemotePlugin.RemoteTab', 'Stage view URL:'))
>          self.live_url_label.setText(translate('RemotePlugin.RemoteTab', 'Live view URL:'))
> +        self.time_source_label.setText(translate('RemotePlugin.RemoteTab', 'Time source:'))
> +        self.time_source_combo_box.setItemText(TimeSourceType.Client, translate('RemotePlugin.RemoteTab', 'Client'))
> +        self.time_source_combo_box.setItemText(TimeSourceType.Server, translate('RemotePlugin.RemoteTab', 'Server'))
> +        self.time_source_combo_box.setItemText(TimeSourceType.NoTime, translate('RemotePlugin.RemoteTab', 'None'))
>          self.twelve_hour_check_box.setText(translate('RemotePlugin.RemoteTab', 'Display stage time in 12h format'))
>          self.thumbnails_check_box.setText(translate('RemotePlugin.RemoteTab',
>                                                      'Show thumbnails of non-text slides in remote and stage view.'))
> @@ -237,6 +249,8 @@
>          self.port_spin_box.setValue(Settings().value(self.settings_section + '/port'))
>          self.https_port_spin_box.setValue(Settings().value(self.settings_section + '/https port'))
>          self.address_edit.setText(Settings().value(self.settings_section + '/ip address'))
> +        self.time_source = Settings().value(self.settings_section + '/time source')
> +        self.time_source_combo_box.setCurrentIndex(self.time_source)
>          self.twelve_hour = Settings().value(self.settings_section + '/twelve hour')
>          self.twelve_hour_check_box.setChecked(self.twelve_hour)
>          self.thumbnails = Settings().value(self.settings_section + '/thumbnails')
> @@ -271,6 +285,7 @@
>          Settings().setValue(self.settings_section + '/https port', self.https_port_spin_box.value())
>          Settings().setValue(self.settings_section + '/https enabled', self.https_settings_group_box.isChecked())
>          Settings().setValue(self.settings_section + '/ip address', self.address_edit.text())
> +        Settings().setValue(self.settings_section + '/time source', self.time_source)
>          Settings().setValue(self.settings_section + '/twelve hour', self.twelve_hour)
>          Settings().setValue(self.settings_section + '/thumbnails', self.thumbnails)
>          Settings().setValue(self.settings_section + '/authentication enabled', self.user_login_group_box.isChecked())
> @@ -278,6 +293,9 @@
>          Settings().setValue(self.settings_section + '/password', self.password.text())
>          self.generate_icon()
>  
> +    def on_time_source_combo_box_changed(self):
> +        self.time_source = self.time_source_combo_box.currentIndex();
> +        
>      def on_twelve_hour_check_box_changed(self, check_state):
>          """
>          Toggle the 12 hour check box.
> 
> === modified file 'openlp/plugins/remotes/remoteplugin.py'
> --- openlp/plugins/remotes/remoteplugin.py	2015-01-18 13:39:21 +0000
> +++ openlp/plugins/remotes/remoteplugin.py	2015-02-27 19:43:59 +0000
> @@ -25,11 +25,12 @@
>  from PyQt4 import QtGui
>  
>  from openlp.core.lib import Plugin, StringContent, translate, build_icon
> -from openlp.plugins.remotes.lib import RemoteTab, OpenLPServer
> +from openlp.plugins.remotes.lib import RemoteTab, OpenLPServer, TimeSourceType
>  
>  log = logging.getLogger(__name__)
>  
>  __default_settings__ = {
> +    'remotes/time source': TimeSourceType.Client,
>      'remotes/twelve hour': True,
>      'remotes/port': 4316,
>      'remotes/https port': 4317,
> @@ -41,7 +42,6 @@
>      'remotes/thumbnails': True
>  }
>  
> -
>  class RemotesPlugin(Plugin):
>      log.info('Remote Plugin loaded')
>  
> 
> === modified file 'tests/functional/openlp_plugins/remotes/test_remotetab.py'
> --- tests/functional/openlp_plugins/remotes/test_remotetab.py	2015-01-18 13:39:21 +0000
> +++ tests/functional/openlp_plugins/remotes/test_remotetab.py	2015-02-27 19:43:59 +0000
> @@ -30,10 +30,12 @@
>  
>  from openlp.core.common import Settings
>  from openlp.plugins.remotes.lib.remotetab import RemoteTab
> +from openlp.plugins.remotes.lib import TimeSourceType
>  from tests.functional import patch
>  from tests.helpers.testmixin import TestMixin
>  
>  __default_settings__ = {
> +    'remotes/time source': TimeSourceType.Client,
>      'remotes/twelve hour': True,
>      'remotes/port': 4316,
>      'remotes/https port': 4317,
> 
> === modified file 'tests/functional/openlp_plugins/remotes/test_router.py'
> --- tests/functional/openlp_plugins/remotes/test_router.py	2015-02-16 20:56:39 +0000
> +++ tests/functional/openlp_plugins/remotes/test_router.py	2015-02-27 19:43:59 +0000
> @@ -23,16 +23,19 @@
>  This module contains tests for the lib submodule of the Remotes plugin.
>  """
>  import os
> +import time
>  import urllib.request
>  from unittest import TestCase
>  from openlp.core.common import Settings, Registry
>  from openlp.core.ui import ServiceManager
> +from openlp.plugins.remotes.lib import TimeSourceType
>  from openlp.plugins.remotes.lib.httpserver import HttpRouter
>  from urllib.parse import urlparse
>  from tests.functional import MagicMock, patch, mock_open
>  from tests.helpers.testmixin import TestMixin
>  
>  __default_settings__ = {
> +    'remotes/time source': TimeSourceType.Client,
>      'remotes/twelve hour': True,
>      'remotes/port': 4316,
>      'remotes/https port': 4317,
> @@ -50,6 +53,7 @@
>      """
>      Test the functions in the :mod:`lib` module.
>      """
> +    maxDiff = None
>      def setUp(self):
>          """
>          Create the UI
> @@ -340,3 +344,36 @@
>  
>              # THEN: service_manager.next_item() should have been called
>              self.assertTrue(mocked_previous_item.called, 'previous_item() should have been called in service_manager')
> +
> +    def poll_test(self):
> +        """
> +        Test the poll method with default settings
> +        """
> +        # GIVEN: initial setup and mocks
> +        Registry.create()
> +        Registry().register('live_controller', MagicMock)
> +        router = HttpRouter()
> +        router.settings_section = 'remotes'
> +        service_manager = ServiceManager()
> +        with patch.object(router, 'do_json_header') as mocked_json_header, \
> +                patch('openlp.plugins.remotes.lib.httprouter.time') as mocked_time:
> +            router.live_controller.service_item = MagicMock()
> +            router.live_controller.blank_screen = MagicMock()
> +            router.live_controller.theme_screen = MagicMock()
> +            router.live_controller.desktop_screen = MagicMock()
> +            mocked_time.strftime = MagicMock(return_value = '09:36')
> +            service_manager.service_id = '0'
> +            router.live_controller.selected_row = 0
> +            router.live_controller.service_item.unique_identifier = ''
> +            router.live_controller.blank_screen.isChecked.return_value = False
> +            router.live_controller.theme_screen.isChecked.return_value = False
> +            router.live_controller.desktop_screen.isChecked.return_value = False
> +            router.authorised = False
> +
> +            json_test_result = b'{"results": {"blank": false, "display": false, "isAuthorised": false, "isSecure": false, "item": "", "serverTime": "09:36", "service": "0", "slide": 0, "theme": false, "timeSource": 0, "twelve": true, "version": 3}}'
> +
> +            # WHEN: poll called
> +            results = router.poll()
> +
> +            #THEN: the encoded JSON should be equal to the above test string
> +            self.assertEqual(results, json_test_result, 'JSON should be equal')
> 


-- 
https://code.launchpad.net/~oliwee/openlp/selecttimesource/+merge/251322
Your team OpenLP Core is subscribed to branch lp:openlp.