OpenLP

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

  1. click-slide-to-go-live-from-blank
  2. Merge into trunk
Proposed by Azaziah
Status: Superseded
Proposed branch: lp://qastaging/~suutari-olli/openlp/click-slide-to-go-live-from-blank
Merge into: lp://qastaging/openlp
Diff against target: 248 lines (+86/-4)
7 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/slidecontroller.py (+25/-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://qastaging/~suutari-olli/openlp/click-slide-to-go-live-from-blank
Reviewer Review Type Date Requested Status
Tim Bentley Needs Fixing
Tomas Groth Pending
Raoul Snyman Pending
Review via email: mp+292077@code.qastaging.launchpad.net

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

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

Description of the change

In this re-proposal:

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)"

lp:~suutari-olli/openlp/click-slide-to-go-live-from-blank (revision 2644)
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/1445/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-02-Functional-Tests/1362/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-03-Interface-Tests/1300/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1107/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/698/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05a-Code_Analysis/765/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05b-Test_Coverage/633/

------------------------------------------------------------------------
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.
-------------------------------------------------------------------------
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.

To post a comment you must log in.
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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.

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

Looks good to me. Nice tests.

review: Approve
Revision history for this message
Tim Bentley (trb143) wrote :

Couple of tests and some tests are needed.

review: Needs Fixing
lp://qastaging/~suutari-olli/openlp/click-slide-to-go-live-from-blank updated
2645. By Azaziah

- Removed _ from end of one setting name.
- Fixed ident on replace_service_manager

Revision history for this message
Azaziah (suutari-olli) wrote :

I need to start working on tests...

Fixed the diff comments.

lp://qastaging/~suutari-olli/openlp/click-slide-to-go-live-from-blank updated
2646. By Azaziah

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

2647. By Azaziah

Noticed I had removed one test from end of the file, added it back.

2648. By Azaziah

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

merged trunk on 14.6.16

2650. By Azaziah

- Turned the two new hidden settings into registry flags

2651. By Azaziah

Comment cleanup / Improvements

2652. By Azaziah

- Merged trunk on 27.6.16

2653. By Azaziah

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

2654. By Azaziah

Merged trunk on 14.7.16

2655. By Azaziah

pep8 fixes, test still broken.

2656. By Azaziah

Fixed the tests.

2657. By Azaziah

- Reduced comments
- Removed unrequired reg_value from test.

2658. By Azaziah

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

2659. By Azaziah

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

2660. By Azaziah

- Moved them to another init since something broke

2661. By Azaziah

- removed one unrequired if statement.

2662. By Azaziah

- Merged trunk on 31.7.16

2663. By Azaziah

- Merged trunk on 10/8/16.

2664. By Azaziah

- 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
The diff is not available at this time. You can reload the page or download it.