Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python based screensavers #1072

Closed
wants to merge 1 commit into from
Closed

Conversation

dersphere
Copy link
Contributor

Please don't see this as complete pull request - at the moment it is more a proof-of-concept.

These changes should do the following:

  • add "xbmc.python.screensaver" extension point for addons
  • show the new kind of screensaver addon in the screensaver select dialog in settings (in addition to the binary ones)
  • run the addon on CApplication::ActivateScreenSaver()
  • pass on CApplication::WakeUpScreenSaver()

the screensaver addon should exit itself via waiting for the onScreensaverDeactivated-announcement

I created a simple python addon for demonstrating, you can get it from here:
https://github.com/downloads/dersphere/script.screensaver.test/script.screensaver.test-0.0.1.zip
source is here: https://github.com/dersphere/script.screensaver.test

It works already, means, you can add this addon, set it as screensaver and it gets started and exits like a normal screensaver.

Of course there would still some things to do:
ex. combine both types of screensaver-addons in AddonsDirectory

But I'm more interested in your opinions about the general idea of having python based screensavers and if its worth more work.

regards,
sphere

@ghost
Copy link

ghost commented Jun 15, 2012

yes please. i actually started on something similar. but please, don't add another extension point. just have CScreenSaver behave differently based on the library type (py vs xbs or whatever it is these days)

@garbear
Copy link
Member

garbear commented Jun 16, 2012

+1. Off topic: started this also, ran into a problem: my python screen saver couldn't use includes.xml or Fonts.xml (even tho xbmc complains about not having one). When this gets in I'll dust off my old branch and see what I can do.

@dersphere
Copy link
Contributor Author

Thanks for your comments - I will do the changes spiff requested.

Off Topic: Are the HAS_PYTHON-ifdef's still needed?

@ghost
Copy link

ghost commented Jun 16, 2012

While not really relevant on full fledged port platforms these defines are really handy when porting to new platforms so please respect.

@garbear
Copy link
Member

garbear commented Jun 19, 2012

I was able to do python screensavers without adding another extension point. It reuses the existing one, so a python file with <extension point="xbmc.ui.screensaver" library="default.py"/> will show up to the system as a screensaver.

Two issues with my solution: addons can't be a script AND a screensaver (limitation with xbmc: see this fixme), and the few existing scripts I've tried don't die immediately when a key is pressed.

Here's the patch. a888d2bf6

This is a combination of @garbear's and my implemention:
- Extension point needs to be "xbmc.ui.screensaver"
- Python script needs to exit itself after receiving an onScreensaverDeactivated-announcement
@dersphere
Copy link
Contributor Author

@cptspiff Thanks, just wanted to know. Will respect them.

@garbear I would not recommend calling g_pythonParser.StopScript() (at least not so early): It would take several seconds to get them killed. Even my demo screensaver which is able to exit itself after receiving the announcement will hang for a few seconds after StopScript() is called.

I'm also wondering about your other comment about the single extension point limitation: Addons can have multiple extension point and it also works in that case here (For a demonstration see https://github.com/dersphere/script.screensaver.test branch:multiple_extension_points).

I updated this PR and my demo screensaver to reuse the xbmc.ui.screensaver extension point.

@garbear
Copy link
Member

garbear commented Jun 19, 2012

I guess multiple extension points do work, but I'm clueless where this is in the code.

The test screensaver has problems running on top of other addons. I see a CAnnouncementManager - Announcement: OnScreensaverActivated from xbmc event fire, but the screen only dims - no SS.

@ghost
Copy link

ghost commented Jun 19, 2012

why can't you reuse ::Create and ::Destroy ?

@dersphere
Copy link
Contributor Author

@garbear

The test screensaver has problems running on top of other addons.

This affects all screensavers - if any modal window is open (or a script-addon which uses windowxml.doModal() is running), m_screensaver is changed to "screensaver.xbmc.builtin.dim" - independently what screensaver was set in the settings, see:
https://github.com/dersphere/xbmc/blob/1bf884017651df89f8a5608924f3e63c574d3a3c/xbmc/Application.cpp#L4597

May I change that behavior?

@cptspiff Aye sir - I will (try to) move it :)

@jmarshallnz
Copy link
Contributor

There's a reason for the dim on modal. The problem is several fold:

  1. We allow a vis as a screensaver.
  2. If a modal dialog is on screen, then we must eliminate the dialog in order to display the screensaver, unless the screensaver is also a modal dialog that is topmost. Clearly this is the best thing to do, but there's a problem with this if 1 holds.
  3. We can't just eliminate dialogs willy-nilly as the actions that occur due to cancelling a dialog might not be in the best interests of the user.

If we can eliminate the vis stuff and move all screensavers to actual dialogs (dim/black are already - the fullscreen builtin ones probably aren't) then we can remove the dim stuff.

@dersphere
Copy link
Contributor Author

@jmarshallnz thanks for your comment (btw. willy-nilly is actually in my dictionary :D).

Regarding the modal-problem:
Changing so much logic/structure in other code parts clearly exceeds my c++ skills - I don't care about commit authorship, so if anyone is willing to do the needed changes: You're welcome :)

@ghost ghost assigned garbear Jun 30, 2012
@garbear
Copy link
Member

garbear commented Jun 30, 2012

Added to the July merge window. Screen savers during modals is not included in this PR

@jmarshallnz
Copy link
Contributor

Are we planning on acting on cptspiff's suggestion (using ::Create and ::Destroy)

@garbear
Copy link
Member

garbear commented Jun 30, 2012

To run the script? I'm afraid I'm confused

@dersphere
Copy link
Contributor Author

I planned on doing the changes requested by @cptspiff but got out of time. I won't have time for the next weeks.

So if you want to wait, if anyone wants to open another pull request and close this one or if you want to merge the actual version - feel free.

@garbear
Copy link
Member

garbear commented Jul 3, 2012

I can carry the torch. Can you clarify sphere, what is meant by using ::Create and ::Destroy?

@ghost
Copy link

ghost commented Jul 3, 2012

::Create and ::Destroy are the CScreenSaver start/stop methods. it should be abstracted away from the application.

@garbear garbear mentioned this pull request Jul 7, 2012
@dersphere
Copy link
Contributor Author

closed in favor of #1127 (...carry the torch... ;-))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants