[PATCH] shell: fix weston_output_mode_switch() usage

[Pekka Paalanen]

On Fri, 19 Sep 2014 14:43:23 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> Calling weston_output_mode_switch() with WESTON_MODE_SWITCH_RESTORE_NATIVE
> will result in the mode being set "back" to the passed in mode - so the
> passed mode should be the native mode.

That's kind of silly, don't you think? :-)
If it's RESTORE_NATIVE, I'd probably expect the passed in mode to be
ignored, or even better, required to be NULL. And as the scale argument
is useless too, I'd probably want a whole another function just
weston_output_restore_mode(output).

And that leaves weston_output_mode_switch() with essentially a boolean
"operation" argument, so might as well refactor and make it two
separate functions. All three could share some static helpers.

> Additionally, weston_output_mode_switch() should be called when
> output->original_mode is non-NULL (which indicates we had a temporary
> mode set).  The comparison to current_mode results in a lot of
> log chatter.

Yes, that is true. I just wonder if there was a better way to fix this.

Like, what is weston_output_mode_switch() actually supposed to do, and
why is it "bad" (that it is logged) to restore when you're already ok?
And what is original_mode?

The basic idea as I recall it was to have a permanent video mode, and
optionally a temporary video mode. When the permanent (native?) video
mode changes, all clients get notified and the desktop is re-laid-out.
Setting a temporary video mode does none of that, but is only used to
accommodating fullscreen apps sometimes. Switches to and from a
temporary mode are completely hidden from clients.

As a detail, if the permanent mode is changed while a temporary mode is
in effect, the notifications should be sent only when coming out of
temporary mode, i.e. restoring. (Does that make sense?)

Hmm, yes, so the change below does make sense. Ok, that's what the
original_mode is: the mode that is current in clients' point of view.

Therefore: pushed. Feel free to refactor the code in a follow-up,
IMHO. ;-)


Thanks,
pq


> ---
>  desktop-shell/shell.c               | 6 +++---
>  fullscreen-shell/fullscreen-shell.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 3a5a702..6ef36b5 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -2247,11 +2247,11 @@ shell_surface_set_class(struct wl_client *client,
>  static void
>  restore_output_mode(struct weston_output *output)
>  {
> -	if (output->current_mode != output->original_mode ||
> +	if (output->original_mode ||
>  	    (int32_t)output->current_scale != output->original_scale)
>  		weston_output_switch_mode(output,
> -					  output->original_mode,
> -					  output->original_scale,
> +					  output->native_mode,
> +					  output->native_scale,
>  					  WESTON_MODE_SWITCH_RESTORE_NATIVE);
>  }
>  
> diff --git a/fullscreen-shell/fullscreen-shell.c b/fullscreen-shell/fullscreen-shell.c
> index 25932d4..c7950df 100644
> --- a/fullscreen-shell/fullscreen-shell.c
> +++ b/fullscreen-shell/fullscreen-shell.c
> @@ -284,11 +284,11 @@ fs_output_for_output(struct weston_output *output)
>  static void
>  restore_output_mode(struct weston_output *output)
>  {
> -	if (output->current_mode != output->original_mode ||
> +	if (output->original_mode ||
>  	    (int32_t)output->current_scale != output->original_scale)
>  		weston_output_switch_mode(output,
> -					  output->original_mode,
> -					  output->original_scale,
> +					  output->native_mode,
> +					  output->native_scale,
>  					  WESTON_MODE_SWITCH_RESTORE_NATIVE);
>  }
>