[PATCH weston] xdg-shell: Make stable

[Jasper St. Pierre]

On Wed, Aug 6, 2014 at 9:39 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Thu, 17 Jul 2014 17:57:45 -0400
> "Jasper St. Pierre" <jstpierre at mecheye.net> wrote:
>
> Hi Jasper,
>
> I am not reviewing this patch now. Instead, I took xdg-shell.xml from
> Weston master as today, and I'm giving comments on it, as you have
> declared it practically ready for stabilization.
>
> In irc, I remember you mentioned a few more things you'd like to solve,
> like size hints, but I won't specifically comment about those, unless
> it comes up through what's below.
>
> I am not too familiar in how desktops work, what is the state of the
> art on X11, or even all of the concepts, so please allow me some room
> for inaccuracy.
>
> I am reading this protocol specification only, and I am not looking at
> the implementation bits at all. I believe the protocol spec should be
> enough for specifying everything, even though we don't have a good track
> record with that.
>
> Almost all questions and comments I give should probably lead to some
> changes or additions in the documentation answering those questions.
>

Hey Pekka,

I'm going to answer inline here. Anything I've snipped out I agree with and
have already fixed. I've started on a rewrite of the documentation in here.


> This is a single global, yes? What happens if a client binds to this
> global multiple times, therefore creating multiple xdg_shell objects?
> The ping/pong spec needs to define what happens there.
>

 Ping/pongs are on xdg_shell objects, not on surfaces. Semantics of whether
a client is "active" meaning whether all of its xdg_shell objects respond
to pings or just one of them is not defined here. See below.


> Does the compositor have to track, which xdg_* object has been created
> from which xdg_shell object? For ping/pong purposes?
>

Compositors might save all the xdg_shell objects they've created, and every
minute ping every one. "Pinging a surface" may involve pinging the
xdg_shell object that created the surface, or pinging all xdg_shell objects
for the client. Weston implements the former, mutter currently implements
the latter. I may eventually change mutter to do the former as well.


> >       Only one shell or popup surface can be associated with a given
> >       surface.
> >       </description>
> >       <arg name="id" type="new_id" interface="xdg_surface"/>
> >       <arg name="surface" type="object" interface="wl_surface"/>
>
> Need to define an error case for 'surface' already having a role.
>

This has been solved in a generic way by patches on the list.

Where does a client get the serial it needs to pass here?

> How does the compositor process the serial?
>

See the xdg_popup documentation for the explanation of these parameters. I
stripped out all the local text in the "get_xdg_*" method and just said
"see xdg_* documentation", so we can put this in a central place and point
people in the right location.


> >       <arg name="flags" type="uint"/>
>
> What are these flags? Are there no flags defined yet? Would passing an
> unknown flag cause a protocol error?
>

Let's just remove the flags. This stemmed from wl_shell_surface, where
somebody had a flag for transients, but none for popups. I don't expect us
to have flags any time soon, either.


> This talks about unresponsive, the ping talks about alive.
>

Are the two different?


> >       It provides requests to treat surfaces like windows, allowing to
> set
> >       properties like maximized, fullscreen, minimized, and to move and
> resize
> >       them, and associate metadata like title and app id.
> >
> >       On the server side the object is automatically destroyed when
> >       the related wl_surface is destroyed.  On client side,
> >       xdg_surface.destroy() must be called before destroying
> >       the wl_surface object.
>
> This seems to conflict the description of 'destroy' request below.
> Instead of this, we could explain what happens if the wl_surface is
> destroyed before its xdg_surface.
>

Protocol error. I've made this clearer in both places now.


> What about surface/window mapping? To get a window mapped, is it enough
> to just create a xdg_surface and commit connect to the wl_surface? Does
> the order of these operations matter?
>

The way I've always viewed a surface, it's a bag of bits that has some
existing state, like the current buffer attached, the input/opaque regions,
and some others. Creating a role just does something with that state like
putting it on the screen. Committing a surface shouldn't depend on the role
of the surface, unless it's in another mode (like synchronized subsurfaces).

This is obviously not how reality works (cursor surfaces seem to reset
their absolute attachment offset when calling set_pointer), but I'd like to
consider anything else as a weird special case, and we should strive to not
make more of those. You or Kristian can veto me here if you disagree :)

So no, it doesn't matter when commit is called in comparison to giving it a
role. Mapping the window means that it has a window role (xdg_surface, for
example) and that it has a non-NULL buffer attached.

That said, mutter implements the semantics right now that if you create an
xdg_surface, and then at some point commit the surface with no content
currently attached to the surface, it's a protocol error. I don't like the
idea of attaching a NULL buffer to unmap a window surface, but it's weird
that the behavior of commit changes based on the existence of a role object.

