<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Semyon Sadetsky
semyon.sadetsky at oracle.com
Thu Mar 17 13:25:56 UTC 2016
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