Merge lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank into lp:openlp

Proposed by Azaziah on 2016-04-20
Status: Superseded
Proposed branch: lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank
Merge into: lp:openlp
Diff against target: 263 lines (+90/-4)
8 files modified
openlp/core/common/settings.py (+3/-0)
openlp/core/lib/mediamanageritem.py (+3/-0)
openlp/core/ui/generaltab.py (+7/-0)
openlp/core/ui/maindisplay.py (+1/-1)
openlp/core/ui/mainwindow.py (+3/-0)
openlp/core/ui/slidecontroller.py (+26/-2)
openlp/plugins/presentations/lib/messagelistener.py (+5/-1)
tests/functional/openlp_core_ui/test_slidecontroller.py (+42/-0)
To merge this branch: bzr merge lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank
Reviewer Review Type Date Requested Status
Raoul Snyman 2016-04-20 Needs Fixing on 2016-04-21
Tim Bentley 2016-04-20 Pending
Tomas Groth 2016-04-20 Pending
Review via email: mp+292407@code.launchpad.net

This proposal supersedes a proposal from 2016-04-17.

This proposal has been superseded by a proposal from 2016-04-27.

Description of the change

This branch introduces the functionality of unblanking
display from Blank to Black/Theme/Desktop for:

a) Clicking slide in "Live panel"
b) Next/Previous shortcuts (Green arrows)
c) Go to verse x.
d) When starting automatic playback (To end or Loop)
Also added "Unblank display when changing slide in Live" to advanced
options tab for disabling/enabling this behavior for a-c.

Additionally this branch also includes fix for bug
https://bugs.launchpad.net/openlp/+bug/1531691
Do note that this branch does not fix this for Escape item blanking,
creating yet an another Escape exclusive bug.

-----------------------------------------------------------------------------
The only reason Escape item has been a good alternative for other
blank to methods is the functionality of resuming Live by clicking
slides and the fact it worked in single screen scenarios.
I can’t see any reason why it should not be removed after this branch
is merged since the single screen issue was already fixed earlier.

-----------------------------------------------------------------------------
Added 3 tests for checking display is re-blanked if it was
blanked before re-processing edited Live item.
Also fixed the issue where Next/Previous slide does not
unblank display for PowerPoint/Impress.

-----------------------------------------------------------------------------
Better fix for bug where display is unblanked on editing current live item.
This now sets a hidden setting to true while processing Live item and then changes it back to false. Display is thus not unblanked at all during the
process. (Old fix showed the edited slide for a small time)

Downside: All the new tests were based on the old
method and thus they were removed.

Fixed bug 1462420 (Double clicking preview adds items to service unlimited times)
- Added a hidden setting for controlling this behavior.
  It is reset if any item is sent to preview from library.
- Sending the same item to service multiple times is still
  possible by using the "Add button (icon)"
-----------------------------------------------------------------------------
In this re-proposal:

- Added two tests for checking if doubleclicking preview should
  add item to service or send it to live.

lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank (revision 2647)
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/1467/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-02-Functional-Tests/1383/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-03-Interface-Tests/1321/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1123/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/714/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05a-Code_Analysis/781/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05b-Test_Coverage/649/

To post a comment you must log in.
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

Just tested a bit.
You have introduced the "Click live slide to unblank" setting, but a "Unblank display when adding new item" also exists in the general tab. You should probably move yours to be under the exiting one to keep similar settings in the same place. Currently your code doesn't honor the "Unblank display when adding new item", which it will have to do. As it is now the item goes live no matter if the setting is enabled or not.
Also the new setting should be "false" as default, this is new behavior, so users should enabled it if they want it.
Also added a code-comment below.

review: Needs Fixing
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

Hi Tomas and thank you for your comments!

"Unblank display when adding new item" also exists in the general tab. You should probably move yours to be under the exiting one to keep similar settings in the same place. Currently your code doesn't honor the "Unblank display when adding new item

# Done and Done, now also honoring double clicking preview

Also the new setting should be "false" as default, this is new behavior, so users should enabled it if they want it.

# Done
# Currently this is default behavior for Escape item and probably easier
to use than our current blank to modes. I think this and some other settings
should eventually be changed to be more user friendly by default.

Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

Looks good now. As you mention in one of your source-comments the previous slide is shortly visible when display is unblanked. Don't know if it can be helped.
Added a comment to the code below.
Besides that, you need tests :)

