[Xcb] documentation patches

[Uli Schlachter]

On 26.03.2012 14:18, Michael Stapelberg wrote:
> Hi Julien,
> 
> Excerpts from Julien Danjou's message of 2012-03-26 10:38:53 +0200:
>> Could you resend your patches or point me at it, so I can take a look?
>> I'll merge them if nobody has a good reason to object.
> Sure, I’ve attached the most recent version to this mail.
> 
>> Thanks Michael, and sorry for the delay.
> Thanks for reviewing it! :)

Since the patches are huge, I decided to search for TODOs instead:

+      <field name="depth"><![CDATA[
+Specifies the new window's depth (TODO: what unit?).
+
+The special value `XCB_COPY_FROM_PARENT` means the depth is taken from the
+`parent` window.
+      ]]></field>

The bit depth for CreateWindow should be the number of bits per pixel. So that
doesn't have any unit, it's a count.

+      <field name="border_width"><![CDATA[
+      TODO:
+
+Must be zero if the `class` is `InputOnly` or a `xcb_match_error_t` occurs.
+      ]]></field>

No idea what this TODO means, but just in case: border_width is the width of the
window border in number of pixels.

[I skipped all the errors. There isn't much to say about BadDrawable and
BadMatch feels like an all-purpose error code to me. ;-) ]

+      <brief>Changes a client's save set</brief>
+      <description><![CDATA[
+TODO: explain what the save set is for.
+
+This function either adds or removes the specified window to the client's (your
+application's) save set.
+      ]]></description>

Taken from http://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html
(license issues?):

> The save set of a client is a list of other clients' windows that, if
> they are inferiors of one of the client's windows at connection close,
> should not be destroyed and that should be remapped if currently unmapped.
> Save sets are typically used by window managers to avoid lost windows
> if the manager terminates abnormally.

> +      <error type="Window"><![CDATA[
> +The specified window does not exist. TODO: any other reason?
> +      ]]></error>

(This is from ConfigureWindow)

Looking at the possible XCB_CONFIG_WINDOW_*-values, I can only think about
XCB_CONFIG_WINDOW_SIBLING producing a Window error, but that should be covered
by "the specified window does not exist", too.

