Just a couple quick comments below. I can't fin where this goes, so I'm putting it here: Why are we having compositors send an initial configure event again? Given that we have a serial, tiling compositors can just send a configure and wait until the client responds to the event. Non-tiling compositors will just send 0x0 for "I don't care" right? I seem to recall something about saving a repaint on application launch in the tiling case but I don't remember that well. I'm not sure that I like the required roundtrip any more than the extra repaint. It's probably a wash. On Aug 22, 2014 1:48 PM, "Jasper St. Pierre" <<a href="mailto:jstpierre@mecheye.net">jstpierre@mecheye.net</a>> wrote: > > On Wed, Aug 6, 2014 at 9:39 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: >> >> On Thu, 17 Jul 2014 17:57:45 -0400 >> "Jasper St. Pierre" <<a href="mailto:jstpierre@mecheye.net">jstpierre@mecheye.net</a>> 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. Just thinking out loud here but why not just have the client send a list of locations in order of preference. That way the client can make its popups more interesting without breaking the world. Also, the client probably wants feedback of where ilthe popup is going to be before it renders. I'm thinking about GTK's little popup boxes with the little pointer thing that points to whatever you gicked to get the menu. (I know they have a name, I just can't remember it right now. They're all over the place in GNOME shell.) > >> >> > </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. I'm pretty sure I've seen this somewhere actually. I think it was a compiz plugin. In any case, it's not in any of the usual ones and probably won't be baked into toolkits. > >> >> 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. The attach.x/y are supposed to tell the compositor how the surface resized. There are a lot of places we ignore it including whenever the surface is full-screen. If you specify in the spec that the resize is pinned by the compositor, that's fine. I would agree that set_cursor is the wonky one hear; at least given the way Tue spec is worded. > >> >> 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. Yeah, not sure what to do here. I like the idea of the compositor doing it for the client. Sure, you could use wl_viewport, subsurfaces, and a couple black surfaces for letterboxing. However that is going to be far more difficult for the compositor to translate into overlays/planes than just the one surface and some scaling instructions. > >> > </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 > > _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> >