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

[Anthony Petrov]

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

[Artem Ananiev-2]

Hi, Anthony, Damjan,

here are a couple of comments from my side:

1. _NET_STARTUP_INFO atom looks redundant. Check the example from
http://www.freedesktop.org/wiki/Specifications/startup-notification-spec 
for details.

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?

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.

Thanks,

Artem

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

[Damjan Jovanovic-2]

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

[Dmitry Cherepanov]

Just a small suggestion.

It might make sense to clarify whether this fix covers the UI tools
(like jconsole) or not. So far, the report says that

-----------------------------------------------------------------------------------------
Copy this text into a .desktop file in your ~/Desktop directory:

[Desktop Entry]
Name=Startup notification test
Type=Application
Exec=jconsole
StartupNotify=true

jconsole can be replaced by any Java application that opens a window.
-----------------------------------------------------------------------------------------

According this description, the fix is supposed to work OK with the UI
tools.

At the same time, the java process doesn't inherit the env variable
(DESKTOP_STARTUP_ID) of the UI process. As a result, the newly
introduced method will not remove the startup notification. So, my
suggestion is to explicitly clarify this behaviour in the bug report.

The fix itself looks fine for me.

Thanks,
Dmitry

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
>

[Artem Ananiev-2]

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

[Damjan Jovanovic-2]

On Wed, Nov 11, 2009 at 11:31 AM, Dmitry Cherepanov
<Dmitry.Cherepanov@...> wrote:

> Just a small suggestion.
>
> It might make sense to clarify whether this fix covers the UI tools (like
> jconsole) or not. So far, the report says that
>
> -----------------------------------------------------------------------------------------
> Copy this text into a .desktop file in your ~/Desktop directory:
>
> [Desktop Entry]
> Name=Startup notification test
> Type=Application
> Exec=jconsole
> StartupNotify=true
>
> jconsole can be replaced by any Java application that opens a window.
> -----------------------------------------------------------------------------------------
>
> According this description, the fix is supposed to work OK with the UI
> tools.
>
> At the same time, the java process doesn't inherit the env variable
> (DESKTOP_STARTUP_ID) of the UI process. As a result, the newly introduced
> method will not remove the startup notification. So, my suggestion is to
> explicitly clarify this behaviour in the bug report.
>
> The fix itself looks fine for me.

Startup notifications have to start and stop. They don't automatically
start when you double-click on an executable file from your file
manager or run an executable from a terminal. They can only start when
you run a .desktop file that has the "StartupNotify=true" line.

If startup notifications don't start, then there is no real problem -
it just looks bad because you know you started the application but you
aren't getting any feedback that it's starting (busy mouse cursor,
"Starting ..." on the task bar).

If startup notifications do start, then it looks really bad if they
don't stop - the application starts in a separate task bar tab, while
the "Starting..." tab and the busy mouse cursor continue for quite
some time. My patch addresses this case, by stopping them when the
first window shows.