+      <brief>Get atom identifier by name</brief>
+      <description><![CDATA[
+Retrieves the identifier (xcb_atom_t TODO) for the atom with the specified
+name. Atoms are used in protocols like EWMH, for example to store window titles
+(`_NET_WM_NAME` atom) as property of a window.

What's this TODO about? If it is about "identifier":
I know that this doesn't explain what an atom is, but I'd suggest "Retrieves the
atom for the specified string".

+TODO: talk about `type`
+
+TODO: talk about `delete`
+
+TODO: talk about the offset/length thing. what's a valid use case?

I have nothing to say about the first two TODOs, but for the first ones: There
is a maximum length for the reply to GetProperty. If a property is larger than
that limit, you have to transfer it in "parts". I don't think I ever did that...

+    // TODO: a reasonable long_length for WM_NAME?
+    cookie = xcb_get_property(c, 0, window, property, type, 0, 0);
+    if ((reply = xcb_get_property_reply(c, cookie, NULL))) {
+        int len = xcb_get_property_value_length(reply);
+        if (len == 0) {
+            printf("TODO\\n");
+            free(reply);
+            return;
+        }
+        printf("WM_NAME is %.*s\\n", len,
+               (char*)xcb_get_property_value(reply));
+    }

A quick grep through awesome's source code says that UINT_MAX works fine. Dunno
how reasonable that is.
For the second TODO: How about puts("Error!");

+      <brief>Sets the owner of a selection</brief>
+      <description><![CDATA[
+Makes `window` the owner of the selection `selection` and updates the
+last-change time of the specified selection.
+
+TODO: briefly explain what a selection is.
+      ]]></description>

http://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html says:

A selection can be thought of as an indirect property with dynamic type; that
is, rather than having the property stored in the server, it is maintained by
some client (the “owner”). A selection is global in nature and is thought of as
belonging to the user (although maintained by clients), rather than as being
private to a particular window subhierarchy or a particular set of clients. When
a client asks for the contents of a selection, it specifies a selection “target
type”. This target type can be used to control the transmitted representation of
the contents. For example, if the selection is “the last thing the user clicked
on” and that is currently an image, then the target type might specify whether
the contents of the image should be sent in XY format or Z format. The target
type can also be used to control the class of contents transmitted; for example,
asking for the “looks” (fonts, line spacing, indentation, and so on) of a
paragraph selection rather than the text of the paragraph. The target type can
also be used for other purposes. The protocol does not constrain the semantics.

(This TODO is present more than once, I don't think you want to explain
"selection" twice, could the second one be replaced with a "see <first one>"?)

+      <field name="event_mask"><![CDATA[
+Specifies which pointer events are reported to the client.
+
+TODO: which values?
+      ]]></field>

Example use of xcb_grab_pointer() taken from the awesome WM:

  xcb_grab_pointer_unchecked(globalconf.connection, false, root,

    XCB_EVENT_MASK_BUTTON_PRESS
    | XCB_EVENT_MASK_BUTTON_RELEASE
    | XCB_EVENT_MASK_POINTER_MOTION,
    XCB_GRAB_MODE_ASYNC,
    XCB_GRAB_MODE_ASYNC,
    root, cursor, XCB_CURRENT_TIME);

So event_mask is a, well, event mask. :-P

(This todo also exists for other requests)

> +      <field name="4"><![CDATA[
> +Scroll wheel. TODO: direction?
> +      ]]></field>
> +      <field name="5"><![CDATA[
> +Scroll wheel. TODO: direction?
> +      ]]></field>

/usr/bin/xev says that button 4 is scrolling up and button 5 is scrolling down
(down = towards me).

Why are there so many occurances of "`XCB_NONE` (TODO)"?

> +      <field name="focus"><![CDATA[
> +The window to focus. All keyboard events will be reported to this window. The
> +window must be viewable (TODO), or a `xcb_match_error_t` occurs (TODO).

If I understand that first TODO correctly, figuring out what "viewable" means
should be left as an exercise for the reader. I don't understand the second TODO.

> +    <doc>
> +      <brief>query font metrics</brief>
> +      <description><![CDATA[
> +Queries information associated with the font.
> +      ]]></description>
> +      <field name="font"><![CDATA[
> +The fontable (Font or Graphics Context) to query.
> +      ]]></field>
> +      <!-- TODO: example -->
> +    </doc>

Core fonts are evil. Don't encourage people to use them by providing examples.

> +      <brief>Creates a pixmap</brief>
> +      <description><![CDATA[
> +Creates a pixmap. The pixmap can only be used on the same screen as `drawable`
> +is on and only with drawables of the same `depth`.
> +      ]]></description>
> +      <field name="depth"><![CDATA[
> +TODO
> +      ]]></field>

"Bit depth to use for the pixmap." Perhaps even say something about "Match error
occur if you e.g. CopyArea where the drawables don't have the same bit depth",
but I'm not sure if something like that should be said here.

[Lots of TODOs for GC-related stuff skipped]

> +The special value of `XCB_KILL_ALL_TEMPORARY`, the resources of all clients
> +that have terminated in `RetainTemporary` (TODO) are destroyed.

That phrase no verb. "The special value of foo means that...."?

No idea how much of this should be fixed before the patches are merged. These
docs are definitely better than no docs. But something should happen, so if you
want to ignore my comments from above, say so and the patch can get merged
instead. But most of these TODOs should be resolved before this ends up in a
released xcb-proto.

Uli
-- 
"Do you know that books smell like nutmeg or some spice from a foreign land?"
                                                  -- Faber in Fahrenheit 451