<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Phil Race
philip.race at oracle.com
Tue Apr 5 18:35:04 UTC 2016
I see. I looked through the newly changed files and it seems fine although
I don't have time to re-apply the updated patch.
-phil.
On 04/05/2016 11:16 AM, Semyon Sadetsky wrote:
> XTaskbarPeer.java, awt_Taskbar.c and awt_Taskbar.h were changed to use
> the new common gtk_interface.
>
> Glib calls which are necessary for the taskbar module are added to
> gtk_interface: g_object_unref(), g_list_append(), g_list_free(),
> g_list_free_full().
>
> --Semyon
>
>
> On 4/5/2016 8:54 PM, Phil Race wrote:
>> Could you point me to what was updated ?
>>
>> -phil.
>>
>> On 04/05/2016 10:04 AM, Semyon Sadetsky wrote:
>>> The fix is updated because JEP 272 was pushed. JEP 272 has
>>> dependency on GTK.
>>>
>>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.04/
>>>
>>> --Semyon
>>>
>>> On 3/18/2016 4:02 PM, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 3/17/2016 10:57 PM, Phil Race wrote:
>>>>> I ran your changes (which included my fix) through jprt and all
>>>>> Solaris platforms failed as follows :-
>>>>>
>>>>> jdk/src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c",
>>>>> line 37: error: typedef redeclared: GtkWindow (E_TYPEDEF_REDECLARED)
>>>>> cc: acomp failed for
>>>>> jdk/src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c
>>>>>
>>>> I have fixed the Solaris build:
>>>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.03/.
>>>>
>>>> > gint intval = NULL;
>>>> I think this happens with you specific compiler only. Old
>>>> gtk2_interface.c always contained the same line.
>>>>
>>>> I ran JPRT with webrev.03. The URL is:
>>>> http://sthjprt.uk.oracle.com/archives/2016/03/2016-03-18-070745.ssadetsk.client/
>>>>
>>>> It seem macosx_x64_10.9-product-c2-jdk_math build failed because of
>>>> another change. (?) Could you help me to interpret the JPRT job
>>>> status?
>>>>
>>>> --Semyon
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 03/17/2016 09:42 AM, Phil Race wrote:
>>>>>> This reminded me I had not yet tried the build myself.
>>>>>> When I did I not get the error that Sergey did but I got this as
>>>>>> well :-
>>>>>>
>>>>>> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:
>>>>>> In function ‘get_integer_property’:
>>>>>> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:2542:19:
>>>>>> error: initialization makes integer from pointer without a cast
>>>>>> [-Werror]
>>>>>> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:
>>>>>> In function ‘get_boolean_property’:
>>>>>> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:2549:19:
>>>>>> error: initialization makes integer from pointer without
>>>>>>
>>>>>> Both lines it complains about were :
>>>>>>
>>>>>> gint intval = NULL;
>>>>>>
>>>>>> I changed them to
>>>>>> gint intval = 0;
>>>>>>
>>>>>> I am not using the 'official' compiler, but you really should
>>>>>> make sure jprt is
>>>>>> happy before confirming the webrev as final - and also fix all of
>>>>>> these even if
>>>>>> the official compiler does not complain.
>>>>>>
>>>>>> And I did not notice any problems running SwingSet2 with either
>>>>>> of GTK2 or GTK3
>>>>>> specified. It at least ran fine and clearly picked up different
>>>>>> versions.
>>>>>>
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>>
>>>>>> On 03/17/2016 06:25 AM, 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.
>>>>>>>>>> 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