My comments on the WM spec

[Sasha_Vasko osca state mo us]

<snip>
>Regards,
>                                        Owen
>
> _NET_NUMBER_OF_DESKTOPS
> 
>  I find the language
> 
>    _NET_VIRTUAL_ROOTS MUST be set  store the new number of desktop
>    virtual root window IDs
> 
>  a bit confusing.
> 
>  I would suggest:
> 
>    The contents of _NET_VIRTUAL_ROOTS MUST be altered so that the
>    number of window IDS it contain is new_desktops_number.

It makes it sound as if any kind of Window ID can be there :)
perhaps like so: 

The contents of _NET_VIRTUAL_ROOTS MUST be updated so that the
number of virtual root window IDS it contain is new_desktops_number.

> 
>  (If that is the proper interpretation; I'm not completely sure
>  about _NET_VIRTUAL_ROOTS, see below)
> 
>  I would also suggest that language be added indicating that
>  if _NET_CURRENT_DESKTOP need to be changed or windows need
>  to be moved to a different desktop, these changes must
>  be changed _before_ _NET_NUMBER_OF_DESKTOP is changed.

Well, good practice would be to see what exactly you are about to do:
If you are shrinking the list of desktops - then you have to update
property PRIOR to actually removing desktops and moving windows, 
so that Pagers and whoever else, will not attempt to use desktops that are
about to be removed.
If you are expanding list of the desktops - then yes,  you have to 
create desktops first and THEN update property.

All of that comes from the standard practice that you have to create 
resource prior to actually advertising it, and you have to stop 
advertising 
prior to dropping resource. Dunno if it is actually worse mentioning 
in the specs, even thou there are some rather unaducated ppl writing 
window managers ( remmebering myself few years ago ).

> 
> _NET_DESKTOP_GEOMETRY
> 
>  Its probably worth saying that this size must be equal or
>  greater than the actual dimensions of the screen
>  as determined by ScreenWidth, ScreenHeight.

I'd prefer it to be a ScreenWidth * multiplier1 , 
ScreenHeight * multiplyer2, sine it would allow for whole number of 
viewports(screens) on each desk in each direction.
But this is not really critical since window manager is allowed to ban
any illigal sizes that it chooses not to support, besides one does 
not want to restrict specs like that.

> 
>  [ Current proposals to make it possible to change the size of the
>  screen on the fly might cause this to be momemtarily violated when
>  changing screen size ]

Any app that reads this or any other property should be prepared for 
possible misconfiguration. But yes allowed ranges for values should be 
mentioned in the specs.
I double that.

> 
>  The specification of the format for _NET_DESKTOP_GEOMETRY
>  client messages is missing.

Yep, it should be something like :
_NET_DESKTOP_GEOMETRY
        message_type = _NET_DESKTOP_GEOMETRY
        format = 32
        data.l[0] = new_width
        data.l[1] = new_height

Window Manager MAY choose to ignore that message, in which case 
_NET_DESKTOP_GEOMETRY property will remain unchanged.


>  Also, I would recommend making this section correspond to
>  _NET_NUMBER_OF_DESKTOPS by saying that the window manager is not
>  required to honour _NET_DESKTOP_GEOMETRY client messages.

That is correct. /me agrees 

> 
> _NET_DESKTOP_VIEWPORT
> 
>  Might also be useful to specify the constraints that
> 
>   0 <= x < desktop geometry width - ScreenWidth
>   0 <= y < desktop geometry height - ScreenHeight
> 
>  Somewhere, the spec should to specify whether coordinates for client
>  windows when mapping the window or in ConfigureRequest are taken with
>  respect to the viewport origin or the desktop origin. I believe the
>  standard is to use the desktop origin, but would have to check what
>  various window managers do.

That is actually major pain in the neck.
Normal practice is to map window on current viewport, so if your window 
requests +100+100 and your currect viewport is +1024+768 then resulting 
position in virtual space will be +1124+868, position reported back to 
window will still be +100+100 (with adjustments for frame size/gravity )
untill viewport is changed.

> 
>  Again the client message format is missing

