[PATCH] shell: fix weston_output_mode_switch() usage

[Derek Foreman]

On 22/09/14 06:41 AM, Pekka Paalanen wrote:
> 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? :-)

Yeah, I quite dislike it.  I was thinking about splitting it into 3
functions that only take appropriate parameters...

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

Don't forget that if the mode is SET_NATIVE the mode pointer must be one
of the modes known to the backend, but if it's SET_TEMPORARY you can
pass in a pointer to your own struct...  This function's full of bees.

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

Nod.

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

Yeah, right now that warning just seems to mean "the mouse button has
been clicked".

I do think there's merit to the warning - trying to restore non-temp
mode when you're not in a temp mode seems to indicate something
questionable is afoot.

original_mode appears to be "the mode we were in at the time we switched
into temporary mode" or NULL if we're not in temporary mode.

original_scale on the other hand is always a valid value...

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

That's what I understood.  The commit that introduced much of this says
"as discussed on IRC" and I wasn't around for that discussion.

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

That too is my understanding - though it took me a couple of reads to
come to the conclusion, and I'm far from authoritative on this piece of
code.

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

In the... oh, all the clients except the fullscreen one?  I think I
follow.  Thank you for finding yet another head hurting way of
describing what's in that variable.  ;)

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

Ok, I'll try to get to that in the next couple of days.

> 
> 
> 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);
>>  }
>>  
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>