Fwd: [kdelibs/frameworks] tier1/kidletime/src: Port the native event filtering to XCB, to fix compilation with Qt5.

[Fredrik Höglund]

On Thursday 26 July 2012, David Faure wrote:
> On Wednesday 25 July 2012 17:10:57 Fredrik Höglund wrote:
> > They use the freedesktop bugzilla, and there's also an xcb mailinglist
> > there.
> 
> Actually the bug has been reported to the freedesktop bugzilla 3 years ago...
> https://bugs.freedesktop.org/show_bug.cgi?id=23403
> I added a comment.
> 
> The good thing is that it includes a workaround, so I'll use that for now.
> 
> Hmm, but that only fixes one issue, not the other one:
> 
>     // Workaround for https://bugs.freedesktop.org/show_bug.cgi?id=23403
> #define xcb_sync_systemcounter_name(sc) (((char *) &(sc)->name_len) + 2)
> 
>     xcb_sync_list_system_counters_cookie_t cookie = xcb_sync_list_system_counters(m_xcb_connection);
>     xcb_sync_list_system_counters_reply_t* reply = xcb_sync_list_system_counters_reply(m_xcb_connection, cookie, NULL);
>     int xcbcounters = xcb_sync_list_system_counters_counters_length(reply);
>     xcb_sync_systemcounter_iterator_t it = xcb_sync_list_system_counters_counters_iterator(reply);
>     for (int i = 0; i < xcbcounters; ++i) {
>         qDebug() << it.data->counter << it.rem << it.index;
>         qDebug() << "name length" << xcb_sync_systemcounter_name_length(it.data);
>         QByteArray name(xcb_sync_systemcounter_name(it.data), xcb_sync_systemcounter_name_length(it.data));
>         qDebug() << name;
>         xcb_sync_systemcounter_next(&it);
>     }
>     delete reply;
> 
> outputs
> 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: 345 2 32 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: name length 10 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: "SERVERTIME" 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: 0 1 26 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: name length 17481 
> kidletime_example(12226)/default XSyncBasedPoller::XSyncBasedPoller: "LETIME_TONS_OF_GARBAGE_HERE"
> (This should have been IDLETIME instead, according to the old X11 based code)
> 
> The name length for the second entry is bogus (it says 17481 bytes!).
> Am I using the iterator wrongly?
> 
> I tried the code from the attachment of the above bug report, but
>     for (iter = xcb_sync_list_system_counters_counters_iterator(reply) ;
>          iter.rem ; xcb_sync_systemcounter_next(&iter)) {
>       ...
>    }
> only goes once into the loop, i.e. it prints "SERVERTIME" but not "IDLETIME".

I believe that's how the iterators are meant to be used, but I don't see how
it can only loop once when it.rem starts at 2.  I also can't reproduce that
behavior when I try it here.

At any rate I think xcb_sync_systemcounter_next() suffers from the same
problem as xcb_sync_systemcounter_name(), since it also relies on
sizeof(xcb_sync_systemcounter_t).

If I modify the code to advance the pointer manually instead of using
the iterator it works correctly:

xcb_sync_systemcounter_t *counter = xcb_sync_list_system_counters_counters_iterator(reply).data;
for (int i = 0; i < xcbcounters; ++i) {
    const QByteArray name(((const char *) counter) + 14, counter->name_len);
    qDebug() << counter->counter << name;
    counter = (xcb_sync_systemcounter_t *) (((char *) counter) + ((14 + counter->name_len + 3) & ~3));
}

Of course at this point the code has become quite ugly.

> [If only this xcb stuff was documented.... but generating code including empty
> doxygen comments gives a pretty crappy result in the end, documentation-wise]

The XML format the code is generated from has documentation tags,
so if they're present the generated doxygen comments won't be empty.
That doesn't help when the documentation tags aren't there of course.

> > > Also, the alarms don't trigger, so I wonder if this is another bug in xcb,
> > > or if my port (which mixes the use of XCB and X11) is faulty...
> > 
> > The only thing I can think of is that it may be necessary to call XFlush()
> > after making Xlib calls.  The output buffer is automatically flushed when
> > XNextEvent() is called, but since Qt is using xcb to handle events it
> > never calls that function.  So it is quite possible that the requests to
> > set the alarms are just sitting in the queue.
> 
> Wow, you're good! (Not that I had a doubt).
> This fixed the issue. Thanks!

Or it was a lucky guess ;)
 
> This leads me to wonder: is there any reason for actually porting the remaining
> X11 code to XCB? I ported the event handling because Qt5 forces us to use XCB events, but
> I could just keep XSyncListSystemCounters and XSyncCreateAlarm, XSyncChangeAlarm,
> and be done with this? Or is that suboptimal?

It's not really necessary as long as Qt continues to use Xlib to open the
connection to the X server.  And it probably will for the forseeable future
since right now you can't use OpenGL without doing that.

It probably makes the most sense to prioritize porting the event handling
code, and make sure that we don't use Xlib types in public header files.
That way we won't have to break the API if Qt stops using Xlib entirely at
some point in the future.

The disadvantages I see are additional dependencies on X extension
libraries, and the need to flush the output buffer manually.  Flushing
the output buffer too frequently will also hurt performance.

Fredrik