_NET_DESKTOP_VIEWPORT
        message_type = _NET_DESKTOP_VIEWPORT
        format = 32
        data.l[0] = new_vx
        data.l[1] = new_vy

Window Manager MAY choose to ignore that message, in which case 
_NET_DESKTOP_VIEWPORT property will remain unchanged.

> 
> _NET_DESKTOP_NAMES
> 
>  The type and format are not specified. Is this the standard
>  NUL-separated list of strings?

_NET_DESKTOP_NAMES, UTF8_STRING[]

NULL separated list of strings in UTF-8 encoding, 
with additional NULL at the end of the list (to match other standard X 
properties ).

> 
>  Also, note typo: "Window Mangaer"
> 

> _NET_SUPPORTING_WM_CHECK
> 
>  "set with the same value" is potentially unclear. Better to say
>  "also set to the ID of the child window". And I think it
>  is useful to extend the rational here: the property on the
>  child window is used so that an active window manager can
>  be distinguished from a stale _NET_SUPPORTING_WM_CHECK
>  property that happens to point to another window. If the
>  _NET_SUPPORTING_WM_CHECK window on the client window is missing
>  or not properly set, clients should assume that no conforming
>  window manager is present.
> 
>  Reading over the ICCCM again, I realized that this really
>  is the wrong way of doing things - really we should be
>  working on top of the selection mechanism described in
>  sections 2.8 and 4.3. The X selection mechanism is meant
>  the way in X to control ownership of shared resources,
>  and the ICCCM describes how starting one window manager
>  should cause the previous window manager to cleanly give
>  up control of the root window. Of course, I don't know if any window
>  managers actually conform to the ICCCM in this respect.

I don't know of anyone that does it either. At least AfterStep does
not do that (heh, sounds like another piece of work todo).
It really sound cleaner to use selections for this purpose
Perhaps the following target on selection WM_Sn :

_NET_SUPPORTED  ATOM            list of atoms identifying supported hints.
_NET_VERSION    INTEGER two numbers identifying supported 
                                        version of specs.
I don't think its nice to do such a change so close to final release,
at the same time if it is not done, then future versions of this specs
will either have to stick to current way, or will have to mandate both 
ways for compatibility purposes.

> 
> _NET_WORKAREA
> 
>  Should say that the coordinates here are taken with respect
>  to the the current viewport. (If they are.)

It really should be screen coordinates.

> 
> _NET_VIRTUAL_ROOTS
> 
>  Is this a) an (unordered) list of all virtual root windows other
>  than the real root window, or b) a list of the window that
>  corresponds to each desktop, either real or virtual?
> 
>  I'm a little nervous about including this since it is known
>  that using virtual root windows introduces a number of problems
>  for various operations (the ICCCM claims that a protocol
>  extension would be needed to properly implement virtual root
>  windows.)

Problem is that Virtual roots are the only way to correctly implement
multiple virtual desktops. All other ways are either ICCCM incompliant
(those that unmap windows when desktops are switched ), or has to do wierd
window moving , in order to get them off screen.
Virtual roots are no different from other frame decorations, and as such 
conform to ICCCM. After all ICCCM says that clients should not try and 
determine how much and what kind of frame  decorations they have, and 
try to move/resize itself, based on this information. 
So the problems with virtual root arrise only in specific apps that tryes 
to do "illegal" ( from point of view of ICCCM ) things - namely
file managers that tries to place icons on desktop, and apps like
xident - that grab pointer on root in order to let user choose any 
particular
client. Both of this apps has to know what virtual roots are in order to 
work correctly. Since those things are usual part of any desktop 
environment,
then the list of virtual roots advertised by window manager is must-have
if this specs choose to support virtual desktops.
 
> _NET_CLOSE_WINDOW
> 
>  There is a formatting problem with client message format
>  specification.

it should be :
_NET_CLOSE_WINDOW
        window = target_client_window
        message_type = _NET_CLOSE_WINDOW
        format = 32
        data.l[0] = 0 /* may be used later */