If the order matters, should xdg_shell.get_xdg_surface raise a protocol
> error if the wl_surface alrady has content (i.e. a non-NULL wl_buffer
> committed)?
>

No.


> What about the initial size of the window? Does this interface
> automatically deliver any events the moment the protocol object has
> been created?
>

Thanks for reminding me about this. This is something we need to stick in
here at the last minute: an initial configure to the client after
get_xdg_surface should be mandated.


> >     <request name="destroy" type="destructor">
> >       <description summary="remove xdg_surface interface">
> >       The xdg_surface interface is removed from the wl_surface object
> >       that was turned into a xdg_surface with
> >       xdg_shell.get_xdg_surface request. The xdg_surface properties,
> >       like maximized and fullscreen, are lost. The wl_surface loses
> >       its role as a xdg_surface. The wl_surface is unmapped.
> >       </description>
>
> Very good.
>
> What about, say, xdg_popup surfaces associated to this one?
>

Yeah, we should unmap those too.


> I think 'set_parent' already describes what happens with xdg_surfaces
> who had 'set_parent' called with this surface as argument.
>

This is actually something I changed to match mutter's semantics: it's
illegal to unmap a parent window before unmapping a child window.


> >     </request>
> >
> >     <request name="set_parent">
> >       <description summary="surface is a child of another surface">
> >       Child surfaces are stacked above their parents, and will be
> >       unmapped if the parent is unmapped too. They should not appear
> >       on task bars and alt+tab.
>
> Any other behavioral changes compared to an xdg_surface without this
> request called?
>

The "taskbar / alt-tab" thing is actually wrong, I think, and stemmed from
transient windows. mutter might hide certain kind of "child windows" (ugh,
I hate this term in the context of subsurfaces) from appearing on the
taskbar, but Wayland can't get those types yet. Perhaps this is an idea for
a future addti


> >       </description>
> >       <arg name="parent" type="object" interface="wl_surface"
> allow-null="true"/>
>
> What kind of wl_surfaces are allowed here, what raises a protocol error?
>
> >     </request>
> >
> >     <request name="set_app_id">
> >       <description summary="set surface class">
>
> Id or class? Pick one. I would kind of prefer not id, because it is
> quite overloaded term, but if it is the term what DEs nowdays use, then
> so be it.
>

Old documentation, came from wl_shell_surface, which was reusing
terminology from X11: WM_CLASS. Fixed.


> >       Set an id for the surface.
> >
> >       The app id identifies the general class of applications to which
> >       the surface belongs.
> >
> >       It should be the ID that appears in the new desktop entry
> >       specification, the interface name.
>
> Could we have a reference to the specification here?
>
> From some very old memory, I thought this would be the .desktop file
> path, but apparently this is the... the... dbus interface name like
> thing, right? Like "org.gnome.shotwell" or whatever.
>
> And the DE will use this app_id to find the .desktop file to get an
> icon for this window, etc.?
>

Yeah. I've updated the documentation to make this clearer.


> >       </description>
> >       <arg name="app_id" type="string"/>
>
> Are there any cases where a string could raise a protocol error?
>

No.


> >     <request name="show_window_menu">
> >       <description summary="show the window menu">
> >         Clients implementing client-side decorations might want to show
> >         a context menu when right-clicking on the decorations, giving the
> >         user a menu that they can use to maximize or minimize the window.
> >
> >         This request asks the compositor to pop up such a window menu at
> >         the given position, relative to the parent surface. There are
> >         no guarantees as to what the window menu contains.
> >
> >         Your surface must have focus on the seat passed in to pop up the
> >         window menu.
>
> Is any focus, e.g. pointer, really enough? I.e. just moving the cursor
> over a window would allow it to pop this up, without needing a
> button-down? I see for instance wl_pointer.enter carries a serial, will
> that do?
>

I've reworded this (and all the other places we talk about grabbing and use
wl_seat/serial pairs) to mention that it must be in response to a user
action. The implementation in mutter checks that it's in response to a
pointer or touch event. The fact that it doesn't allow keyboard is probably
a bug.


> How will be client know, when the window menu is gone?
>

Why does it need to know? This is just like some other compositor popup
came up and stole focus from it. Or screen lock, or...

Oh btw. we still don't have the popup placement negotiation protocol
> (to avoid popup going off-screen), but the draft I read a long time ago
> assumed, that the client knows in advance the size of the popup
> surface. We don't know the size here. I'm not sure if solving that
> would change something here.
>

