<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Semyon Sadetsky
semyon.sadetsky at oracle.com
Mon Mar 21 19:00:15 UTC 2016
On 3/21/2016 4:52 PM, Sergey Bylokhov wrote:
> 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.
XDesktopPeer loads GTK version. Upon GTK lib was load successfully the
value of the jdk.gtk.version property doesn't make sense anymore. I
don't see misconfiguration here.
>
>>>
>>>>> - 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.
Didn't get why is it unstable? This is a constant data.
>
>>
>> --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
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
More information about the swing-dev
mailing list