> 
> _NET_WM_MOVERESIZE
> 
>  A thing to be careful about here is that this operation is
>  essentially talking about transferring a pointer grab from
>  one window to another. It needs to be pointed out that the
>  window manager has no guarantee of getting the release from
>  the mouse click that started the operation, and needs to
>  check the state field of motion events to safely know when
>  to release the grab.

it may be sufficient to say that client should not grab pointer,
or release garb if it already has one, prior to sending this message 
to the Window Manager

> 
>  (With this protocol there is no way of getting around the fact
>  that a fast press/release/press can be misinterpreted.)
> 
>  For proper operation, I think you really should include
>  the mouse button that started the operation, not just the
>  x and y coordinates.

hmm, Mouse Button is indeed needed if window managers wants to grab 
correct (pressed) button.

> 
>  The name of the enumerations probably really should use
>  North,South,East,West, instead of top,right,...to correspond with
>  similar names used in X elsewhere.

I'm not so sure, this message identifies part of the client window,
and you never call left border of the window its West side.

> 
> _NET_WM_NAME
> 
>  The property type for this property isn't specified - I would
>  suggest UTF8_STRING. _NET_WM_ICON_NAME is missing.

my opinion is that both of those properties are redundand and 
WM_NAME /WMICON_NAME should be used instead, but what do I know.

> 
> _NET_WM_STATE
> 
>  I don't have concrete suggestions here, but I will note that
>  this property feels very hackish to me:
> 
>   - It groups together different things
> 
>     - current state of the window (SHADED/MAXIMIZED)
>     - the way the window should be treated
>       (modal, skip taskbar, sticky)

The only questionable part here is skip_taskbar, but since
it really is a status of the window vs. its type - it should be here.

> 
>   - When mapping a window, does a missing atom mean
>     that the state should is "unset" or "not specified" -
>     this doesn't matter so much for, say, SHADED,
>     but for MODAL and SKIP_TASKBAR, it is quite likely
>     that the window manager may, e.g., determine automatically
>     determine that a window should be MODAL by examining,
>     for instance, its WINDOW_TYPE and/or TRANSIENT_FOR
>     properties.

"unset" and "not specified" in this context really means the same - 
unset. By default all of those flags considered to be unset, 
user may selectively turn them on, but you never really expect user to 
come up and say "Hey I really want to prvent this window from starting up
in Shaded state, no matter what my window manager thinks!", even if it is 
so - 
its more like window manager configuration problem.

> 
>   - The concept that exactly two properties may be changed
>     together seems very odd to me.

its not exactly - its up to.
merely a convinience feature. So if you need to maximize window - 
you don't have to send 2 events, one for horizontal, another for vertical.

> 
>   Assuming that we want to keep mixing all these types of "state"
>   together, the way I would do this property is to make it
>   a bitfield, or actually, two bitfields - one for the bits
>   that are present, and one for the the state of these
>   bits.
> 
>   The client message would then include a bitfield, and an
>   operation - set, clear, toggle, and unset_to_default.
>   But I really don't understand the coherence of the different
>   atoms currently in the property.

there is no sense in default status. Status of the window is dynamic
property and is evolving over time, based on user actions. The only 
correct way to unset it to default would be to withdraw window and 
map it again, thus causing WM to forget its history.
 
> _NET_WM_ICON_GEOMETRY
> 
>  How do we resolve who sets this property? What if there
>  are multiple task lists?

hehehe, I envision them fighting for this property :)
As it is some sort of selection mechanism is needed here,
just like for _NET_SUPPORTED_WM_CHECK above.

> 
> _NET_WM_ICON
> 
>  It currently says that 1 byte should be each of the width and height
>  I think this is probably a typo and it is supposed to be the
>  first two elements. (This would correspond to the format being 32.)
>  If it isn't a typo, it should be changed, since while 256x256
>  is just barely big enough for icons for the forseeable future,
>  it is limiting, and we might want to use image specifications
>  like this elsewhere.
> 
>  Using CARDINAL for the property type is a bit suspect, since the type
>  of the data is really something a lot more specific than
>  just a bunch of cardinals - its a simple image format.
> 
>  Using a specific type also is a little more forward looking since we
>  could later add additional formats, and only require changes
>  to window managers, and not to clients.
> 
> _NET_WM_HANDLED_ICONS
> 
>  What's the format (and contents?) of this property?

