[GTK] Use gtk_widget_set_visible instead of gtk_widget_hide/show

[fedejeanne]

Use the old native method as a wrapper that redirects to GTK3/4

[fedejeanne]

I was wondering if this is a valid approach, @akurtakov ?

Also, I don't know why but the contribution headers were automatically removed, either when I saved some changes or when I ran the Maven target to build the SWT native binaries. Does this happen to you too?

[akurtakov]

This complicates things IMO. As https://docs.gtk.org/gtk3/method.Widget.set_visible.html is available since Gtk 2.18 it should be valid to replace the calls to gtk_widget_show/hide with gtk_widget_set_visible and have things even clearer. Regarding the headers - this has never happened to me.

[github-actions[bot]]

Test Results

   545 files  + 6     545 suites  +6   30m 20s ⏱️ +37s  4 377 tests +37   4 359 ✅ +35   18 💤 +3  0 ❌  - 1  16 647 runs  +37  16 506 ✅ +35  141 💤 +3  0 ❌  - 1 

Results for commit 6bd572e8. ± Comparison against base commit 4df99bad.

:recycle: This comment has been updated with latest results.

[akurtakov]

You can see how straightforward the c code is at https://gitlab.gnome.org/GNOME/gtk/-/blob/gtk-3-24/gtk/gtkwidget.c?ref_type=heads#L9091 so if you want to reduce deprecated usages please start replacing gtk_widget_hide/show with gtk_widget_set_visible.

[fedejeanne]

I know that the change is trivial: just renaming and adding 1 boolean parameter i.e. in GTK4:

• GTK.gtk_widget_show(w) becomes GTK4.gtk_widget_set_visible(w, true)
• GTK.gtk_widget_hide(w) becomes GTK4.gtk_widget_set_visible(w, false)

My point was that it would be better to preserve the existing methods GTK.gtk_widget_show and GTK.gtk_widget_hide and use them to delegate to their natives implementations in GTK3/4. This would reduce the amount of if (GTK.GTK4) { ... } else { ... } constructs (duplicated code) and centralize it in the class GTK, which would make the next step in the migration (deprecating and removing the calls to GTK3) easier since they would all be centralized.

If this approach (centralizing the if-else block in the GTK class and using that class to delegate to the native methods) is OK then we could try with more complex scenarios where the GTK3 methods and their replacements in GTK4 have different parameters or the way the methods should be used is completely different.

[...] if you want to reduce deprecated usages please start replacing gtk_widget_hide/show with gtk_widget_set_visible.

I reduced the usage of deprecated methods from 80+ to 2 since GTK.gtk_widget_hide/show are not the native (deprecated) methods anymore, they just happen to be named the same for simplicity ;-)

[fedejeanne]

BTW the next step in the migration (deprecating and removing the calls to GTK3) would be easier because one would only have to:

• Remove the if-else blocks in GTK.gtk_widget_hide/show and leave only the call to GTK4.gtk_widget_set_visible(w, true/false)
• Inline all the calls to the GTK.gtk_widget_hide/show methods and then remove the methods (automated refactoring)
• Delete the unnecessary native methods GTK3.gtk_widget_hide/show

[akurtakov]

My suggestion if you want to continue that path is to put the method delegating to gtk3/4 methods in Widget.java for the sake of consistency - there are already a bunch of such "helper" methods there like gdk_event_free/gdk_event_get_surface_or_window/gdk_event_get_state/gtk_box_set_child_packing/gtk_box_pack_end/gtk_container_get_border_width_or_margin/etc. The overall goal is to not touch Gtk/Gtk3/Gtk4 unless actual binding change happens so to not cause native rebuilds.

[akurtakov]

BTW the next step in the migration (deprecating and removing the calls to GTK3) would be easier because one would only have to:

* Remove the `if-else` blocks in `GTK.gtk_widget_hide/show` and leave only the call to `GTK4.gtk_widget_set_visible(w, true/false)`

You can not call GTK4.gtk_widget_set_visible on GTK3 as it will be linked to gtk4.so file which will cause runtime crash. The method has to stay in GTK.java if it's to be used in both GTK3/4.

* Inline all the calls to the `GTK.gtk_widget_hide/show` methods and then remove the methods (automated refactoring)

* Delete the unnecessary native methods  `GTK3.gtk_widget_hide/show`

[fedejeanne]

