diff mbox

sdl: Do not disable screensaver by default

Message ID 4BF8E76E.7040601@web.de
State New
Headers show

Commit Message

Jan Kiszka May 23, 2010, 8:29 a.m. UTC 
From: Jan Kiszka <jan.kiszka@siemens.com>

Unless we are running in full-screen mode, QEMU's SDL window should not
disable the host's screensaver. The user can still change this behaviour
by setting the environment variable SDL_VIDEO_ALLOW_SCREENSAVER as
desired.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Cool, thanks for digging out SDL_VIDEO_ALLOW_SCREENSAVER. I came across
by this issue as well but I was too lazy to analyze to reason. This
patch solves it for me.

 sdl.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Comments

Anthony Liguori May 24, 2010, 8:28 p.m. UTC | #1 
On 05/23/2010 03:29 AM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> Unless we are running in full-screen mode, QEMU's SDL window should not
> disable the host's screensaver. The user can still change this behaviour
> by setting the environment variable SDL_VIDEO_ALLOW_SCREENSAVER as
> desired.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>    

Applied.  Thanks.

This also fixes https://bugs.launchpad.net/qemu/+bug/583462.

Regards,

Anthony Liguori

> ---
>
> Cool, thanks for digging out SDL_VIDEO_ALLOW_SCREENSAVER. I came across
> by this issue as well but I was too lazy to analyze to reason. This
> patch solves it for me.
>
>   sdl.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/sdl.c b/sdl.c
> index 16a48e9..3bdd518 100644
> --- a/sdl.c
> +++ b/sdl.c
> @@ -855,6 +855,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
>       if (no_frame)
>           gui_noframe = 1;
>
> +    if (!full_screen) {
> +        setenv("SDL_VIDEO_ALLOW_SCREENSAVER", "1", 0);
> +    }
> +
>       flags = SDL_INIT_VIDEO | SDL_INIT_NOPARACHUTE;
>       if (SDL_Init (flags)) {
>           fprintf(stderr, "Could not initialize SDL(%s) - exiting\n",
>
Michael Tokarev May 24, 2010, 9:40 p.m. UTC | #2 
23.05.2010 12:29, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> Unless we are running in full-screen mode, QEMU's SDL window should not
> disable the host's screensaver. The user can still change this behaviour
> by setting the environment variable SDL_VIDEO_ALLOW_SCREENSAVER as
> desired.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
> Cool, thanks for digging out SDL_VIDEO_ALLOW_SCREENSAVER. I came across
> by this issue as well but I was too lazy to analyze to reason. This
> patch solves it for me.
>
>   sdl.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/sdl.c b/sdl.c
> index 16a48e9..3bdd518 100644
> --- a/sdl.c
> +++ b/sdl.c
> @@ -855,6 +855,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
>       if (no_frame)
>           gui_noframe = 1;
>
> +    if (!full_screen) {
> +        setenv("SDL_VIDEO_ALLOW_SCREENSAVER", "1", 0);
> +    }
> +

I think it's conceptually wrong.

It's trivial to toggle full-screen mode by hitting Ctrl+Alt+F.
Following this logic, on each toggle we should toggle the
environment variable (which wont work).  Or else the next bug
report to be filed is that screen saver isn't re-enabled when
switching from fullscreen to window and it isn't re-disabled
when switching to fullscreen.

How about not poking at screensaver by default unconditionally?
To me it sound more correct.  I.e., the above setenv(), but
without the if part...

Thanks!

/mjt
Michael Tokarev May 24, 2010, 9:42 p.m. UTC | #3 
25.05.2010 01:40, Michael Tokarev wrote:
> 23.05.2010 12:29, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> Unless we are running in full-screen mode, QEMU's SDL window should not
>> disable the host's screensaver. The user can still change this behaviour
>> by setting the environment variable SDL_VIDEO_ALLOW_SCREENSAVER as
>> desired.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>> Cool, thanks for digging out SDL_VIDEO_ALLOW_SCREENSAVER. I came across
>> by this issue as well but I was too lazy to analyze to reason. This
>> patch solves it for me.
>>
>> sdl.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/sdl.c b/sdl.c
>> index 16a48e9..3bdd518 100644
>> --- a/sdl.c
>> +++ b/sdl.c
>> @@ -855,6 +855,10 @@ void sdl_display_init(DisplayState *ds, int
>> full_screen, int no_frame)
>> if (no_frame)
>> gui_noframe = 1;
>>
>> + if (!full_screen) {
>> + setenv("SDL_VIDEO_ALLOW_SCREENSAVER", "1", 0);
>> + }
>> +
>
> I think it's conceptually wrong.
>
> It's trivial to toggle full-screen mode by hitting Ctrl+Alt+F.
> Following this logic, on each toggle we should toggle the
> environment variable (which wont work). Or else the next bug
> report to be filed is that screen saver isn't re-enabled when
> switching from fullscreen to window and it isn't re-disabled
> when switching to fullscreen.

One more data point: guest can not turn off our monitor anyway,
even in full-screen mode.  So I think that by default our local
screen saver should continue working regardless.

> How about not poking at screensaver by default unconditionally?
> To me it sound more correct. I.e., the above setenv(), but
> without the if part...
>
> Thanks!
>
> /mjt
Anthony Liguori May 24, 2010, 9:52 p.m. UTC | #4 
On 05/24/2010 04:40 PM, Michael Tokarev wrote:
> 23.05.2010 12:29, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> Unless we are running in full-screen mode, QEMU's SDL window should not
>> disable the host's screensaver. The user can still change this behaviour
>> by setting the environment variable SDL_VIDEO_ALLOW_SCREENSAVER as
>> desired.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>> Cool, thanks for digging out SDL_VIDEO_ALLOW_SCREENSAVER. I came across
>> by this issue as well but I was too lazy to analyze to reason. This
>> patch solves it for me.
>>
>>   sdl.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/sdl.c b/sdl.c
>> index 16a48e9..3bdd518 100644
>> --- a/sdl.c
>> +++ b/sdl.c
>> @@ -855,6 +855,10 @@ void sdl_display_init(DisplayState *ds, int 
>> full_screen, int no_frame)
>>       if (no_frame)
>>           gui_noframe = 1;
>>
>> +    if (!full_screen) {
>> +        setenv("SDL_VIDEO_ALLOW_SCREENSAVER", "1", 0);
>> +    }
>> +
>
> I think it's conceptually wrong.
>
> It's trivial to toggle full-screen mode by hitting Ctrl+Alt+F.

I don't think it's worth jumping through hoops to conditionally 
disable/enable screensaver.

More sophisticated screen saver behavior should be implemented in 
upstream SDL.

Regards,

Anthony Liguori

> Following this logic, on each toggle we should toggle the
> environment variable (which wont work).  Or else the next bug
> report to be filed is that screen saver isn't re-enabled when
> switching from fullscreen to window and it isn't re-disabled
> when switching to fullscreen.
>
> How about not poking at screensaver by default unconditionally?
> To me it sound more correct.  I.e., the above setenv(), but
> without the if part...
>
> Thanks!
>
> /mjt
diff mbox

Patch

diff --git a/sdl.c b/sdl.c
index 16a48e9..3bdd518 100644
--- a/sdl.c
+++ b/sdl.c
@@ -855,6 +855,10 @@  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
     if (no_frame)
         gui_noframe = 1;
 
+    if (!full_screen) {
+        setenv("SDL_VIDEO_ALLOW_SCREENSAVER", "1", 0);
+    }
+
     flags = SDL_INIT_VIDEO | SDL_INIT_NOPARACHUTE;
     if (SDL_Init (flags)) {
         fprintf(stderr, "Could not initialize SDL(%s) - exiting\n",