<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 20:38:13 UTC 2016
On 3/21/2016 10:56 PM, Sergey Bylokhov wrote:
> On 21.03.16 22:30, Semyon Sadetsky wrote:
>>
>>
>> On 3/21/2016 10:20 PM, Sergey Bylokhov wrote:
>>> On 21.03.16 22:00, Semyon Sadetsky wrote:
>>>>> 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.
>>>
>>> XDesktopPeer can start load one version and Unix toolkit can start to
>>> load another one, because getEnabledGtkVersion() will return a
>>> different values. This is a standard approach to read properties in th
>>> e beginning(like sun.java2d.openg/d3d/xrender,
>>> awt.image.incrementaldraw, awt.image.redrawrate) and so on.
>> This is an another issue unrelated to the property. XDesktopPeer should
>> use the same synchronization monitor for GTK init.
>
> This does not solve the problem when check/init/load will try to use
> the different versions. I guess I provided enough arguments to
> initialize these data only once.
I've already explained that GTK library cannot be loaded twice, only one
version may be loaded regardless of the property value.
>
>>>
>>>>>
>>>>> 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.
>>>
>>> Because the values will be changed if the order of elements in the
>>> enum will be changed or the new value will be added.
>> If you meant future code changes, they should be changed synchronously.
>> This approach is used everywhere in GTK implementation since there
>> plenty different constants that need to be transferred between java and
>> the native code.
>
> It will be better to make it obvious and not to depends on some order.
> I am curious where you find it widely used. At least in the client I
> found only one usage.
I can help you. Look at GTKEngine.java: WidgetType, StateType, Settings,
ShadowType, Orientation, PositionType, etc...
>
>>>
>>>>>
>>>>>>
>>>>>> --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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160321/69c7f1a7/attachment.html>
More information about the swing-dev
mailing list