[Xcb] bug in icccm.c ?

[Vincent Torri]

On Tue, 5 Sep 2006, Jamey Sharp wrote:

> On Tue, Sep 05, 2006 at 10:01:05AM +0200, Vincent Torri wrote:
>> (of course, that remarks also applies with props.c, and maybe other
>> functions in xcb-utils)
>
> First disclaimer: the original libraries in xcb-util are still lacking a
> coherent design, and may be full of bugs. icccm is one of those
> libraries; property is another. They shouldn't be used in real projects
> without some discussion on the list here about their many flaws. :-) I
> don't know of anything but my window manager demo that uses them, and it
> doesn't adequately test them.

Right, they need a complete review. The problem is that I have to use them 
in ecore. It's the fourth time I rewrite ecore_xcb (used later for 
enlightenment). I want it to be the last (or at least with small 
modifications later, and not in the design). Ecore will test them all :) 
It will be a good test.

I also think that making unit test is recessary for icccm.

>> I've noticed that all the SetFlag* functions set the flag, but do no add
>> it :
>>
>> hints->flags = ***
>>
>> and not
>>
>> hints->flags |= ***
>>
>> Is it normal ?
>
> I have no idea what those were for, though it looks like I'm the one who
> wrote them. Looking at them now, they seem useless to me. The functions
> that follow -- SizeHintsSetPosition, etc. -- do things that I think are
> sensible though.
>
>> Also, I would like to add functions similar to those in icccm.c, but which
>> take a reply, so that we can avoid round trips. Currently, we just have
>> mimic the Xlib functions, hence, we have suppressed all the benefit of
>> xcb. What do you think ?
>
> According to git history, you wrote GetWMSizeHints, so I guess you're
> welcome to change how it works. :-)

hehe

> By the way, on inspection, GetWMSizeHints has at least two correctness
> bugs and one memory leak that mean it couldn't have worked anyway. At
> first I thought I wrote it, bugs and all -- it mostly looks like my
> style, and does a number of good things, like handling pre-ICCCM
> clients. Anyway, since you want to redesign it, a rewrite might be in
> order rather than fixing the bugs.
>
> One approach is to mimic xcb_atom's InternAtomFast function, which
> returns a tagged union containing either the actual atom or an
> XCBInternAtom cookie. When you need the value later, the
> InternAtomFastReply function updates the tagged union in-place with the
> actual value, if it wasn't already there. All of the SizeHintsGet*
> functions could work like InternAtomFastReply.
>
> Another approach is to provide simple wrappers for XCBGetProperty and
> XCBGetPropertyReply that just provide the right parameters and unpack
> the results correctly. That may be better in this case.

I prefer the second one. Maybe because the first one can later use the 
second one :)

I would write 3 functions : wrappers for XCBGetProperty and 
XCBGetPropertyReply, and another one that uses the reply only, as it can 
be used for other functions.

I also think that I have to wrap the ***Uncheck function from plan 7.

Also, we have already mention that before in the mailing list. If I 
rewrite the functions, which naming convention should I use ? I 
really don't like the Xlib one.

Vincent