----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125309/#review86036 -----------------------------------------------------------
hard cost. I now understand why I didn't get what you meant in the other review request ;-) Yeah from code side it looks nicer, though the enum is really hard to get IMHO. src/platforms/xcb/atoms_p.h (line 1) <https://git.reviewboard.kde.org/r/125309/#comment59357> even if questionable whether there is any copyright on the atom listing: I suggest to add the copyright header. src/platforms/xcb/atoms_p.h (lines 4 - 20) <https://git.reviewboard.kde.org/r/125309/#comment59358> uff, I just needed a quarter of an hour to understand how that works. Could you please add a comment section, to explain what it does. src/platforms/xcb/netwm.cpp (lines 299 - 300) <https://git.reviewboard.kde.org/r/125309/#comment59359> shouldn't we undef the ENUM_CREATE_CHAR_ARRAY again? src/platforms/xcb/netwm_p.h (line 39) <https://git.reviewboard.kde.org/r/125309/#comment59360> I don't like the type name "Atom" as that's also an XLib type. As the actual name is hardly used we could get it to something more precise, maybe NetAtom? - Martin Gräßlin On Sept. 19, 2015, 3:22 p.m., Thomas Lübking wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125309/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2015, 3:22 p.m.) > > > Review request for KDE Frameworks and Martin Gräßlin. > > > Repository: kwindowsystem > > > Description > ------- > > alteration review #125259 minus the autotests > > Significant difference is the creation and handling of the Atom enum. > > 1. Net::Utf8 makes no sense > 2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE > (there's nothing such as XA_WM_STATE) > 3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, > KDE_NET_, or ... UTF8, I'm not sure why one would add more enums (since this > is internal API and the resolved type will always be xcb_atom_t - in worst > case we run into errors on comparing the enums > 4. I wanted to get rid of the explicit counter variable as well as any loose > assignment between atom variable/enum and string. > > ----- > > So far on first creation of a NETRootInfo or NETWinInfo a static > initialization of atoms was performed. This meant that the NET classes > are only able to interact with the X server the first NET class is > connected to. > > Normally applications don't need to interact with multiple X servers. > An exception is kwin_wayland which needs a connection to its Xwayland > server and on the x11 backend to the X server it renders to. So far > KWin could not use the NET classes for it, causing the rendering window > to e.g. not have a window title. > > This change removes the implicit constraint on one X server by > creating a hash of connection and atoms. For each created NET class > we check whether we have already resolved the atoms, if yes we reuse > otherwise we create them. > > For the normal use case of one X11 connection the change should be > rather minimal. Instead of performing the check whether the static > atoms have been created, it now is a check whether the atoms for the > connection have been created. The atoms are kept in a > QSharedDataPointer ensuring that we don't needless copy the atoms into > each class. > > CHANGELOG: Allow interacting with multiple X servers in the NETWM classes. > > > Diffs > ----- > > src/platforms/xcb/netwm_p.h 917a86e > src/platforms/xcb/atoms_p.h PRE-CREATION > src/platforms/xcb/netwm.cpp d99a925 > > Diff: https://git.reviewboard.kde.org/r/125309/diff/ > > > Testing > ------- > > > Thanks, > > Thomas Lübking > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