The compositor can pivot the menu around a point, which is very likely
going to be the current cursor position. Specifying a box would be overall
more correct if the popup instead stemmed from a button (so it could appear
on all sides of the box) but I don't imagine that clients will ever use
this on a button. We could add it for completeness if you're really
concerned.


> >     </request>
> >
> >     <request name="move">
> >       <description summary="start an interactive move">
> >       Start a pointer-driven move of the surface.
>
> or touch, or maybe even keyboard?
> Could we implement keyboard-based window movement with this?
>

Currently, both mutter and weston check to see that either the pointer or
touch are pressed. This is designed for the "I pressed the titlebar" case.
It could be used for a keyboard shortcut for initiating window move, but we
implement that as a compositor keybinding. Use cases for client-initiated
keyboard move are welcome.

>       This request must be used in response to a button press event.
> >       The server may ignore move requests depending on the state of
> >       the surface (e.g. fullscreen or maximized).
>
> Again, how does the client know, when the move has ended?
>

Should it care? The compositor can move the window at any time it wants,
without the client knowing. I don't see the value in telling the client
when it has moved, but only sometimes.


> >     </request>
> >
> >     <enum name="resize_edge">
> >       <description summary="edge values for resizing">
> >       These values are used to indicate which edge of a surface
> >       is being dragged in a resize operation. The server may
> >       use this information to adapt its behavior, e.g. choose
> >       an appropriate cursor image.
>
> And automatically reposition the surface accordingly? (Resize from
> top/left.)
>

Surfaces don't care about positioning, so it doesn't need to know.


> >       </description>
> >       <entry name="none" value="0"/>
> >       <entry name="top" value="1"/>
> >       <entry name="bottom" value="2"/>
> >       <entry name="left" value="4"/>
> >       <entry name="top_left" value="5"/>
> >       <entry name="bottom_left" value="6"/>
> >       <entry name="right" value="8"/>
> >       <entry name="top_right" value="9"/>
> >       <entry name="bottom_right" value="10"/>
>
> No use for centered resize, e.g. left_right?
>

You might recognize that the enum is actually a bitfield. Clever users
might notice this and try to resize with a value like LEFT | RIGHT. Clever
users get protocol errors.

If you know of a WM that uses centered resize, let me know and I'll check
it out. I can't imagine a good UI for this.


> What about touch and keyboard, not just pointer? Or any other input
> style. Resize with keyboard might be a nice proof of concept, but with
> this method rather than what weston-resizor does, no?
>

That's implemented by a WM keybinding. Again, this is more to initiating
resize dragging from decoration edges.


> So with this, the compositor is handling the repositioning of the
> surface automatically, is it not? No need for the client to fiddle with
> wl.surface.attach arguments x,y?
>

Yeah, we should probably mention that the surface is anchored. In fact,
attach x/y are completely ignored when in the "resizing" state. I keep
discovering more reasons to hate attach x/y.


> Could also mention, that the client will be receiving configure events
> while interactive resizing happens, and when the operation ends.
>

Sure.

>       </description>
> >       <entry name="maximized" value="1" summary="the surface is
> maximized">
> >         The surface is maximized. The window geometry specified in the
> configure
> >         event must be obeyed by the client.
>
> I suppose "the configure event" really means "the latest configure
> event the client has received"? A client may receive many configure
> events before it gets to repainting, right?
>
> >       </entry>
> >       <entry name="fullscreen" value="2" summary="the surface is
> fullscreen">
> >         The surface is fullscreen. The window geometry specified in the
> configure
> >         event must be obeyed by the client.
>
> Really? So, will we rely on wl_viewport for scaling low-res apps to
> fullscreen? No provision for automatic black borders in aspect ratio or
> size mismatch, even if the display hardware would be able to generate
> those for free while scanning out the client buffer, bypassing
> compositing?
>

Since we have a big space for these states, I suppose we could do those
> mismatch cases in separate and explicit state variants of fullscreen,
> could we not?
>

I explicitly removed this feature from the first draft of the patch simply
to make my life easier as a compositor writer. We could add additional
states for this, or break fullscreen into multiple states: "fullscreen +
size_strict" or "fullscreen + size_loose" or something.

I am not familiar enough with the sixteen different configurations of the
old fullscreen system to make an informed decision about what most clients
want. Your help and experience is very appreciated. I'm not keen to add
back the matrix of configuration options.

>       </entry>
> >       <entry name="resizing" value="3">
> >         The surface is being resized. The window geometry specified in
> the
> >         configure event is a maximum; the client cannot resize beyond it.
> >         Clients that have aspect ratio or cell sizing configuration can
> use
> >         a smaller size, however.
>
> Oh, there is an explicit state for this, ok.
>

