Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GTK4] Migrate deprecated FontChooser to FontDialog #1583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ptziegler
Copy link
Contributor

This moves all native FontChooser bindings from the shared GTK to the GTK3 component and also defines new GTK4 bindings for the FontDialog API.

Note: The FontDialog doesn't seem to remember the initial font that is passed as an argument. This looks like a bug within GTK, given that the same behavior also happens for the FontDialogButton[1].

[1] - https://gitlab.gnome.org/GNOME/gtk/-/issues/6892

@ptziegler
Copy link
Contributor Author

ptziegler commented Nov 9, 2024

In terms of appearance, both dialogs are pretty much identical, though I think the old one looks better...

GTK3:
image

GTK4:
image

Note that this PR currently depends on #1582 because I'm using the new AsyncReadyCallback class.

Copy link
Contributor

github-actions bot commented Nov 9, 2024

Test Results

   383 files   -   100     383 suites   - 100   5m 12s ⏱️ - 2m 34s
 4 095 tests ±    0   4 088 ✅ +    3   7 💤 ±0  0 ❌  - 3 
12 147 runs   - 4 026  12 064 ✅  - 4 016  83 💤  - 7  0 ❌  - 3 

Results for commit 0ab423e. ± Comparison against base commit 7300cb4.

♻️ This comment has been updated with latest results.

@akurtakov
Copy link
Member

This PR should be kept strictly to FontChooser->FontDialog change and the async helper code change together with adopting existing code should go into #1582

@ptziegler
Copy link
Contributor Author

This PR should be kept strictly to FontChooser->FontDialog change and the async helper code change together with adopting existing code should go into #1582

Of course, that's why this is still marked as "draft" :)

The plan was to wait for the other refactoring to be merged, do a rebase and then the offending commit should disappear on its own. I just didn't want to use the "old" approach when refactoring it anyway.

@laeubi
Copy link
Contributor

laeubi commented Nov 27, 2024

@ptziegler we have now GTK4 build enabled, so if you rebase your PR it will at last check that compilation has no issues!

@akurtakov
Copy link
Member

Please continue with this one.
Note that starting with https://download.eclipse.org/eclipse/downloads/drops4/I20241127-1240/ builds for gtk4 are provided and can be started passing SWT_GTK4=1 env variable.

@ptziegler ptziegler marked this pull request as ready for review November 27, 2024 20:38
public void async(long callback) {
// The font dialog ignores the given font and simply picks the first installed font
// See https://gitlab.gnome.org/GNOME/gtk/-/issues/6892
GTK4.gtk_font_dialog_choose_font(handle, shellHandle, font.handle, 0, callback, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in the linked issue: What exactly is the procedure for dealing with actual bugs in GTK? As far as I can tell, there is no way to work around this problem inside of SWT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option is to stay with FontChooser until it's fixed and use FontDialog in an "if (GTK4 > 4.16)" or similar version block where we know it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried out the FontChooser with GTK4 and there I see the same issue...
Given that the FontChooser dialog looks exactly like the FontDialog dialog, I believe that internally, the method calls are simply delegated from one to the other. So I don't think there is anything to gain by keeping the old implementation.

This moves all native FontChooser bindings from the shared GTK to the
GTK3 component and also defines new GTK4 bindings for the FontDialog
API.

Note: The FontDialog doesn't seem to remember the initial font that is
passed as an argument. This looks like a bug within GTK, given that the
same behavior also happens for the FontDialogButton[1].

[1] - https://gitlab.gnome.org/GNOME/gtk/-/issues/6892
@ptziegler
Copy link
Contributor Author

I don't see any warnings/errors regarding the gtk_font_dialog methods, so I believe the building the binaries fails due to -Werror.

2024-12-08T07:47:12.4387322Z [INFO] [exec] -e *** GTK4 Build failed, aborting further actions..
2024-12-08T07:47:12.4394328Z [INFO] [exec] cc1: all warnings being treated as errors
2024-12-08T07:47:12.4399139Z [INFO] [exec] make: *** [make_linux.mak:144: os.o] Error 1

But why doesn't this also happen on the master?

@laeubi
Copy link
Contributor

laeubi commented Dec 8, 2024

The Jenkins build succeeded but windows was aborted/timed out

grafik

For the Github verification, this might be because ubuntu is using a more recent GTK version (with more deprecation) as our Jenkins job. So this can be ignored for now until we reaching a clean state, that's also the reason we currently not build for other architectures than x86.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants