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

[Damjan Jovanovic]

On Wed, Nov 11, 2009 at 5:05 PM, Artem Ananiev <Artem.Ananiev at sun.com> wrote:
> 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.

The widely used GUI framework gtk+ has been sending startup
notification messages this way for many years, so it must work - for
example follow the function gdk_x11_display_broadcast_startup_message
in http://git.gnome.org/cgit/gtk+/tree/gdk/x11/gdkdisplay-x11.c

> No more comments from my side :)
>
> Thanks,
>
> Artem

Thank you
Damjan Jovanovic

> 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
>