review: Needs Fixing
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

Thanks again,

if by previous slide being shortly visible you mean it happens once you change slide inside the Live Panel
and the transition effect is applied, yes that can be changed. I've heard people complaining there's no transition effects when unblanking or changing items so maybe it should be left as it is since it would mean loosing effects.

If you were talking about this happening on song edit:
Think it would be possible but would require quite a effort,
maybe we can let it pass for the time being, it's still mostly fixed.

I really suck at writing tests, is one enough for a rookie?

I also replied to the code comment.

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

Looks good to me. Nice tests.

review: Approve
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Couple of tests and some tests are needed.

review: Needs Fixing
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

I need to start working on tests...

Fixed the diff comments.

Raoul Snyman (raoul-snyman) wrote :

Check my comment. I'll try to look at this more in-depth over the weekend and give you more feedback.

review: Needs Fixing
Azaziah (suutari-olli) wrote :

> Check my comment. I'll try to look at this more in-depth over the weekend and
> give you more feedback.

Thank you for checking this out, I've replied to your comment.

I'm away from my coding station until next week, so no new commits
(if required) will be made until then.

I wish you all the best for your day.

Azaziah (suutari-olli) wrote :

I noticed the comment was not saved, so here it is. ^^ Sorry about that :/

Azaziah (suutari-olli) wrote :

I commented on replace live item function and resetting the setting.

2648. By Azaziah on 2016-04-27

Added this to program startup code, should replace_service_manager_item ever crash the program.

Settings().setValue('core/is live item edited and replaced', False)

2649. By Azaziah on 2016-06-13

merged trunk on 14.6.16

2650. By Azaziah on 2016-06-26

- Turned the two new hidden settings into registry flags

2651. By Azaziah on 2016-06-26

Comment cleanup / Improvements

2652. By Azaziah on 2016-06-26

- Merged trunk on 27.6.16

2653. By Azaziah on 2016-07-14

- Tried to make the new text work with the registry changes but failed.
 > Test is broken, do not merge!

2654. By Azaziah on 2016-07-14

Merged trunk on 14.7.16

2655. By Azaziah on 2016-07-14

pep8 fixes, test still broken.

2656. By Azaziah on 2016-07-16

Fixed the tests.

2657. By Azaziah on 2016-07-16

- Reduced comments
- Removed unrequired reg_value from test.

2658. By Azaziah on 2016-07-17

- Fixed the issue where items sent to Preview from Service may cause tracebacks.

2659. By Azaziah on 2016-07-17

- Moved the 2nd new registry flag from one init to an another init which also has the 1st

2660. By Azaziah on 2016-07-17

- Moved them to another init since something broke

2661. By Azaziah on 2016-07-17

- removed one unrequired if statement.

2662. By Azaziah on 2016-07-31

- Merged trunk on 31.7.16

2663. By Azaziah on 2016-08-09

- Merged trunk on 10/8/16.

2664. By Azaziah on 2016-08-10

