<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Victor D'yakov
victor.dyakov at oracle.com
Thu Mar 17 15:25:44 UTC 2016
On 17.03.2016 6:25, Semyon Sadetsky wrote:
> The awt_Desktop.c is missed in the webrev.
> Please see the updated one:
> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.02/
>
> --Semyon
>
> On 3/17/2016 3:57 PM, Sergey Bylokhov wrote:
>> 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/
>>
>> On my system this version fails to build(Ubuntu 15.10 + gcc 5.2.1):
>>
>> ERROR: Build failed for target 'images' in configuration
>> 'linux-x86_64-normal-server-release' (exit code 2)
>> === Output from failing command(s) repeated here ===
>> * For target support_native_java.desktop_libawt_xawt_awt_Desktop.o:
>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:
>> In function ‘Java_sun_awt_X11_XDesktopPeer_init’:
>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:46:9:
>> error: implicit declaration of function ‘gtk2_load’
>> [-Werror=implicit-function-declaration]
>> if (gtk2_load(env) && gtk2_show_uri_load(env)) {
>> ^
>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:46:27:
>> error: implicit declaration of function ‘gtk2_show_uri_load’
>> [-Werror=implicit-function-declaration]
>> if (gtk2_load(env) && gtk2_show_uri_load(env)) {
>> ^
>> cc1: all warnings being treated as errors
>> === End of repeated output ===
>> === Make failure sequence repeated here ===
>> Awt2dLibraries.gmk:369: recipe for target
>> '/mnt/hgfs/moe/workspaces/jdk/9/client-gate/build/linux-x86_64-normal-server-release/support/native/java.desktop/libawt_xawt/awt_Desktop.o'
>> failed
>> make/Main.gmk:175: recipe for target 'java.desktop-libs' failed
>> === End of repeated output ===
>>
>>
>>> 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.
Ok.
Please place the JPRT link here on review thread once you are ready.
Thanks,
Victor
>>>> 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