You can not call GTK4.gtk_widget_set_visible on GTK3 as it will be linked to gtk4.so file which will cause runtime crash. The method has to stay in GTK.java if it's to be used in both GTK3/4.

I know that, but I see that I was unclear so let me rephrase: when the time is proper and all code using GTK3 is to be removed from SWT (it might take years to reach that point) having all such if-else constructs centralized will ease the task. But of course, once that happens, it means that running Eclipse on GTK3 will be not possible anymore.

Thank you for the hint about the helper methods on the Widget class, I didn't know that :-)

[HeikoKlare]

Also, I don't know why but the contribution headers were automatically removed, either when I saved some changes or when I ran the Maven target to build the SWT native binaries. Does this happen to you too?

I adapted the JNI generator tools when introducing a new JNI class to the Win32 implementation (https://github.com/eclipse-platform/eclipse.platform.swt/pull/2054), as the auto-generated contributor information was basically wrong. Since this contributor information is rather unnecessary (we have the Git history for that) and cannot be properly maintained for JNI-generated classes (it will always just contain IBM) anyway, I chose to remove it.

[fedejeanne]

@akurtakov I moved the methods to Widget like you suggested. Only a few locations can't use them so I had to resort to using the if-else blocks.

[fedejeanne]

Hm, odd. There are issues with the native binaries in Linux:

Error:  Tests run: 140, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 5.237 s <<< FAILURE! -- in org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo
Error:  org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_copy -- Time elapsed: 0.645 s <<< ERROR!
java.lang.UnsatisfiedLinkError: 'void org.eclipse.swt.internal.gtk3.GTK3.gtk_editable_paste_clipboard(long)'
	at org.eclipse.swt.internal.gtk3.GTK3.gtk_editable_paste_clipboard(Native Method)
	at org.eclipse.swt.widgets.Combo.paste(Combo.java:1968)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_copy(Test_org_eclipse_swt_widgets_Combo.java:238)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	Suppressed: java.lang.Throwable: Screenshot written to /tmp/org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_copy.png
		at org.eclipse.test.Screenshots$ScreenshotOnFailure.failed(Screenshots.java:41)

Error:  org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_paste -- Time elapsed: 0.565 s <<< ERROR!
java.lang.UnsatisfiedLinkError: 'void org.eclipse.swt.internal.gtk3.GTK3.gtk_editable_paste_clipboard(long)'
	at org.eclipse.swt.internal.gtk3.GTK3.gtk_editable_paste_clipboard(Native Method)
	at org.eclipse.swt.widgets.Combo.paste(Combo.java:1968)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_paste(Test_org_eclipse_swt_widgets_Combo.java:503)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	Suppressed: java.lang.Throwable: Screenshot written to /tmp/org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Combo.test_paste.png
		at org.eclipse.test.Screenshots$ScreenshotOnFailure.failed(Screenshots.java:41)

@akurtakov Do you know what could be the issue here?

[akurtakov]

It's because of the gtk4 label. The GH builder tries to run the tests with gtk4 in this case but this port still crashes. I'll remove the label and restart it.

[akurtakov]

As it's restart of failed builds it still does run tests on gtk4. Maybe when you fix the review issues it will start on gtk3?

[laeubi]

@akurtakov actually I would just keep the gtk4 label, as Jenkins run the test as well, we can at least see that GTK4 still crash (but maybe not crash that often). In any case if you add/remove the label after PR creation, one need to push new changes to the branch to take it into effect.

If it makes too much trouble we can also think about changing the label to trigger this e.g. to gtk4-tests and use the gtk4 label as general purpose.

[akurtakov]

@laeubi It would be best to have the gtk3 build always and the gtk4 being an extra one not showing failure (if possible) for now or with a label (ignored/for devel purposes only).

[laeubi]

It would be best to have the gtk3 build always

gtk3 +4 are always build the only difference is if tests are executed using gtk3 or gtk4. Making the gtk4 an extra one would require more work and possible duplication, and you can't easily compare it against master build.

and the gtk4 being an extra one not showing failure (if possible) for now or with a label (ignored/for devel purposes only).

gtk4 currently is that label you can add for devel purpose. That's why I suggested to maybe change it to gtk4-test or similar if that fits better.

Another approach of course would be to have that test not working on GTK4 have an assumeThat(GTK_VERSION == 3) I think the goal should be that at least nothing crashes, otherwise it feels quite hard to improve GTK4 support if we can not even run the tests.

[akurtakov]

Thanks!