Hi Pankaj, Sorry for replying late to this!
I think the patch is good, I'm still unsure if we shouldn't try to support the GTK laf on anything non KDE instead, but I believe this patch preserves compatibility with the current state of things. Cheers, Mario On Thu, Aug 6, 2020 at 12:03 PM Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com> wrote: > > +1 > > Regards > Prasanta > On 06-Aug-20 1:23 AM, Philip Race wrote: > > Approved. > > -phil. > > On 8/5/20, 11:22 AM, Pankaj Bansal wrote: > > Hello Phil, > > I have made the changes you suggested. > > webrev: http://cr.openjdk.java.net/~pbansal/8247753/webrev02/ > > <<I am assuming that you have a reason for both toLowerCase() and contains() > rather than equals() ? > > Yes, in some distros XDG_CURRENT_DESKTOP is set to some string containing > "gnome" string, but not exactly "gnome". For example, on Ubuntu 18.04, it is > "ubuntu:GNOME". I could have used "endsWith" method as of now, but for more > inclusiveness, I have used contains, so we don't have to change again if some > distro decides to set XDG_CURRENT_DESKTOP to something not covered by > "endsWith". > > Regards, > > Pankaj > > > > On 05/08/20 9:35 PM, Philip Race wrote: > > I've read the bug comments and it looks like you've eventually come to the > right answer. > I just would code it differently for a couple of reasons > 1) To make it easier some day to remove checking the old variable by just > deleting a few lines > 2) To avoid repeating the string literal which I always think of as > error-prone > > I am assuming that you have a reason for both toLowerCase() and contains() > rather than equals() ? > > diff --git a/src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java > b/src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java > --- a/src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java > +++ b/src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java > @@ -95,10 +95,19 @@ > > @Override > public String getDesktop() { > + String gnome = "gnome"; > String gsi = AccessController.doPrivileged( > (PrivilegedAction<String>) () > -> > System.getenv("GNOME_DESKTOP_SESSION_ID")); > - return (gsi != null) ? "gnome" : null; > + if (gsi != null) { > + return gnome; > + } > + String desktop = AccessController.doPrivileged( > + (PrivilegedAction<String>) () > + -> System.getenv("XDG_CURRENT_DESKTOP")); > + return (desktop != null && desktop.toLowerCase().contains(gnome)) > + ? gnome : null; > + > } > > -phil. > > On 8/4/20, 10:16 PM, Prasanta Sadhukhan wrote: > > Looks ok to me. > > One observation is, in testcase you can remove line71 linux check as we > already did the check in l54. > > Regards > Prasanta > On 04-Aug-20 11:12 PM, Pankaj Bansal wrote: > > Hi All, > > Please review the following fix for jdk16. > > Bug : https://bugs.openjdk.java.net/browse/JDK-8247753 > webrev: http://cr.openjdk.java.net/~pbansal/8247753/webrev00 > > Bug: UIManager.getSytemLookAndFeelClassName() returns wrong value on Fedora > 32. It is returning the MetalLookAndFeel classname instead of GTKLookAndFeel > classname. > > Cause: Java uses a environment variable GNOME_DESKTOP_SESSION_ID to verify if > the system is gnome (which is set by gnome) and then checks for > GTKLookAndFeel support. If both conditions are satisfied, then GTKLookAndFeel > classname is returned, else cross platform MetalLookAndFeel is selected. > > The GNOME_DESKTOP_SESSION_ID environment has been deprecated for a long time > and the value is set to "this-is-deprecated" by gnome. In java, the actual > value of variable is not being verified. As long as the value is not null (it > has been set to something by gnome), things have been working fine though the > value set by gnome is "this-is-deprecated". Now, gnome has removed the > variable completely and this is causing issues in Fedora 32. As the variable > is not set at all, java is returning the cross platform MetalLookAndFeel > classname for the SystemLookAndFeelClassName instead of GTKLookAndFeel. > > More information on this in JBS. Right now the issue is seen only on Fedora > 32, but this can very well come in future releases of RHEL, CentOS, Oracle > Linux etc. > > Fix: We should check XDG_CURRENT_DESKTOP also along with > GNOME_DESKTOP_SESSION_ID to verify gnome based linux. XDG_CURRENT_DESKTOP is > set by gnome based platforms to some string containing "gnome" substring. So, > this can be used to verify the gnome based desktop. For backward > compatibility, we need to continue checking GNOME_DESKTOP_SESSION_ID as well > because for some linux platforms, XDG_CURRENT_DESKTOP may be set to something > else as they are not gnome based, but have been returning GTKL&F as > GNOME_DESKTOP_SESSION_ID was set by them. so, we dont want to change anything > for them. > > Other solution could have been to just make GTKL&F default on all linux, but > that would result in GTKL&F being selected on some new platfoms like KDE > based platforms where currently Metal L&F is selected. > > I have modified the test case > test/jdk/javax/swing/LookAndFeel/SystemLookAndFeel/SystemLookAndFeelTest.java. > The test fails on Fedora 32 without fix and passes with the fix. I have run > the required mach5 tests and this fix is not breaking anything. Link in JBS. > > > Regards, > > Pankaj Bansal > > -- Mario Torre Associate Manager, Software Engineering Red Hat GmbH <https://www.redhat.com> 9704 A60C B4BE A8B8 0F30 9205 5D7E 4952 3F65 7898