This is about the interactive resizing. The comment makes me think you
might be confused and think it's related to fullscreen.


> >       The width and height arguments specify a hint to the window
> >         about how its surface should be resized in window geometry
> >         coordinates. The states listed in the event specify how the
> >         width/height arguments should be interpreted.
>
> The window geometry coordinates are not clearly specified. Are they
> defined by the set_window_geometry such that 0,0 here means x,y there?
>

What this means is that if you get a configure event with a width/height of
let's say 640x480, then the window geometry of the window should be
640x480. If you have 10px shadows around the edges of your window, then the
buffer will be 660x500, and you will call set_window_geometry(10, 10, 640,
480).

It doesn't mean anything about the current geometry set. It means something
about the new window geometry you should be setting.

See the documentation of set_window_geometry for the definition of "window
geometry coordinate space" and the rationale behind it.


> >         A client should arrange a new surface, and then send a
> >         ack_configure request with the serial sent in this configure
> >         event before attaching a new surface.
>
> "Before attaching a new surface" is vague. Does this mean "immediately
> before issuing wl_surface.commit on the associated wl_surface"?
> One might also argue about "immediately", I think your point is that
> the very next commit after a ack_configure applies all the changes
> suggested by that configure. Right?
>

It allows compositors to know where in the event stream this client is. For
instance, the compositor might want to maximize a constantly animating
surface. To do that, it will need to move the surface's view to the
top-left of the screen, but in order to know when it can move the view, it
needs to know when the proper buffer is attached so we don't see a two-step
change of the surface moving to the top-left, then becoming maximized. The
surface is constantly animating, so there will be extra attaches between
the round-trips of "set_maximized", "configure" and then "attach/commit".

The compositor could use heuristics like "when I see window geometry
equivalent to what I sent out, then assume it's the surface we're waiting
on", but better design this into the protocol.

(Note that this isn't specific to constantly animating surfaces. When
releasing the maximize button in weston-terminal, it first attaches a new
surface of the maximize button going from the pressed state back to hover
state at the small size, before it receives the configure. The compositor
sees two attaches: small window with hover, and big maximized window.)


> What should a client do, if it receives states it does not understand?
>

Ignore it.


> What if the compositor sends a state that demands something, and the
> client does not obey the demand, because a) the client is broken, or b)
> the client is not broken, but does not know the state?
>

There was a plan for a complex handshaking protocol, but I'm tempted to say
that clients must understand all core states (up to their surface version),
and clients need to ignore states in extensions they don't understand.
Compositors that send core states that are below a surface's version are
broken.

Handshaking for extension states is done by some other unspecified
mechanism. In mutter/GTK+, we use the existence of a bound global from our
private protocol (gtk_shell) to know the client knows about the state.

>       <arg name="serial" type="uint"/>
>
> This serial is in a completely new "serial name space", isn't it?
> Not to be confused with e.g. input serials.
>

It's a serial. wl_display_next_serial serial. I thought a serial was a
serial was a serial.

>     </event>
> >
> >     <request name="ack_configure">
> >       <description summary="ack a configure event">
> >         When a configure event is received, a client should then ack it
> >         using the ack_configure request to ensure that the compositor
> >         knows the client has seen the event.
> >
> >         By this point, the state is confirmed, and the next attach should
> >         contain the buffer drawn for the configure event you are acking.
>
> Again, "attach" is vague, you probably mean wl_surface.commit.
>
> Is it allowed to skip acks? If a client receives configure events with
> serials 3,4,5, can it just ack 5 directly?
>

Yes. I thought we had this written down somewhere.


> What if you ack 4, ack 5, without a commit in between?
>

Right now, we just update the request for the new serial.


> What if you ack 5, ack 3 (going backwards, i.e. client bug), would
> there be a protocol error?


No. Again, right now we just update the serial that we'll use on the
client. Keep in mind that it could be correct if the serial has wrapped
around.


> Or if you ack a serial the compositor has
> not sent yet? Or ack the same serial twice?


Right now, we only use this information for pending moves, so surfaces
might move too early or too late. I don't really care what happens to
illegal clients, but we can't really validate this properly. Is "undefined
behavior", which perhaps includes a protocol error, OK to you?


> >       </description>
> >       <arg name="x" type="int"/>
> >       <arg name="y" type="int"/>
> >       <arg name="width" type="int"/>
> >       <arg name="height" type="int"/>



> Is there a protocol error, if width or height <= 0?
>

Let's say there is, even if there isn't one right now.