- Merged trunk and resolved conflict that was created by ui-messages-part-1 branch.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openlp/core/common/settings.py'
2--- openlp/core/common/settings.py 2016-04-21 21:16:24 +0000
3+++ openlp/core/common/settings.py 2016-04-27 20:40:38 +0000
4@@ -141,6 +141,7 @@
5 'core/auto preview': False,
6 'core/audio start paused': True,
7 'core/auto unblank': False,
8+ 'core/click live slide to unblank': False,
9 'core/blank warning': False,
10 'core/ccli number': '',
11 'core/has run wizard': False,
12@@ -163,6 +164,8 @@
13 'core/display on monitor': True,
14 'core/override position': False,
15 'core/application version': '0.0',
16+ 'core/is live item edited and replaced': False,
17+ 'core/has doubleclicking preview added item to service': False,
18 'images/background color': '#000000',
19 'media/players': 'system,webkit',
20 'media/override player': QtCore.Qt.Unchecked,
21
22=== modified file 'openlp/core/lib/mediamanageritem.py'
23--- openlp/core/lib/mediamanageritem.py 2016-04-17 19:32:15 +0000
24+++ openlp/core/lib/mediamanageritem.py 2016-04-27 20:40:38 +0000
25@@ -482,6 +482,9 @@
26 'You must select one or more items to preview.'))
27 else:
28 log.debug('%s Preview requested' % self.plugin.name)
29+ # If ('advanced/double click live') is not enabled, double clicking preview adds the item to Service.
30+ # This setting prevents it from being sent to Service multiple times, in here it is reset to False.
31+ Settings().setValue('core/has doubleclicking preview added item to service', False)
32 service_item = self.build_service_item()
33 if service_item:
34 service_item.from_plugin = True
35
36=== modified file 'openlp/core/ui/generaltab.py'
37--- openlp/core/ui/generaltab.py 2016-04-17 18:57:03 +0000
38+++ openlp/core/ui/generaltab.py 2016-04-27 20:40:38 +0000
39@@ -209,6 +209,9 @@
40 self.auto_unblank_check_box = QtWidgets.QCheckBox(self.settings_group_box)
41 self.auto_unblank_check_box.setObjectName('auto_unblank_check_box')
42 self.settings_layout.addRow(self.auto_unblank_check_box)
43+ self.click_live_slide_to_unblank_check_box = QtWidgets.QCheckBox(self.settings_group_box)
44+ self.click_live_slide_to_unblank_check_box.setObjectName('click_live_slide_to_unblank')
45+ self.settings_layout.addRow(self.click_live_slide_to_unblank_check_box)
46 self.auto_preview_check_box = QtWidgets.QCheckBox(self.settings_group_box)
47 self.auto_preview_check_box.setObjectName('auto_preview_check_box')
48 self.settings_layout.addRow(self.auto_preview_check_box)
49@@ -259,6 +262,8 @@
50 self.save_check_service_check_box.setText(translate('OpenLP.GeneralTab',
51 'Prompt to save before starting a new service'))
52 self.auto_unblank_check_box.setText(translate('OpenLP.GeneralTab', 'Unblank display when adding new live item'))
53+ self.click_live_slide_to_unblank_check_box.setText(translate('OpenLP.GeneralTab',
54+ 'Unblank display when changing slide in Live'))
55 self.auto_preview_check_box.setText(translate('OpenLP.GeneralTab',
56 'Automatically preview next item in service'))
57 self.timeout_label.setText(translate('OpenLP.GeneralTab', 'Timed slide interval:'))
58@@ -292,6 +297,7 @@
59 self.password_edit.setText(settings.value('songselect password'))
60 self.save_check_service_check_box.setChecked(settings.value('save prompt'))
61 self.auto_unblank_check_box.setChecked(settings.value('auto unblank'))
62+ self.click_live_slide_to_unblank_check_box.setChecked(settings.value('click live slide to unblank'))
63 self.display_on_monitor_check.setChecked(self.screens.display)
64 self.warning_check_box.setChecked(settings.value('blank warning'))
65 self.auto_open_check_box.setChecked(settings.value('auto open'))
66@@ -336,6 +342,7 @@
67 settings.setValue('update check', self.check_for_updates_check_box.isChecked())
68 settings.setValue('save prompt', self.save_check_service_check_box.isChecked())
69 settings.setValue('auto unblank', self.auto_unblank_check_box.isChecked())
70+ settings.setValue('click live slide to unblank', self.click_live_slide_to_unblank_check_box.isChecked())
71 settings.setValue('auto preview', self.auto_preview_check_box.isChecked())
72 settings.setValue('loop delay', self.timeout_spin_box.value())
73 settings.setValue('ccli number', self.number_edit.displayText())
74
75=== modified file 'openlp/core/ui/maindisplay.py'
76--- openlp/core/ui/maindisplay.py 2016-04-01 02:32:36 +0000
77+++ openlp/core/ui/maindisplay.py 2016-04-27 20:40:38 +0000
78@@ -471,7 +471,7 @@
79 self.footer(service_item.foot_text)
80 # if was hidden keep it hidden
81 if self.hide_mode and self.is_live and not service_item.is_media():
82- if Settings().value('core/auto unblank'):
83+ if Settings().value('core/auto unblank') and not Settings().value('core/is live item edited and replaced'):
84 Registry().execute('slidecontroller_live_unblank')
85 else:
86 self.hide_display(self.hide_mode)
87
88=== modified file 'openlp/core/ui/mainwindow.py'
89--- openlp/core/ui/mainwindow.py 2016-04-23 13:46:59 +0000
90+++ openlp/core/ui/mainwindow.py 2016-04-27 20:40:38 +0000
91@@ -656,6 +656,9 @@
92 """
93 Give all the plugins a chance to perform some tasks at startup
94 """
95+ # This setting is set to false, in case def replace_service_manager_item in slidecontroller would crash the
96+ # program and make blank to modes unavailable.
97+ Settings().setValue('core/is live item edited and replaced', False)
98 self.application.process_events()
99 for plugin in self.plugin_manager.plugins:
100 if plugin.is_active():
101
102=== modified file 'openlp/core/ui/slidecontroller.py'
103--- openlp/core/ui/slidecontroller.py 2016-04-22 18:32:45 +0000
104+++ openlp/core/ui/slidecontroller.py 2016-04-27 20:40:38 +0000
105@@ -792,11 +792,17 @@
106 def replace_service_manager_item(self, item):
107 """
108 Replacement item following a remote edit
109+ This action also takes place when a song that is sent to live from Service Manager is edited.
110+ If display is blanked, it will get unblanked if automatic unblanking is enabled. We prevent this from happening
111+ by setting a hidden setting to "True" and then to "False" after the processing is done.
112+ The setting is also set to "False" on every start up, should the program ever crash during this process.
113
114 :param item: The current service item
115 """
116 if item == self.service_item:
117+ Settings().setValue('core/is live item edited and replaced', True)
118 self._process_item(item, self.preview_widget.current_slide_number())
119+ Settings().setValue('core/is live item edited and replaced', False)
120
121 def add_service_manager_item(self, item, slide_no):
122 """
123@@ -1093,6 +1099,15 @@
124 self.log_debug('Could not get lock in slide_selected after waiting %f, skip to avoid deadlock.'
125 % timeout)
126 return
127+ # If "click live slide to unblank" is enabled, unblank the display. And start = Item is sent to Live.
128+ # 'core/is live item edited and replaced' is only True when replacing Live item with the same item from Service.
129+ # Note: If this if statement is placed at the bottom of this function instead of top slide transitions are lost.
130+ if self.is_live and Settings().value('core/click live slide to unblank'):
131+ # With this display stays blanked when "auto unblank" setting is not enabled and new item is sent to Live.
132+ if not Settings().value('core/auto unblank') and start:
133+ ()
134+ if not start and not Settings().value('core/is live item edited and replaced'):
135+ Registry().execute('slidecontroller_live_unblank')
136 row = self.preview_widget.current_slide_number()
137 old_selected_row = self.selected_row
138 self.selected_row = 0
139@@ -1261,6 +1276,8 @@
140 self.play_slides_once.setText(UiStrings().PlaySlidesToEnd)
141 self.play_slides_menu.setDefaultAction(self.play_slides_loop)
142 self.play_slides_once.setChecked(False)
143+ if Settings().value('core/click live slide to unblank'):
144+ Registry().execute('slidecontroller_live_unblank')
145 else:
146 self.play_slides_loop.setIcon(build_icon(':/media/media_time.png'))
147 self.play_slides_loop.setText(UiStrings().PlaySlidesInLoop)
148@@ -1284,6 +1301,8 @@
149 self.play_slides_loop.setText(UiStrings().PlaySlidesInLoop)
150 self.play_slides_menu.setDefaultAction(self.play_slides_once)
151 self.play_slides_loop.setChecked(False)
152+ if Settings().value('core/click live slide to unblank'):
153+ Registry().execute('slidecontroller_live_unblank')
154 else:
155 self.play_slides_once.setIcon(build_icon(':/media/media_time'))
156 self.play_slides_once.setText(UiStrings().PlaySlidesToEnd)
157@@ -1338,7 +1357,7 @@
158 Triggered when a preview slide item is doubleclicked
159 """
160 if self.service_item:
161- if Settings().value('advanced/double click live'):
162+ if Settings().value('advanced/double click live') and Settings().value('core/auto unblank'):
163 # Live and Preview have issues if we have video or presentations
164 # playing in both at the same time.
165 if self.service_item.is_command():
166@@ -1346,8 +1365,13 @@
167 if self.service_item.is_media():
168 self.on_media_close()
169 self.on_go_live()
170- else:
171+ # If ('advanced/double click live') is not enabled, double clicking preview adds the item to Service.
172+ # Prevent same item in preview from being sent to Service multiple times. Changing preview slide resets
173+ # this setting. Sending to preview from Service does not reset this setting, this is a design choise.
174+ # Do note that this still allows to add item to Service multiple times if icon is clicked.
175+ elif not Settings().value('core/has doubleclicking preview added item to service'):
176 self.on_preview_add_to_service()
177+ Settings().setValue('core/has doubleclicking preview added item to service', True)
178
179 def on_go_live(self, field=None):
180 """
181
182=== modified file 'openlp/plugins/presentations/lib/messagelistener.py'
183--- openlp/plugins/presentations/lib/messagelistener.py 2016-02-05 20:11:36 +0000
184+++ openlp/plugins/presentations/lib/messagelistener.py 2016-04-27 20:40:38 +0000
185@@ -26,7 +26,7 @@
186
187 from PyQt5 import QtCore
188
189-from openlp.core.common import Registry
190+from openlp.core.common import Registry, Settings
191 from openlp.core.ui import HideMode
192 from openlp.core.lib import ServiceItemContext, ServiceItem
193 from openlp.plugins.presentations.lib.pdfcontroller import PDF_CONTROLLER_FILETYPES
194@@ -419,6 +419,8 @@
195 is_live = message[1]
196 if is_live:
197 self.live_handler.next()
198+ if Settings().value('core/click live slide to unblank'):
199+ Registry().execute('slidecontroller_live_unblank')
200 else:
201 self.preview_handler.next()
202
203@@ -431,6 +433,8 @@
204 is_live = message[1]
205 if is_live:
206 self.live_handler.previous()
207+ if Settings().value('core/click live slide to unblank'):
208+ Registry().execute('slidecontroller_live_unblank')
209 else:
210 self.preview_handler.previous()
211
212
213=== modified file 'tests/functional/openlp_core_ui/test_slidecontroller.py'
214--- tests/functional/openlp_core_ui/test_slidecontroller.py 2016-02-27 14:25:31 +0000
215+++ tests/functional/openlp_core_ui/test_slidecontroller.py 2016-04-27 20:40:38 +0000
216@@ -713,6 +713,48 @@
217 slide_controller.theme_screen, slide_controller.blank_screen
218 ])
219
220+ @patch('openlp.core.ui.slidecontroller.Settings')
221+ def on_preview_double_click_unblank_display_test(self, MockedSettings):
222+ # GIVEN: A slide controller, actions needed, settins set to True.
223+ slide_controller = SlideController(None)
224+ mocked_settings = MagicMock()
225+ mocked_settings.value.return_value = True
226+ MockedSettings.return_value = mocked_settings
227+ slide_controller.service_item = MagicMock()
228+ slide_controller.service_item.is_media = MagicMock()
229+ slide_controller.on_media_close = MagicMock()
230+ slide_controller.on_go_live = MagicMock()
231+ slide_controller.on_preview_add_to_service = MagicMock()
232+ slide_controller.media_reset = MagicMock()
233+
234+ # WHEN: on_preview_double_click is called
235+ slide_controller.on_preview_double_click()
236+
237+ # THEN: The call to addActions should be correct
238+ self.assertEqual(1, slide_controller.on_go_live.call_count, 'on_go_live should have been called once.')
239+ self.assertEqual(0, slide_controller.on_preview_add_to_service.call_count, 'Should have not been called.')
240+
241+ @patch('openlp.core.ui.slidecontroller.Settings')
242+ def on_preview_double_click_add_to_service_test(self, MockedSettings):
243+ # GIVEN: A slide controller, actions needed, settins set to False.
244+ slide_controller = SlideController(None)
245+ mocked_settings = MagicMock()
246+ mocked_settings.value.return_value = False
247+ MockedSettings.return_value = mocked_settings
248+ slide_controller.service_item = MagicMock()
249+ slide_controller.service_item.is_media = MagicMock()
250+ slide_controller.on_media_close = MagicMock()
251+ slide_controller.on_go_live = MagicMock()
252+ slide_controller.on_preview_add_to_service = MagicMock()
253+ slide_controller.media_reset = MagicMock()
254+
255+ # WHEN: on_preview_double_click is called
256+ slide_controller.on_preview_double_click()
257+
258+ # THEN: The call to addActions should be correct
259+ self.assertEqual(0, slide_controller.on_go_live.call_count, 'on_go_live Should have not been called.')
260+ self.assertEqual(1, slide_controller.on_preview_add_to_service.call_count, 'Should have been called once.')
261+
262
263 class TestInfoLabel(TestCase):
264