Merge lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank into lp:openlp
- click-slide-to-go-live-from-blank
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 2689 |
Proposed branch: | lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank |
Merge into: | lp:openlp |
Diff against target: |
247 lines (+85/-6) 6 files modified
openlp/core/common/settings.py (+1/-0) openlp/core/lib/mediamanageritem.py (+1/-0) openlp/core/ui/generaltab.py (+7/-0) openlp/core/ui/slidecontroller.py (+25/-5) openlp/plugins/presentations/lib/messagelistener.py (+5/-1) tests/functional/openlp_core_ui/test_slidecontroller.py (+46/-0) |
To merge this branch: | bzr merge lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Raoul Snyman | Approve | ||
Tomas Groth | Pending | ||
Review via email: mp+302587@code.launchpad.net |
This proposal supersedes a proposal from 2016-07-17.
Commit message
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 general
options tab for disabling/enabling this behavior for a-c.
Additionally this branch also includes fix for bug
https:/
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.
-------
lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank (revision 2664)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal | # |
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 :)
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.
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal | # |
Couple of tests and some tests are needed.
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 : Posted in a previous version of this proposal | # |
Check my comment. I'll try to look at this more in-depth over the weekend and give you more feedback.
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
> 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 : Posted in a previous version of this proposal | # |
I noticed the comment was not saved, so here it is. ^^ Sorry about that :/
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
I commented on replace live item function and resetting the setting.
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal | # |
I still do not agree with using settings for temporary flags. Find another way.
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
"I still do not agree with using settings for temporary flags. Find another way."
Well I can't think of an other way.
I tried to fix: https:/
but it didn't work properly and was way more complex.
Since we need to trigger actions in many files/functions based on temporary flags,
what would be a proper solution without the use of settings?
Also replied to diff comments.
Thank you for your review.
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal | # |
I've just merged the flags section of the Registry object.
Having said that, and looking at your branch, I'm not entirely convinced this is the way to go about this. While you can explain to me what the difference is between this and the other auto-unblank setting, you can't explain it to our users, and I refuse to be lumped with the problem (I'm about the only person who actually actively deals with our users).
I recommend rather merging this with the current "auto-unblank" setting and make a single setting with a set of modes instead.
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
> I've just merged the flags section of the Registry object.
>
> Having said that, and looking at your branch, I'm not entirely convinced this
> is the way to go about this. While you can explain to me what the difference
> is between this and the other auto-unblank setting, you can't explain it to
> our users, and I refuse to be lumped with the problem (I'm about the only
> person who actually actively deals with our users).
>
> I recommend rather merging this with the current "auto-unblank" setting and
> make a single setting with a set of modes instead.
I'll be looking forward to moving the flags to registry.
This branch adds the unblanking of display by clicking slides in Live.
Currently this is possible when using Escape item, but not while using the real blank to modes.
The current behavior is highly confusing.
This setting will make things more understandable and easier to manage.
So ultimately this just makes it possible for users to easily resume Live presentation by clicking a slide they wish to show.
It's not related to sending items to Live, It's more about unblanking display and resuming the projection of the current Live item. Thus they are a very different thing.
After this is merged, I think we should begin working on removing the Escape item,
It's very much a broken and confusing format.
I am sorry about the current user support situation, hope things will be better someday.
Ultimately it would be great if the customer support would be something you wouldn't have to do.
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal | # |
Traceback (most recent call last):
File "/home/
elif not Registry(
File "/home/
raise KeyError('Working Flag {key} not found in list'.format(
KeyError: 'Working Flag has doubleclick added item to service not found in list'
Blank live and double click on a preview slide.
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
I am unable to reproduce this traceback,
need to investigate further.
Fixed the commented stuff.
Thank you for the review.
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal | # |
> Traceback (most recent call last):
> File "/home/
> blank/openlp/
> on_preview_
> elif not Registry(
> True:
> File "/home/
> blank/openlp/
> raise KeyError('Working Flag {key} not found in list'.format(
> KeyError: 'Working Flag has doubleclick added item to service not found in
> list'
>
> Blank live and double click on a preview slide.
Fixed this,
also removed the ??? if statement.
Raoul Snyman (raoul-snyman) : Posted in a previous version of this proposal | # |
Tim Bentley (trb143) : Posted in a previous version of this proposal | # |
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal | # |
Sorry but needs to respin and other merges have broken this
Raoul Snyman (raoul-snyman) : | # |
Tim Bentley (trb143) : | # |
Preview Diff
1 | === modified file 'openlp/core/common/settings.py' |
2 | --- openlp/core/common/settings.py 2016-07-01 21:17:20 +0000 |
3 | +++ openlp/core/common/settings.py 2016-08-10 18:57:47 +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 | |
13 | === modified file 'openlp/core/lib/mediamanageritem.py' |
14 | --- openlp/core/lib/mediamanageritem.py 2016-06-12 22:02:24 +0000 |
15 | +++ openlp/core/lib/mediamanageritem.py 2016-08-10 18:57:47 +0000 |
16 | @@ -488,6 +488,7 @@ |
17 | 'You must select one or more items to preview.')) |
18 | else: |
19 | log.debug('%s Preview requested' % self.plugin.name) |
20 | + Registry().set_flag('has doubleclick added item to service', False) |
21 | service_item = self.build_service_item() |
22 | if service_item: |
23 | service_item.from_plugin = True |
24 | |
25 | === modified file 'openlp/core/ui/generaltab.py' |
26 | --- openlp/core/ui/generaltab.py 2016-07-24 20:20:25 +0000 |
27 | +++ openlp/core/ui/generaltab.py 2016-08-10 18:57:47 +0000 |
28 | @@ -209,6 +209,9 @@ |
29 | self.auto_unblank_check_box = QtWidgets.QCheckBox(self.settings_group_box) |
30 | self.auto_unblank_check_box.setObjectName('auto_unblank_check_box') |
31 | self.settings_layout.addRow(self.auto_unblank_check_box) |
32 | + self.click_live_slide_to_unblank_check_box = QtWidgets.QCheckBox(self.settings_group_box) |
33 | + self.click_live_slide_to_unblank_check_box.setObjectName('click_live_slide_to_unblank') |
34 | + self.settings_layout.addRow(self.click_live_slide_to_unblank_check_box) |
35 | self.auto_preview_check_box = QtWidgets.QCheckBox(self.settings_group_box) |
36 | self.auto_preview_check_box.setObjectName('auto_preview_check_box') |
37 | self.settings_layout.addRow(self.auto_preview_check_box) |
38 | @@ -258,6 +261,8 @@ |
39 | self.settings_group_box.setTitle(translate('OpenLP.GeneralTab', 'Application Settings')) |
40 | self.save_check_service_check_box.setText(translate('OpenLP.GeneralTab', |
41 | 'Prompt to save before starting a new service')) |
42 | + self.click_live_slide_to_unblank_check_box.setText(translate('OpenLP.GeneralTab', |
43 | + 'Unblank display when changing slide in Live')) |
44 | self.auto_unblank_check_box.setText(translate('OpenLP.GeneralTab', 'Unblank display when sending ' |
45 | 'items to Live')) |
46 | self.auto_preview_check_box.setText(translate('OpenLP.GeneralTab', |
47 | @@ -293,6 +298,7 @@ |
48 | self.password_edit.setText(settings.value('songselect password')) |
49 | self.save_check_service_check_box.setChecked(settings.value('save prompt')) |
50 | self.auto_unblank_check_box.setChecked(settings.value('auto unblank')) |
51 | + self.click_live_slide_to_unblank_check_box.setChecked(settings.value('click live slide to unblank')) |
52 | self.display_on_monitor_check.setChecked(self.screens.display) |
53 | self.warning_check_box.setChecked(settings.value('blank warning')) |
54 | self.auto_open_check_box.setChecked(settings.value('auto open')) |
55 | @@ -337,6 +343,7 @@ |
56 | settings.setValue('update check', self.check_for_updates_check_box.isChecked()) |
57 | settings.setValue('save prompt', self.save_check_service_check_box.isChecked()) |
58 | settings.setValue('auto unblank', self.auto_unblank_check_box.isChecked()) |
59 | + settings.setValue('click live slide to unblank', self.click_live_slide_to_unblank_check_box.isChecked()) |
60 | settings.setValue('auto preview', self.auto_preview_check_box.isChecked()) |
61 | settings.setValue('loop delay', self.timeout_spin_box.value()) |
62 | settings.setValue('ccli number', self.number_edit.displayText()) |
63 | |
64 | === modified file 'openlp/core/ui/slidecontroller.py' |
65 | --- openlp/core/ui/slidecontroller.py 2016-07-24 20:20:25 +0000 |
66 | +++ openlp/core/ui/slidecontroller.py 2016-08-10 18:57:47 +0000 |
67 | @@ -87,6 +87,8 @@ |
68 | self.is_live = False |
69 | self.display = None |
70 | self.controller_type = None |
71 | + Registry().set_flag('has doubleclick added item to service', True) |
72 | + Registry().set_flag('replace service manager item', False) |
73 | |
74 | def send_to_plugins(self, *args): |
75 | """ |
76 | @@ -796,12 +798,15 @@ |
77 | |
78 | def replace_service_manager_item(self, item): |
79 | """ |
80 | - Replacement item following a remote edit |
81 | + Replacement item following a remote edit. |
82 | + This action also takes place when a song that is sent to live from Service Manager is edited. |
83 | |
84 | :param item: The current service item |
85 | """ |
86 | if item == self.service_item: |
87 | + Registry().set_flag('replace service manager item', True) |
88 | self._process_item(item, self.preview_widget.current_slide_number()) |
89 | + Registry().set_flag('replace service manager item', False) |
90 | |
91 | def add_service_manager_item(self, item, slide_no): |
92 | """ |
93 | @@ -970,9 +975,10 @@ |
94 | |
95 | def on_slide_unblank(self): |
96 | """ |
97 | - Handle the slidecontroller unblank event |
98 | + Handle the slidecontroller unblank event. |
99 | """ |
100 | - self.on_blank_display(False) |
101 | + if not Registry().get_flag('replace service manager item') is True: |
102 | + self.on_blank_display(False) |
103 | |
104 | def on_blank_display(self, checked=None): |
105 | """ |
106 | @@ -1103,6 +1109,11 @@ |
107 | self.log_debug('Could not get lock in slide_selected after waiting %f, skip to avoid deadlock.' |
108 | % timeout) |
109 | return |
110 | + # If "click live slide to unblank" is enabled, unblank the display. And start = Item is sent to Live. |
111 | + # Note: If this if statement is placed at the bottom of this function instead of top slide transitions are lost. |
112 | + if self.is_live and Settings().value('core/click live slide to unblank'): |
113 | + if not start: |
114 | + Registry().execute('slidecontroller_live_unblank') |
115 | row = self.preview_widget.current_slide_number() |
116 | old_selected_row = self.selected_row |
117 | self.selected_row = 0 |
118 | @@ -1285,6 +1296,8 @@ |
119 | self.play_slides_once.setText(UiStrings().PlaySlidesToEnd) |
120 | self.play_slides_menu.setDefaultAction(self.play_slides_loop) |
121 | self.play_slides_once.setChecked(False) |
122 | + if Settings().value('core/click live slide to unblank'): |
123 | + Registry().execute('slidecontroller_live_unblank') |
124 | else: |
125 | self.play_slides_loop.setIcon(build_icon(':/media/media_time.png')) |
126 | self.play_slides_loop.setText(UiStrings().PlaySlidesInLoop) |
127 | @@ -1308,6 +1321,8 @@ |
128 | self.play_slides_loop.setText(UiStrings().PlaySlidesInLoop) |
129 | self.play_slides_menu.setDefaultAction(self.play_slides_once) |
130 | self.play_slides_loop.setChecked(False) |
131 | + if Settings().value('core/click live slide to unblank'): |
132 | + Registry().execute('slidecontroller_live_unblank') |
133 | else: |
134 | self.play_slides_once.setIcon(build_icon(':/media/media_time')) |
135 | self.play_slides_once.setText(UiStrings().PlaySlidesToEnd) |
136 | @@ -1362,7 +1377,7 @@ |
137 | Triggered when a preview slide item is doubleclicked |
138 | """ |
139 | if self.service_item: |
140 | - if Settings().value('advanced/double click live'): |
141 | + if Settings().value('advanced/double click live') and Settings().value('core/auto unblank'): |
142 | # Live and Preview have issues if we have video or presentations |
143 | # playing in both at the same time. |
144 | if self.service_item.is_command(): |
145 | @@ -1371,8 +1386,13 @@ |
146 | if self.service_item.is_media(): |
147 | self.on_media_close() |
148 | self.on_go_live() |
149 | - else: |
150 | + # If ('advanced/double click live') is not enabled, double clicking preview adds the item to Service. |
151 | + # Prevent same item in preview from being sent to Service multiple times. |
152 | + # Changing the preview slide resets this flag to False. |
153 | + # Do note that this still allows to add item to Service multiple times if icon is clicked. |
154 | + elif not Registry().get_flag('has doubleclick added item to service') is True: |
155 | self.on_preview_add_to_service() |
156 | + Registry().set_flag('has doubleclick added item to service', True) |
157 | |
158 | def on_go_live(self, field=None): |
159 | """ |
160 | |
161 | === modified file 'openlp/plugins/presentations/lib/messagelistener.py' |
162 | --- openlp/plugins/presentations/lib/messagelistener.py 2016-07-01 21:17:20 +0000 |
163 | +++ openlp/plugins/presentations/lib/messagelistener.py 2016-08-10 18:57:47 +0000 |
164 | @@ -26,7 +26,7 @@ |
165 | |
166 | from PyQt5 import QtCore |
167 | |
168 | -from openlp.core.common import Registry |
169 | +from openlp.core.common import Registry, Settings |
170 | from openlp.core.ui import HideMode |
171 | from openlp.core.lib import ServiceItemContext |
172 | from openlp.plugins.presentations.lib.pdfcontroller import PDF_CONTROLLER_FILETYPES |
173 | @@ -419,6 +419,8 @@ |
174 | is_live = message[1] |
175 | if is_live: |
176 | self.live_handler.next() |
177 | + if Settings().value('core/click live slide to unblank'): |
178 | + Registry().execute('slidecontroller_live_unblank') |
179 | else: |
180 | self.preview_handler.next() |
181 | |
182 | @@ -431,6 +433,8 @@ |
183 | is_live = message[1] |
184 | if is_live: |
185 | self.live_handler.previous() |
186 | + if Settings().value('core/click live slide to unblank'): |
187 | + Registry().execute('slidecontroller_live_unblank') |
188 | else: |
189 | self.preview_handler.previous() |
190 | |
191 | |
192 | === modified file 'tests/functional/openlp_core_ui/test_slidecontroller.py' |
193 | --- tests/functional/openlp_core_ui/test_slidecontroller.py 2016-06-01 21:42:54 +0000 |
194 | +++ tests/functional/openlp_core_ui/test_slidecontroller.py 2016-08-10 18:57:47 +0000 |
195 | @@ -713,6 +713,52 @@ |
196 | slide_controller.theme_screen, slide_controller.blank_screen |
197 | ]) |
198 | |
199 | + @patch('openlp.core.ui.slidecontroller.Settings') |
200 | + def on_preview_double_click_unblank_display_test(self, MockedSettings): |
201 | + # GIVEN: A slide controller, actions needed, settins set to True. |
202 | + slide_controller = SlideController(None) |
203 | + mocked_settings = MagicMock() |
204 | + mocked_settings.return_value = True |
205 | + MockedSettings.return_value = mocked_settings |
206 | + slide_controller.service_item = MagicMock() |
207 | + slide_controller.service_item.is_media = MagicMock() |
208 | + slide_controller.on_media_close = MagicMock() |
209 | + slide_controller.on_go_live = MagicMock() |
210 | + slide_controller.on_preview_add_to_service = MagicMock() |
211 | + slide_controller.media_reset = MagicMock() |
212 | + Registry.create() |
213 | + Registry().set_flag('has doubleclick added item to service', True) |
214 | + |
215 | + # WHEN: on_preview_double_click is called |
216 | + slide_controller.on_preview_double_click() |
217 | + |
218 | + # THEN: The call to addActions should be correct |
219 | + self.assertEqual(1, slide_controller.on_go_live.call_count, 'on_go_live should have been called once.') |
220 | + self.assertEqual(0, slide_controller.on_preview_add_to_service.call_count, 'Should have not been called.') |
221 | + |
222 | + @patch('openlp.core.ui.slidecontroller.Settings') |
223 | + def on_preview_double_click_add_to_service_test(self, MockedSettings): |
224 | + # GIVEN: A slide controller, actions needed, settins set to False. |
225 | + slide_controller = SlideController(None) |
226 | + mocked_settings = MagicMock() |
227 | + mocked_settings.value.return_value = False |
228 | + MockedSettings.return_value = mocked_settings |
229 | + slide_controller.service_item = MagicMock() |
230 | + slide_controller.service_item.is_media = MagicMock() |
231 | + slide_controller.on_media_close = MagicMock() |
232 | + slide_controller.on_go_live = MagicMock() |
233 | + slide_controller.on_preview_add_to_service = MagicMock() |
234 | + slide_controller.media_reset = MagicMock() |
235 | + Registry.create() |
236 | + Registry().set_flag('has doubleclick added item to service', False) |
237 | + |
238 | + # WHEN: on_preview_double_click is called |
239 | + slide_controller.on_preview_double_click() |
240 | + |
241 | + # THEN: The call to addActions should be correct |
242 | + self.assertEqual(0, slide_controller.on_go_live.call_count, 'on_go_live Should have not been called.') |
243 | + self.assertEqual(1, slide_controller.on_preview_add_to_service.call_count, 'Should have been called once.') |
244 | + |
245 | @patch(u'openlp.core.ui.slidecontroller.SlideController.image_manager') |
246 | @patch(u'PyQt5.QtCore.QTimer.singleShot') |
247 | def test_update_preview_live(self, mocked_singleShot, mocked_image_manager): |
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.