[PATCH] shell: fix weston_output_mode_switch() usage

[Pekka Paalanen]

On Mon, 22 Sep 2014 11:01:10 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

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

Ehh, do we actually ever pass a custom mode struct? I think we can just
require that the mode is in the output's list... oh, except RDP which
supports arbitrary "modes" and so cannot be enumerated..?

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

I'm not sure... something has to check whether a mode switch is needed
when e.g. activating a window anyway.

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

I think I was there in the discussion, but memory is vague.

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

The alternative would be to send notifications always when the native
mode changes, whether it is the current mode or not. Sending the
notification causes a lot of work to be done, but OTOH changing the
native mode should not happen often (currently never? oh wait, RDP?).

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

No, its really all clients. Even the fullscreen client does not know if
the temporary mode change it requested actually happened.

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

Thank you.

Hardening, could you keep an eye here and make sure we don't break
anything for the RDP backend or such?

I believe some of the details here are only to cater for RDP, like
non-enumerable modes.


Thanks,
pq