On Tue, Oct 27, 2009 at 2:15 PM, Artem Ananiev <[email protected]> wrote: > Hi, Anthony, Damjan,
Hello Artem, Anthony > here are a couple of comments from my side: You'll find they're followed by my counterarguments :-) > 1. _NET_STARTUP_INFO atom looks redundant. Check the example from > http://www.freedesktop.org/wiki/Specifications/startup-notification-spec for > details. That example has at least 2 problems, the wrong atom name being one of them. I've complained to freedesktop.org some time ago (http://lists.freedesktop.org/archives/xdg/2008-December/010106.html), but things are a bit nebulous that side :-). If it makes you feel any better, the equivalent patch I sent to the Wine project works the same way as this one (http://www.winehq.org/pipermail/wine-cvs/2009-January/051514.html). It's been working fine since it got committed on 7 January 2009. > 2. I see the client message is currently send to the root window, while the > same example from freedesktop.org sends it to some "target window". Is the > fix tested, say, for different virtual desktops? The spec says "In multihead setups, the messages should go to the root window of the X screen where the launchee application is being launched." I think that's what my patch does: + XlibWrapper.XSendEvent(XToolkit.getDisplay(), + XlibWrapper.RootWindow(XToolkit.getDisplay(), getScreenNumber()), Now as for different virtual desktops, my tests show that both patched Java and native applications have the startup notification follow you and continue, with the window opening on the new desktop, if you switch virtual desktops during startup. > 3. Here is a code from removeStartupNotification(): > > 1240 final int msglen = Math.min(message.length - pos, 20); > 1241 int i = 0; > 1242 for (; i < msglen; i++) { > 1243 XlibWrapper.unsafe.putByte(req.get_data() + i, message[pos + i]); > 1244 } > 1245 for (; i < 20; i++) { > 1246 XlibWrapper.unsafe.putByte(req.get_data() + i, (byte)0); > 1247 } > > We first set "msglen" bytes from message array and then 20 zero bytes. It > seems that 20 should be replaced with (20-message.length % 20) % 20, > otherwise we'll get out of XClientMessageEvent bounds. No, we don't reset the variable i to 0, it starts where it left off and run up to 20. There can't be an overrun. > Thanks, > > Artem Thank you Damjan > Anthony Petrov wrote: >> >> Hello, >> >> Please review the latest version of the fix contributed by Damjan >> Jovanovic: >> >> RFE: https://bugs.openjdk.java.net/show_bug.cgi?id=100094 >> There you can also find some latest comments regarding the fix. >> >> webrev: >> http://cr.openjdk.java.net/~anthony/7-24-startupNotify-6863566.2/ >> >> The patch no longer unsets the environment variable, and hence does not >> need core-libs review. >> >> -- >> best regards, >> Anthony >
