Merge lp:~trb143/openlp/bug-850254 into lp:openlp

Proposed by Tim Bentley
Status: Merged
Merged at revision: 1841
Proposed branch: lp:~trb143/openlp/bug-850254
Merge into: lp:openlp
Diff against target: 112 lines (+33/-10)
2 files modified
openlp/core/ui/slidecontroller.py (+24/-6)
openlp/plugins/remotes/lib/httpserver.py (+9/-4)
To merge this branch: bzr merge lp:~trb143/openlp/bug-850254
Reviewer Review Type Date Requested Status
Raoul Snyman Approve
Review via email: mp+85934@code.launchpad.net

This proposal supersedes a proposal from 2011-12-15.

Description of the change

This is the core changes to handle the remote events for hiding and showing screens

To post a comment you must log in.
Revision history for this message
Jonathan Corwin (j-corwin) wrote : Posted in a previous version of this proposal

Too many blank lines 56-58
There is an elif on 53 but an if on 55.

I'm not sure about the term "status" in httpserver, it doesn't seem descriptive enough to me, as lots of things could have a status. Perhaps displaystatus or something?

review: Needs Fixing
Revision history for this message
Jonathan Corwin (j-corwin) wrote : Posted in a previous version of this proposal

Oh, and you seem to be sneaking a feature in along with the fix...

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

81 + (r'^/api/status$', self.status),
82 + (r'^/api/changeStatus/(blank|theme|desktop)$', self.changeStatus),

As Jonathan said, "status" is a little too generic...

(r'^/api/display-status$', self.displayStatus),
(r'^/api/display-status/(blank|theme|desktop)$', self.changeDisplayStatus),

That should work.

review: Needs Fixing
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Can you also change the method names to make them a little more descriptive please? Like...

  def displayStatus(self):
      """
      Obtain the display status of system.
      """

  def changeDisplayStatus(self, action):
      """
      Toggle the display of the system including the status button.
      """

review: Needs Fixing
Revision history for this message
Jonathan Corwin (j-corwin) wrote : Posted in a previous version of this proposal

Just looking at httpserver.py, we already have a method /api/display
It currently only supports hide or show.
Can't we just extend it to support 'theme' and 'desktop' too?

I'm also not convinced about just toggling it, this could just cause chaos if both the PC user and remote user do it at the same time as it could just result in a flicker. I think the user should tell the app what he wants to do, and then the app does it, even if the end result is to do nothing.

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Will fix up the comments.

The API needs changing but if we change it when have to update the clients at the same time which will be a problem. The old API should be deprecated and removed in March.

The current situation is worse as the when and android can set the screen to blank but then have no view of what it is and the display is just changed without the button being set.
This change will make it possible to make the button states consistent across all clients.

Revision history for this message
Jonathan Corwin (j-corwin) wrote : Posted in a previous version of this proposal

But if I write a client that wants to blank the screen, then I want to be able to tell OpenLP to blank the screen. Changing it to a 'toggle' means that I press the button, and the application has no clue as to what OpenLP will do.

I have no issue with a method to get the current display status and for this to then update the ui afterwards with the state, or even poll regularly. But the remote app needs to be able to tell OpenLP exactly what they want to do.

99% of the time the operator will be able to look up at the projector screen and see what the current state of play is if they really badly want to know.

Revision history for this message
Jonathan Corwin (j-corwin) wrote : Posted in a previous version of this proposal

In fact we could just change the existing 'poll' call to get the display status

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

I agree with Jonathan, I think we should probably move away from a "toggle" mentality, and move to a "hide|theme|desktop|show" idea - then we can keep the API the same, just adding new items, keeping the API backwards compatible.

Then we can show 4 buttons in the toolbar, and using the poll event, indicate which button is active.

Further thoughts/ideas/comments/etc ?

