572295 - Add "X11Util" to be shared between X11 toolkits

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

One persistent thorn in my side has been getting the default X Display.  I've seen code for this for gtk/qt copied around about 5 times.

I also ran into a case a few days ago when I wanted an "auto X pointer", on which XFree would be invoked when it went out of scope.  That's enough to merit "X11Util" to me!

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add X11Util.h for X11 toolkits

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 451499 [details] [diff] [review]
Add X11Util.h for X11 toolkits

Looks sensible to me, thanks.

>+inline Display*
>+XDisplay()
>+{
>+  return DISPLAY();
>+}

When I added GetXDisplay(), I initially called it XDisplay() but roc pointed out that it looked like an Xlib function.
How about DefaultXDisplay()?

I would move the DISPLAY stuff to this function, to avoid polluting the preprocessor's namespace.

>+template<typename T>
>+struct NS_STACK_CLASS AutoXFree

NS_STACK_CLASS doesn't seem the right thing here.
I think we really want NS_IS_NOT_SAFE_TO_COPY, but I don't know whether we have that.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 451499 [details] [diff] [review]
Add X11Util.h for X11 toolkits


>diff --git a/gfx/public/X11Util.h b/gfx/public/X11Util.h

>+#if defined(MOZ_WIDGET_GTK2)
>+#  include <gdk/gdkx.h>
>+#  define DISPLAY GDK_DISPLAY
>+#elif defined(MOZ_WIDGET_QT)
>+#  include <QX11Info>
>+#  define DISPLAY QX11Info::display
>+#else
>+#  error Unknown toolkit
>+#endif 

I'm not in love with this #define of DISPLAY. I'd much rather separate implementations of XDisplay(), tbh.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add X11Util.h for X11 toolkits (updated per comments)

Oright, meant to post the updated patch but got sidetracked.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/28f111fe6ee9

Comment 6

[Oleg Romashin (:romaxa)]

Qt build busted
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1277169298.1277169408.27698.gz

I think there is some conflict with Qt defines... like CursorShape

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Build fix for Qt

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 452939 [details] [diff] [review]
Build fix for Qt

If this problem pops up more, we should create a wrapper for QX11Info.h that fixes up this conflict automatically.  It's going to be ugly copying and pasting this macro around everywhere QX11Info.h might be included before X.h.

Comment 9

[Oleg Romashin (:romaxa)]

Reported about Qt include problem:
http://bugreports.qt.nokia.com/browse/QTBUG-11627