> >     </request>
> >
> >     <request name="set_maximized" />
> >     <request name="unset_maximized" />
>
> These two requests are undocumented.
>

... and? Do we need documentation here? It would just be <description
summary="set the window as maximized">Maximize the surface</description>.
Documentation by reshuffling words isn't any more useful than no
documentation.


> >
> >     <request name="set_fullscreen">
> >       <description summary="set the window as fullscreen on a monitor">
> >       Make the surface fullscreen.
> >
> >         You can specify an output that you would prefer to be fullscreen.
> >       If this value is NULL, it's up to the compositor to choose which
> >         display will be used to map this surface.
> >       </description>
> >       <arg name="output" type="object" interface="wl_output"
> allow-null="true"/>
>
> The issue I raised at the fullscreen state enum... otherwise ok.
>
> >     </request>
> >     <request name="unset_fullscreen" />
> >
> >     <request name="set_minimized" />
>
> And a couple more undocumented requests. Maybe answer the FAQ "where is
> my unset_minimized?" I.e. how do you get out of minimized, since there
> is no obvious request.
>
> I would like to see an overview of how this state system works. Can a
> client issue set_maximized at any time? What happens then? At what
> point does the window get drawn in maximized state?
>

Calling set_maximized requests for the compositor to make the surface
maximized. The compositor will then send the surface a configure event
containing the correct set of states ("maximized", "activated"), and the
correct width/height (640x480). The client then takes this information,
draws its new state, attaches it, calls ack_configure, and then commits.

I'm not sure where we should put this overview. Perhaps in xdg_surface?

Can the compositor choose to set arbitrary states on its own will?


Yes. If the user hits the maximize keybinding, the client will get a
configure event out of the blue telling the surface to maximize itself.

>       The popup grab continues until the window is destroyed or a mouse
> >       button is pressed in any other clients window. A click in any of
> >       the clients surfaces is reported as normal, however, clicks in
> >       other clients surfaces will be discarded and trigger the callback.
>
> So the client will be getting input 'enter' and 'leave' events as
> usual, but only this client?
>

Yep. An "owner-events grab" (X11 parlance) means that the client receives
events for all its surfaces, not just the popup. This is so that when
navigating through traditional submenus, the user can navigate back over to
the submenu to the left, and that will receive motion events, allowing the
client to start the timer and dismiss the top menu popup.

I rewrote the documentation to make this and the other concerns clearer.

What if the user alt+tabs during a popup?
>
> When a popup is open, are any xdg_surface operations on the client's
> other surfaces invalid? Can a client create a second popup? What
> happens then?
>

Submenus work by nesting xdg_popups. So, a client creates an xdg_popup B
with parent xdg_surface A, then xdg_popup C with parent xdg_popup B, etc.

I don't know what it means to have two popups for an xdg_surface. We can
make up some strict semantics for now. Let's say it's a protocol error to
make more than one popup, if it's not parented to the existing topmost
popup.

>
> >       The x and y arguments specify the locations of the upper left
> >       corner of the surface relative to the upper left corner of the
> >       parent surface, in surface local coordinates.
>
> This should go with the get_xdg_popup, no?
>

I just put "see xdg_popup" in the docs for "get_xdg_popup".


> >     </request>
> >
> >     <event name="popup_done">
> >       <description summary="popup interaction is done">
> >       The popup_done event is sent out when a popup grab is broken,
> >       that is, when the users clicks a surface that doesn't belong
> >       to the client owning the popup surface.
>
> "The client should destroy the xdg_popup object." I presume?
> Will re-use of the wl_surface be allowed?
>

You can call get_xdg_popup on the same wl_surface again, but you can't
transform it into any other role.


> >       </description>
> >       <arg name="serial" type="uint" summary="serial of the implicit
> grab on the pointer"/>
>
> What is this serial used for?
>
> Oh, is it because the client might not get a input 'enter' event if the
> input focus is already on some other surface of the client?
>

No. It's the serial of the *implicit grab on the pointer* -- that is, the
serial we passed to get_xdg_popup. I have no idea why it's here, actually.
Nothing uses it. Removed it in my local branch.


> >     </event>
> >
> >   </interface>
> > </protocol>
>
>
> Okay, phew. Nothing drastically broken that I can see, if not maybe that
> missing destructor. :-)
>

Thank you for your really good review. You really took the time and thought
it through. I'll have a few more patches going out soon that rewrites the
docs for xdg-shell and corrects all of the issues mentioned.


> Lots and lots of details to specify, though.
>

That's the danger thing: making a spec so dense and thorough it becomes
unreadable.


>
> Thanks,
> pq
>



-- 
  Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140822/78ade21f/attachment-0001.html>