[Xcb] documentation patches

[Uli Schlachter]

On 11.01.2012 19:27, Michael Stapelberg wrote:
> Excerpts from Uli Schlachter's message of 2012-01-11 16:07:44 +0000:
>>> +    if self.doc:
>>> +        if self.doc.brief:
>>> +            _h(' * @brief ' + self.doc.brief)
>>> +        else:
>>> +            _h(' * No brief doc yet')
>>> +
>>> +    _h(' *')
>> Should the "No brief doc yet"-case generate a warning on stderr? Or should this
>> write a log to some file, similar to what doxygen does? Or could one add an
>> error on purpose in that case, so that this shows up in doxygen's log?
> Hm, I can’t find a keyword which will make doxygen do that. But, who actually
> reads the doxygen log in reality? It’s HUGE for xcb. Just grepping for 'No
> brief doc yet' is an easier way, or (and that’s the way I thought it will
> happen) looking at the docs, noticing there are no docs for the particular
> thing you’re interested in, figuring it out, sending a patch.
>
>> Do you plan to fix the remaining "XXX"s in there? If not, what stops them from
>> staying in there forever after this was merged? (I don't really mind, but I do
>> think that "staying in there forever" isn't optimal)
> I agree it’s not optimal. I might fix them sometime in the future, but don’t
> take my word for it. They are in so that other people who read the source are
> aware of it.
>
>> WTF is up with src/list_of_manpages.inc? Could that get some line breaks? Also,
> That is due to autotools stupidity, see
> http://thread.gmane.org/gmane.comp.freedesktop.xcb/7152
> 
>> does this really have to be maintained by hand? Ssince this depends on the
>> version of xproto used, how should this work at all? This essentially means that
>> every addition to xproto will cause a commit to libxcb to be needed.
> Yes, and I agree that it sucks. Solutions welcome :).

For all of the above points: Yeah... :-(

>> src/static-man/xcb-examples.3:
>> FLUSHING:
>> This should *explain* flushing before giving optimizing suggestions. :-/
> I added this paragraph:
> 
>     Flushing means calling \fIxcb_flush\fP to clear the XCB-internal write
>     buffer and send all pending requests to the X11 server. You don't
>     explicitly need to flush before using a reply function (like
>     \fIxcb_query_pointer_reply\fP), but you do need to flush before entering
>     the event loop of your program.
> 
> Does that make it clear enough?

Almost. I still miss that xcb_wait_for_event() doesn't flush (while it's in
there implicitely, I think I'd miss that point when I read this for the first
time). (No idea how bold font works in a man page, so *this* will have to do)

    There are only two cases when XCB flushes by itself. The first Case is when
    its write buffer becomes full, the second case is when you are asking for
    the reply of a request which wasn't flushed out yet (like
    \fIxcb_query_pointer_reply\fP). This last point also includes
    xcb_request_check(). Please note that waiting for an event does *NOT* flush.

>> COMPILATION:
>> I'd drop that whole paragraph. Does this really fit into a manpage?
> I don’t know. The manpage contains everything there is to know about the code
> examples, so I figured people would read that and be encouraged to help us in
> that respect. Does it hurt to have it there?
> 
>> CODING STYLE:
>> Just drop this.
> See above. It’s intended for contributors who want to submit manpages in a
> consistent way. I have no strong opinion on this one, but having the coding
> style documented somewhere is a good thing.
> 
>> All in all, this man page doesn't feel all that manual-y to me.
> I think it’s helpful for understanding the examples.

Well, ok...

>> xcb-requests.3:
>>
>> "datastructure" should be "data structure"
> Changed, thanks.
> 
>> CHECKED VS. UNCHECKED:
>>
>> This explains that unchecked errors go to the event loop, but it doesn't really
>> say that checked errors are returned by xcb_request_check(). This should be
>> mentioned more boldly IMHO.
> Hm, it does:
> 
>     If you want to explicitly check for errors in a blocking fashion, call the
>     _checked version of the function (for example \fIxcb_map_window_checked\fP) and
>     use \fIxcb_request_check\fP.
> 
> If that is not boldly enough, please suggest better wording :).

Ok, you win. I guess I got confused because this talks about the _checked
version of the function and thus kind-of-but-not-really excludes requests which
generate a reply.

>> [The example after this code uses "event_type == 0 => is an error", perhaps this
>> should be mentioned more prominently, too?)
> Uhm, it is:
> 
>     For requests which have no reply (for example \fIxcb_map_window\fP), errors
>     will be delivered to the event loop (you will receive an X11 event of type 0
>     when calling \fIxcb_poll_for_event\fP).
> 
> It seems like you skipped over that paragraph? :)

Ok, how about this:

   .SH CHECKED VS. UNCHECKED

    The checked and [just the sentence which was already there]

    A request is UNCHECKED if errors that it causes are handled by the normal
    main loop. This means that that you will receive an X11 event of type 0 when
    calling \fIxcb_wait_for_event\fP.

    If a request is CHECKED, you will have to explicitly check for errors
    from this single request. Not doing so will result in a memory leak. See
    below for details on how to check for errors.

The point is that this now explains the difference independently from the whole
"has a reply" / "has no reply" part.

> BTW: Updated code is in the github repository. I will re-send the patch after
> the rest was reviewed (unless someone wants me to re-send now).

I'll just assume that nothing got worse. I skipped most of the python parts anyway.
BTW: Thanks for going down the documentation road. :-)

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