again, some sort of selection mechanism, perhaps same as for the 
_NET_WM_ICON_GEOMETRY, is needed here.


> 
> _NET_WM_PING
> 
>  Is the window field of the event really supposed to be
>  set to the root window? Leaving it set to the client window
>  is
> 
>  A problem through out the spec is that the event mask for sending
>  events to the root window is not specified. But it isn't possible
>  to receive ClientMessage events without setting a mask in
>  XSendEvent. Probably this should be specified as
>  SubtructureNotify|StructureRedirectMask, as it is for similar
>  events in the ICCCM.
> 
>  As noted before, "immediately" should be replaced by
>  "as soon as possible".
> 
> Implementation Notes:
> 
> As noted before, interpreting WM_TRANSFIENT_FOR pointing to the
> root window to mean transient for the group breaks legacy applications.
> The statement "is extremely unlikely to break anything" is just
> wrong; and we should not require or even recommend this
> behavior. (To repeat the argument as to why it is wrong,
> 
> Such hints should either be ignored or be treated to be equivalent to
> using "_NET_WM_WINDOW_TYPE_DIALOG" without any explicit
> TRANSIENT_FOR properties.
> 
> The Window Movement section needs some fixing:
> 
>  If you move a window with, for insance, NorthEastGravity to (x,y), the
>  effect is _not_ to move the upper left corner of the frame to
>  (x,y). Instead, if the size of the window is (width, height)
>  _excluding the frame_, then the effect is to move the upper left
>  corner of the frame to (x+width,y).

GRRRR
I'll never get it right. Here is another attempt :

When interpreting both initial geometry when transfering window from 
Withdrawn state, and (x,y) position in XMoveWindow( w, x, y) requests, 
window manager will first convert (x,y) into the client reference point - 
(ref_x, ref_y) based on gravity, and then will move frame window based on 
changes in this reference point and gravity again.

reference point is calculated like so :
Gravity                                 ref_x value

West                                    x
Static                          x+border_width 
East                                    x+border_width+width+border_width
Other(Center or missing)        x+border_width+(width/2)

Gravity                                ref_y value

North                                   y
Static                          y+border_width
South                                   x+border_width+height+border_width
Other(Center or missing)        x+border_width+(height/2)

where border_width is client window's border width at the time when it is 
transfered from Withdrawn position , or in last ConfigureWindow request.
width and height are the client window's size without any 
borders/decorations.

Reference point is used by Window Manager to place frame window like so:

StaticGravity:
        window's left top corner will be placed at (ref_x,ref_y)

NorthWestGravity:
        window frame's left top corner will be placed at (ref_x,ref_y) 

NorthGravity:
        window frame's top center will be placed at (ref_x,ref_y) 

NorthEastGravity:
        window frame's right top corner will be placed at (ref_x,ref_y) 

EastGravity:
        window frame's right center will be placed at (ref_x,ref_y) 

SouthEastGravity:
        window frame's right bottom corner will be placed at (ref_x,ref_y) 

SouthGravity:
        window frame's bottom center will be placed at (ref_x,ref_y) 

SouthWestGravity:
        window frame's left bottom corner will be placed at (ref_x,ref_y) 

WestGravity:
        window frame's left center will be placed at (ref_x,ref_y) 

CenterGravity:
        window frame's center will be placed at (ref_x,ref_y)

All the subsequent XResizeWindow requests will be serviced so
that reference point does not move.


> 
>  Also, the list is missing North,South,East, and West gravity.
> 
> For "Killing Hung Processes", it needs to be noted that if a window
> does not respond to _NET_WM_PING, it does _not_ necesarily mean that
> the process is dead, and window managers MUST ask for confirmation
> from the user before proceeding to kill the process.
> 
> Throughout the document we have "UTF8" - I would suggest writing this
> as UTF-8 instead, as this is the way that it is officially written by
> the Unicode consortium.
> 
> The document needs a references section for at least the Unicode
> standard and the ICCCM.