<AWT Dev> Review request #2: 6863566 (Java should support the freedesktop.org startup notification specification)

[Damjan Jovanovic]

On Tue, Oct 27, 2009 at 2:15 PM, Artem Ananiev <Artem.Ananiev at sun.com> 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
>