The .desktop files for tools like jconsole currently have no
"StartupNotify=..." line, so startup notifications are disabled even
with my patch. I asked we add "StartupNotify=true" to those files and
provided a patch, but my request was denied, see the OpenJDK bug
(https://bugs.openjdk.java.net/show_bug.cgi?id=100094) for details.

> Thanks,
> Dmitry
>
> 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
>>
>
>

Thank you
Damjan Jovanovic

[Damjan Jovanovic-2]

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

[Anthony Petrov]

On 11/17/2009 06:11 PM, Damjan Jovanovic wrote:

> On Wed, Nov 11, 2009 at 11:31 AM, Dmitry Cherepanov
> <Dmitry.Cherepanov@...> wrote:
>> Just a small suggestion.
>>
>> It might make sense to clarify whether this fix covers the UI tools (like
>> jconsole) or not. So far, the report says that
>>
>> -----------------------------------------------------------------------------------------
>> Copy this text into a .desktop file in your ~/Desktop directory:
>>
>> [Desktop Entry]
>> Name=Startup notification test
>> Type=Application
>> Exec=jconsole
>> StartupNotify=true
>>
>> jconsole can be replaced by any Java application that opens a window.
>> -----------------------------------------------------------------------------------------
>>
>> According this description, the fix is supposed to work OK with the UI
>> tools.
>>
>> At the same time, the java process doesn't inherit the env variable
>> (DESKTOP_STARTUP_ID) of the UI process. As a result, the newly introduced
>> method will not remove the startup notification. So, my suggestion is to
>> explicitly clarify this behaviour in the bug report.
>>
>> The fix itself looks fine for me.
>
> Startup notifications have to start and stop. They don't automatically
> start when you double-click on an executable file from your file
> manager or run an executable from a terminal. They can only start when
> you run a .desktop file that has the "StartupNotify=true" line.
>
> If startup notifications don't start, then there is no real problem -
> it just looks bad because you know you started the application but you
> aren't getting any feedback that it's starting (busy mouse cursor,
> "Starting ..." on the task bar).
>
> If startup notifications do start, then it looks really bad if they
> don't stop - the application starts in a separate task bar tab, while
> the "Starting..." tab and the busy mouse cursor continue for quite
> some time. My patch addresses this case, by stopping them when the
> first window shows.
>
> The .desktop files for tools like jconsole currently have no
> "StartupNotify=..." line, so startup notifications are disabled even
> with my patch. I asked we add "StartupNotify=true" to those files and
> provided a patch, but my request was denied, see the OpenJDK bug
> (https://bugs.openjdk.java.net/show_bug.cgi?id=100094) for details.

I think Dmitry means that even if you add the "StartupNotify=..." to the
.desktop file for jconsole, it won't end the startap notification with
your current fix. That is because the .desktop file actually starts the
jconsole launcher, which subsequently starts the java process of the
jconsole w/o inheriting the environment. Therefore, the java process of
the jconsole does not end the startup notification.

AFAIR, your then-proposed fix only added the "StartupNotify=..." to some
  java and java-ws.desktop files. But it didn't address the issues in
the native launcher for jconsole.

However, the description of the bug in the Bugzilla states that with
your fix the .desktop file of the jconsole may be modified, and the
jconsole will work correctly: the startup notification will be ended as
soon as jconsole starts. Dmitry is pointing out that this is not true.

--
best regards,
Anthony

[Damjan Jovanovic-2]

On Tue, Nov 17, 2009 at 5:26 PM, Anthony Petrov <Anthony.Petrov@...> wrote:

> On 11/17/2009 06:11 PM, Damjan Jovanovic wrote:
>>
>> On Wed, Nov 11, 2009 at 11:31 AM, Dmitry Cherepanov
>> <Dmitry.Cherepanov@...> wrote:
>>>
>>> Just a small suggestion.
>>>
>>> It might make sense to clarify whether this fix covers the UI tools (like
>>> jconsole) or not. So far, the report says that
>>>
>>>
>>> -----------------------------------------------------------------------------------------
>>> Copy this text into a .desktop file in your ~/Desktop directory:
>>>
>>> [Desktop Entry]
>>> Name=Startup notification test
>>> Type=Application
>>> Exec=jconsole
>>> StartupNotify=true
>>>
>>> jconsole can be replaced by any Java application that opens a window.
>>>
>>> -----------------------------------------------------------------------------------------
>>>
>>> According this description, the fix is supposed to work OK with the UI
>>> tools.
>>>
>>> At the same time, the java process doesn't inherit the env variable
>>> (DESKTOP_STARTUP_ID) of the UI process. As a result, the newly introduced
>>> method will not remove the startup notification. So, my suggestion is to
>>> explicitly clarify this behaviour in the bug report.
>>>
>>> The fix itself looks fine for me.
>>
>> Startup notifications have to start and stop. They don't automatically
>> start when you double-click on an executable file from your file
>> manager or run an executable from a terminal. They can only start when
>> you run a .desktop file that has the "StartupNotify=true" line.
>>
>> If startup notifications don't start, then there is no real problem -
>> it just looks bad because you know you started the application but you
>> aren't getting any feedback that it's starting (busy mouse cursor,
>> "Starting ..." on the task bar).
>>
>> If startup notifications do start, then it looks really bad if they
>> don't stop - the application starts in a separate task bar tab, while
>> the "Starting..." tab and the busy mouse cursor continue for quite
>> some time. My patch addresses this case, by stopping them when the
>> first window shows.
>>
>> The .desktop files for tools like jconsole currently have no
>> "StartupNotify=..." line, so startup notifications are disabled even
>> with my patch. I asked we add "StartupNotify=true" to those files and
>> provided a patch, but my request was denied, see the OpenJDK bug
>> (https://bugs.openjdk.java.net/show_bug.cgi?id=100094) for details.
>
> I think Dmitry means that even if you add the "StartupNotify=..." to the
> .desktop file for jconsole, it won't end the startap notification with your
> current fix. That is because the .desktop file actually starts the jconsole
> launcher, which subsequently starts the java process of the jconsole w/o
> inheriting the environment. Therefore, the java process of the jconsole does
> not end the startup notification.
>
> AFAIR, your then-proposed fix only added the "StartupNotify=..." to some
>  java and java-ws.desktop files. But it didn't address the issues in the
> native launcher for jconsole.
>
> However, the description of the bug in the Bugzilla states that with your
> fix the .desktop file of the jconsole may be modified, and the jconsole will
> work correctly: the startup notification will be ended as soon as jconsole
> starts. Dmitry is pointing out that this is not true.

I see.

How can I edit the OpenJDK bugzilla comment?

> --
> best regards,
> Anthony
>

Thank you
Damjan

[Anthony Petrov]

Hi Damjan,

On 11/17/2009 9:06 PM Damjan Jovanovic wrote:

>> I think Dmitry means that even if you add the "StartupNotify=..." to the
>> .desktop file for jconsole, it won't end the startap notification with your
>> current fix. That is because the .desktop file actually starts the jconsole
>> launcher, which subsequently starts the java process of the jconsole w/o
>> inheriting the environment. Therefore, the java process of the jconsole does
>> not end the startup notification.
>
> I see.
>
> How can I edit the OpenJDK bugzilla comment?

I guess that isn't possible. You could add a comment instead.

Regarding the fix itself, it looks like everyone likes it. So I'm going
to push it later this week. Thanks Damjan!

--
best regards,
Anthony

[Dalibor Topic-5]

Damjan Jovanovic wrote:
> How can I edit the OpenJDK bugzilla comment?

I believe the easiest way to do it is to just add another
comment to the issue. I don't think that Bugzilla supports
editing past comments:
http://weblogs.mozillazine.org/gerv/archives/006449.html

cheers,
dalibor topic
--
*******************************************************************
Dalibor Topic                   Tel: [phone]
Java F/OSS Ambassador           AIM: robiladonaim
Sun Microsystems GmbH           Mobile: [phone]
Nagelsweg 55                    http://openjdk.java.net
D-20097 Hamburg                 mailto:Dalibor.Topic@...
Sitz der Gesellschaft: Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Amtsgericht München: HRB 161028
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring