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

[Artem Ananiev]

Hi, Damjan,

I wonder if WM teams (kwin, metacity, xfwm, etc.) are aware of the 
errors in the specification/examples, otherwise they'll wait for 
incorrect atom names and use incorrect windows and will miss the 
information what Java applications provide to them.

No more comments from my side :)

Thanks,

Artem

Damjan Jovanovic wrote:
> 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