Revision history for this message
Raoul Snyman (raoul-snyman) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'openlp/core/ui/slidecontroller.py'
--- openlp/core/ui/slidecontroller.py 2011-12-09 12:35:18 +0000
+++ openlp/core/ui/slidecontroller.py 2011-12-15 19:01:50 +0000
@@ -95,7 +95,7 @@
95 u'Edit Song',95 u'Edit Song',
96 ]96 ]
97 self.nextPreviousList = [97 self.nextPreviousList = [
98 u'Previous Slide', 98 u'Previous Slide',
99 u'Next Slide'99 u'Next Slide'
100 ]100 ]
101 self.timer_id = 0101 self.timer_id = 0
@@ -114,8 +114,8 @@
114 self.typeLabel.setText(UiStrings().Live)114 self.typeLabel.setText(UiStrings().Live)
115 self.split = 1115 self.split = 1
116 self.typePrefix = u'live'116 self.typePrefix = u'live'
117 self.keypress_queue = deque() 117 self.keypress_queue = deque()
118 self.keypress_loop = False 118 self.keypress_loop = False
119 else:119 else:
120 self.typeLabel.setText(UiStrings().Preview)120 self.typeLabel.setText(UiStrings().Preview)
121 self.split = 0121 self.split = 0
@@ -187,7 +187,7 @@
187 translate('OpenLP.SlideController', 'Hide'), self.toolbar))187 translate('OpenLP.SlideController', 'Hide'), self.toolbar))
188 self.blankScreen = shortcut_action(self.hideMenu, u'blankScreen',188 self.blankScreen = shortcut_action(self.hideMenu, u'blankScreen',
189 [QtCore.Qt.Key_Period], self.onBlankDisplay,189 [QtCore.Qt.Key_Period], self.onBlankDisplay,
190 u':/slides/slide_blank.png', False, 190 u':/slides/slide_blank.png', False,
191 unicode(UiStrings().LiveToolbar))191 unicode(UiStrings().LiveToolbar))
192 self.blankScreen.setText(192 self.blankScreen.setText(
193 translate('OpenLP.SlideController', 'Blank Screen'))193 translate('OpenLP.SlideController', 'Blank Screen'))
@@ -412,6 +412,9 @@
412 QtCore.QObject.connect(Receiver.get_receiver(),412 QtCore.QObject.connect(Receiver.get_receiver(),
413 QtCore.SIGNAL(u'slidecontroller_live_spin_delay'),413 QtCore.SIGNAL(u'slidecontroller_live_spin_delay'),
414 self.receiveSpinDelay)414 self.receiveSpinDelay)
415 QtCore.QObject.connect(Receiver.get_receiver(),
416 QtCore.SIGNAL(u'slidecontroller_toggle_display'),
417 self.toggleDisplay)
415 self.toolbar.makeWidgetsInvisible(self.loopList)418 self.toolbar.makeWidgetsInvisible(self.loopList)
416 else:419 else:
417 QtCore.QObject.connect(self.previewListWidget,420 QtCore.QObject.connect(self.previewListWidget,
@@ -570,6 +573,21 @@
570 self.display.setVisible(False)573 self.display.setVisible(False)
571 self.mediaController.video_stop([self])574 self.mediaController.video_stop([self])
572575
576 def toggleDisplay(self, action):
577 """
578 Toggle the display settings triggered from remote messages.
579 """
580 if action == u'blank' or action == u'hide':
581 self.onBlankDisplay(True)
582 elif action == u'theme':
583 self.onThemeDisplay(True)
584 elif action == u'desktop':
585 self.onHideDisplay(True)
586 elif action == u'show':
587 self.onBlankDisplay(False)
588 self.onThemeDisplay(False)
589 self.onHideDisplay(False)
590
573 def servicePrevious(self):591 def servicePrevious(self):
574 """592 """
575 Live event to select the previous service item from the service manager.593 Live event to select the previous service item from the service manager.
@@ -618,8 +636,8 @@
618 self.previewSizeChanged()636 self.previewSizeChanged()
619 self.previewDisplay.setup()637 self.previewDisplay.setup()
620 serviceItem = ServiceItem()638 serviceItem = ServiceItem()
621 self.previewDisplay.webView.setHtml(build_html(serviceItem, 639 self.previewDisplay.webView.setHtml(build_html(serviceItem,
622 self.previewDisplay.screen, None, self.isLive, None, 640 self.previewDisplay.screen, None, self.isLive, None,
623 plugins=PluginManager.get_instance().plugins))641 plugins=PluginManager.get_instance().plugins))
624 self.mediaController.setup_display(self.previewDisplay)642 self.mediaController.setup_display(self.previewDisplay)
625 if self.serviceItem:643 if self.serviceItem:
626644
=== modified file 'openlp/plugins/remotes/lib/httpserver.py'
--- openlp/plugins/remotes/lib/httpserver.py 2011-12-14 22:39:52 +0000
+++ openlp/plugins/remotes/lib/httpserver.py 2011-12-15 19:01:50 +0000
@@ -249,7 +249,7 @@
249 (r'^/api/poll$', self.poll),249 (r'^/api/poll$', self.poll),
250 (r'^/api/controller/(live|preview)/(.*)$', self.controller),250 (r'^/api/controller/(live|preview)/(.*)$', self.controller),
251 (r'^/api/service/(.*)$', self.service),251 (r'^/api/service/(.*)$', self.service),
252 (r'^/api/display/(hide|show)$', self.display),252 (r'^/api/display/(hide|show|blank|theme|desktop)$', self.display),
253 (r'^/api/alert$', self.alert),253 (r'^/api/alert$', self.alert),
254 (r'^/api/plugin/(search)$', self.pluginInfo),254 (r'^/api/plugin/(search)$', self.pluginInfo),
255 (r'^/api/(.*)/search$', self.search),255 (r'^/api/(.*)/search$', self.search),
@@ -401,7 +401,13 @@
401 u'item': self.parent.current_item._uuid \401 u'item': self.parent.current_item._uuid \
402 if self.parent.current_item else u'',402 if self.parent.current_item else u'',
403 u'twelve':QtCore.QSettings().value(403 u'twelve':QtCore.QSettings().value(
404 u'remotes/twelve hour', QtCore.QVariant(True)).toBool()404 u'remotes/twelve hour', QtCore.QVariant(True)).toBool(),
405 u'blank': self.parent.plugin.liveController.blankScreen.\
406 isChecked(),
407 u'theme': self.parent.plugin.liveController.themeScreen.\
408 isChecked(),
409 u'display': self.parent.plugin.liveController.desktopScreen.\
410 isChecked()
405 }411 }
406 return HttpResponse(json.dumps({u'results': result}),412 return HttpResponse(json.dumps({u'results': result}),
407 {u'Content-Type': u'application/json'})413 {u'Content-Type': u'application/json'})
@@ -413,8 +419,7 @@
413 ``action``419 ``action``
414 This is the action, either ``hide`` or ``show``.420 This is the action, either ``hide`` or ``show``.
415 """421 """
416 event = u'live_display_%s' % action422 Receiver.send_message(u'slidecontroller_toggle_display', action)
417 Receiver.send_message(event, HideMode.Blank)
418 return HttpResponse(json.dumps({u'results': {u'success': True}}),423 return HttpResponse(json.dumps({u'results': {u'success': True}}),
419 {u'Content-Type': u'application/json'})424 {u'Content-Type': u'application/json'})
420425