<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Mon Mar 21 13:52:10 UTC 2016
On 21.03.16 16:26, Semyon Sadetsky wrote:
>
>
> On 3/19/2016 6:49 PM, Sergey Bylokhov wrote:
>> On 18.03.16 15:37, Semyon Sadetsky wrote:
>>> On 3/17/2016 2:46 PM, Sergey Bylokhov wrote:
>>>> Small notes:
>>>> - It will be good to move the code which reads the system properties
>>>> to the static init block, otherwise we can be in trouble if these
>>>> properties will be changed at runtime.
>>> I don't' see any troubles with this. Can you describe the scenario you
>>> meant?
>>> I think the properties changing at run-time is useful because it gives
>>> better control over GTK load. For example, developer may change the
>>> order to GTK3->GTK2.
>>
>> Yes the users can change the property at the beginning of the program
>> or via command line, but it is not good thing to allow to change it
>> after unix toolkit is loaded. For example isNativeGTKAvailable() can
>> check one version of the library, and loadGTK() can try to load
>> another version, because getEnabledGtkVersion() will return different
>> values. It belongs to all places where getEnabledGtkVersion() is used.
> This is the only suspicious scenario but it is unlikely possible.
> isNativeGTKAvailable() and initialize() are called in the same
> setLookAndFeel() method. I doubt that somebody will find it sensible to
> change the jdk.gtk.version concurrently with the setLookAndFeel() call.
> Even if this happens and GTK becames unavailable the initialize() will
> throw the same UnsupportedLookAndFeelException exception as in
> isNativeGTKAvailable() check, so the behavior will correspond to the
> specification.
But for example getEnabledGtkVersion() is called in the
XDesktopPeer.java also. Note that XDesktopPeer and UnixToolkit
synchronized differently but but tries to init gtk. It will be safer to
remove this possible missconfiguration.
>>
>>>> - Is it necessary to use ordinal? can we replace it with some
>>>> specific data?(the same for the indexed access in .values()[..])
>>> In general I'm good. But what specific data you propose?
>>
>> And javadoc for ordinal:
>> "Most programmers will have no use for this method. It is designed for
>> use by sophisticated enum-based data structures, such as EnumSet and
>> EnumMap."
>>
>> Quote from the internet:
>> "This is not recommended to use ordinal() (Effective Java, Item 31) as
>> it relies on the order of the enum values in client's code determine
>> the ordinal, and any change could lead to bad values being mapped.
>>
>> Instead, it would be better to have users implement a method that
>> would return a unique ID for an enum value. For instance, with an
>> Identifiable interface that has a unique method id(), that the user
>> would have to implement for every enum value."
>>
> These are related to user's code.
> In JDK internal code there are plenty usages of ordinal(). In this
> specific case it is only necessary for transferring the value to the
> internal native code. But the API never returns ordinal values to user.
Transferring such unstable data to the native is more dangerous than to
the user.
>
> --Semyon
>>>
>>> --Semyon
>>>>
>>>> On 16.03.16 20:52, Semyon Sadetsky wrote:
>>>>> Hi Phil,
>>>>>
>>>>> Thank for review. You will find my reply below in the text.
>>>>>
>>>>> The updated webrev is
>>>>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.01/
>>>>> It also contains:
>>>>> - new properties jdk.gtk.version and jdk.gtk.verbose
>>>>> - appearance tuning for Ubuntu 15 (GTK 3.14). It may require more
>>>>> but we
>>>>> can do this later as a separate bug.
>>>>> The main implementation was done for Ubuntu 14.05 LTS (GTK 3.10) and
>>>>> then tuned for OEL 7 (GTK 3.8). Each minor GTK version may have some
>>>>> appearance changes.
>>>>>
>>>>> On 3/15/2016 10:39 PM, Phil Race wrote:
>>>>>> There is a lot to read here. I think I need to patch and try it but
>>>>>> first ...
>>>>>>
>>>>>> Two high level questions :
>>>>>> 1) Have you verified that this behaves properly (or no worse than
>>>>>> currently) with -Djava.awt.headless=true since Swing components
>>>>>> are supposed to be able to draw off-screen in headless mode .. and
>>>>>> yet a dependency on GTK and its dependency on xlibraries seems to
>>>>>> mean
>>>>>> that you can't load GTK in this case.
>>>>>> BTW I think it may be painful to get them to layout in such a case
>>>>>> but that's another issue.
>>>>> I tested it by painting to a BufferedImage. Seems it is enough.
>>>>>>
>>>>>> 2) Have you tried a hi-dpi system ?
>>>>> Yes I have. It is identical to the existing GTK2 based appearance.
>>>>>>
>>>>>> 3) Have you submitted a JPRT job ? It is essential to know that this
>>>>>> builds cleanly on the "official" compilation environment.
>>>>> I will do this before the push. I think it will be OK because I did
>>>>> not
>>>>> use any new C constructs and the new libraries are linked dynamically.
>>>>>> 4)I expect you ran Swingset2 + GTK L&F but have you run any of the
>>>>>> regression test suite ?
>>>>> Yes I ran javax/swing tests but many of them fails with GTK2 as well.
>>>>> With GTK3 the result was the same except for some unstable tests.
>>>>> Unity
>>>>> desktop has new window decorations like borderless windows which are
>>>>> resized by dragging the outer window shadow, invisible overlay
>>>>> scrollbars, etc. Many tests written for old window decorations fails.
>>>>>>
>>>>>> Minor comments :
>>>>>>
>>>>>> GTKEngine.java
>>>>>>
>>>>>> 494 Container parent = context.getComponent().getParent();
>>>>>> 495 if(GTKLookAndFeel.is3()) {
>>>>>> 496 if (parent != null && parent.getParent() instanceof
>>>>>> JComboBox) {
>>>>>> 497 if (parent.getParent().hasFocus()) {
>>>>>> 498 synthState |= SynthConstants.FOCUSED;
>>>>>> 499 }
>>>>>> 500 }
>>>>>> 501 }
>>>>>>
>>>>>> GTKPainter.java
>>>>>>
>>>>>> 746 if (GTKLookAndFeel.is3()) {
>>>>>> 747 if (slider.getOrientation() == JSlider.VERTICAL) {
>>>>>> 748 y += 1;
>>>>>> 749 h -= 2;
>>>>>> 750 } else {
>>>>>> 751 x += 1;
>>>>>> 752 w -= 2;
>>>>>> 753 }
>>>>>> 754 }
>>>>>>
>>>>>> I don't know where these numbers come from or what coordinate system
>>>>>> is being used here but it seems you are changing them for gtk 2.2 as
>>>>>> well as 3
>>>>>> Can you speak to this ?
>>>>> It is an appearance tuning for GTK3. I didn't change it for GTK2,
>>>>> why do
>>>>> you think so?
>>>>> This was used before my fix as well, for example
>>>>>
>>>>> if (containerParent instanceof JComboBox) {
>>>>> x += (focusSize + 2);
>>>>> y += (focusSize + 1);
>>>>> w -= (2 * focusSize + 1);
>>>>> h -= (2 * focusSize + 2);
>>>>> } else {
>>>>> x += focusSize;
>>>>> y += focusSize;
>>>>> w -= 2 * focusSize;
>>>>> h -= 2 * focusSize;
>>>>> }
>>>>>
>>>>> The only place where I changed the existing GTK2 appearance is:
>>>>>
>>>>> 1121 CLASS_SPECIFIC_MAP.put("Slider.thumbWidth",
>>>>> "slider-length");
>>>>>
>>>>> in GTKStyle.java, because this property was omitted by mistake.
>>>>>>
>>>>>> GTKStyle.java
>>>>>>
>>>>>> 735 if(!GTKLookAndFeel.is3()) {
>>>>>>
>>>>>> 840 } else if(GTKLookAndFeel.is3() &&
>>>>>> "ComboBox.forceOpaque".equals(key)) {
>>>>>>
>>>>>>
>>>>>> we prefer a space between "if" and "("
>>>>> Accepted.
>>>>>>
>>>>>> sun_awt_X11_GtkFileDialogPeer.c
>>>>>>
>>>>>> 392 if (gtk->gtk_check_version(2, 8, 0) == NULL) {
>>>>>>
>>>>>>
>>>>>> Maybe I am not looking at the right fn but I thought I saw
>>>>>> this fn return a boolean so a check against NULL looks wrong.
>>>>> The declaration is in GtkApi struct of gtk_interface.h. It returns
>>>>> char*. NULL means that the version is compatible.
>>>>>>
>>>>>> 393
>>>>>> gtk->gtk_file_chooser_set_do_overwrite_confirmation(GTK_FILE_CHOOSER(
>>>>>> 394 dialog), TRUE);
>>>>>>
>>>>>>
>>>>>> You didn't add this but it is awfully specific about the GTK version
>>>>>> and
>>>>>> I wonder if this test is doing what it should be doing on GTK 3?
>>>>> Accepted.
>>>>>>
>>>>>> It is interesting that some equivalent looking Java level dialog
>>>>>> checking in XToolkit.java
>>>>>> checked for 3.0 too ..
>>>>>>
>>>>>> swing_GTKEngine.c :
>>>>>>
>>>>>> 30 /* Static buffer for conversion from java.lang.String to
>>>>>> UTF-8 */
>>>>>> 31 static char convertionBuffer[CONV_BUFFER_SIZE];
>>>>>>
>>>>>> So the variable name should be spelt conversionBuffer.
>>>>> Accepted.
>>>>>>
>>>>>> awt_UNIXToolkit.c
>>>>>>
>>>>>> < 287 free(ret);
>>>>>>
>>>>>> You deleted this free(). Is that correct ? It seems to imply
>>>>>> you now expect a boolean return as discussed above and
>>>>>> so in that case NULL looks odd here (line 260) too.
>>>>> The JNI exported method returns boolean while the GTK method returns
>>>>> char*. free() is deleted intentionally according to the GTK docs it
>>>>> belongs to the library and should not be freed by user code.
>>>>>>
>>>>>> gtk3_interface.h :
>>>>>>
>>>>>> 36 #define G_PI 3.1415926535897932384626433832795028841971693993751
>>>>>>
>>>>>> I don't think that is completely accurate :-) And I should have
>>>>>> reviewed this yesterday [1].
>>>>> :) This is glib's definition I just copied.
>>>>>
>>>>> --Semyon
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> [1] http://www.piday.org/
>>>>>>
>>>>>>
>>>>>> On 03/05/2016 01:14 PM, Semyon Sadetsky wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review fix for JDK9:
>>>>>>>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8145547
>>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.00/
>>>>>>>
>>>>>>> The fix contains GTK3 based implementation for Swing GTK LnF, AWT
>>>>>>> FileChooser for Linux and AWT Robot for Linux.
>>>>>>> Also the new system property is added to request specific GTK
>>>>>>> version
>>>>>>> swing.gtk.version.
>>>>>>>
>>>>>>> --Semyon
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list