GNOME Bugzilla – Bug 530843
The list of X atoms should appear in exactly one place
Last modified: 2008-05-02 18:49:53 UTC
Metacity has a list of X atoms which it uses to communicate with the X server. This list is kept in *four* places (one list a proper subset of the identical other three): - as an array of char*s of names of atom names in display.c - as a sequence of Atoms as members of MetaDisplay in display.c which are assigned the interned values of each of the previous list (keeping the identifier mapped to the correct array index is the programmer's responsibility) - as a set of Atom fields of MetaDisplay in display.h - as a set of Atoms which we advertise that we support using _NET_SUPPORTED, in screen.c. This is a proper subset of the other two; it contains only _NET_WM* atoms and almost all _NET_WM* atoms are members of it. Scattered around are stern notes warning the coder to be sure to keep all four copies up to date. I have been itching to change this for a while so that we only keep one copy of the list. Today I wrote a patch to do so.
Created attachment 110204 [details] [review] All information should live in exactly one place
The patch would be considerably smaller if I hadn't regularised the names of atom fields in MetaDisplay (e.g. from atom_net_wm_ping to atom__NET_WM_PING; it would have been possible to keep their old names to some extent, but it would have required some ugly rewriting during interning).
Line 1887 in screen.c: there are some tabs that should be replaced with spaces for consistency with the rest of the code. Aside from that, the patch seems logical. The method of including is cute too :-) Is it really necessary for there to be 4 places using nearly-identical data? Is there any way to get it so that you just create one array/struct/other and just grab the object as-needed? This patch brings it closer to just one data, but it really only simplifies the task of maintaining 4 lists of all those Atoms. Could you possibly eliminate the necessity of keeping 4 lists/sets and somehow share a single one? I think this patch is probably a good change at least in the sense that it makes code maintenance easier. Did you update all those comments warning about updating the four copies? I only saw a few comment changes in there.
a) oh, thanks, didn't spot that. Must get my editor to highlight such things. c) There are really three ways to deal with the problem of interning X atoms I can think of: i) Interning them on the spot when needed and dropping them immediately after. I don't think this would be exactly the pinnacle of efficiency. ii) Interning them, then keeping them in a hash table mapping strings to atoms, and looking them up by name as needed. This would be the common-sense way in Perl or Python (where resolving the symbol would be a hash lookup anyway), but in C it's so much more extra faff than... iii) Interning them all at the start and then assigning them to variables of type Atom named after the atom. This is in fact how every WM written in C I've ever read works. We'd be able to reduce the number of places we use the list if we went with i) or ii) in C, and in a dynamic language we could do iii) with just one copy of the list, but in our case I think all four places we use the list are pretty much necessary. I just don't like having four separate lists, you know? d) I think I caught them all, but it's possible I missed them.
(Sorry, I didn't make something explicit there: if we intern all atoms at the start and assign them to lists, then we need: * actually to declare the variables * a list of the atom names, as strings * to assign each atom name to a variable after internment Hence, three lists. The fourth comes from a requirement to advertise which atoms (that represent EWMH hints) we understand.
s/assign them to lists/assign them to variables/ Too early in the morning.
Apparently this pattern is called X-Macros (for some reason). http://www.ddj.com/cpp/184401387 http://en.wikipedia.org/wiki/C_preprocessor#X-Macros http://op.closedformodification.com/2006/06/18/x-macros/ I don't think I'd use it a lot if it interfered with readability, but in this case I think it actually increases it.
Committed with one small fix spotted by Elijah (a static function had part of its name capitalised and shouldn't have). http://svn.gnome.org/viewvc/metacity?rev=3702&view=rev