-
-
Notifications
You must be signed in to change notification settings - Fork 785
Initial libadwaita support for GTK4 backend #3925
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
base: main
Are you sure you want to change the base?
Conversation
johnzhou721
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments inline about the way things are in this PR.
This PR makes all currently implemented GTK4 functionalities compatible with libadwaita, which is why ActivityIndicator is in here.
Ready for review.
| "cover-if-plain-gtk4": "os_environ.get('TOGA_GTK', '') != '4' or " | ||
| "os_environ.get('TOGA_GTKLIB', '') != ''", | ||
| "cover-if-plain-gtk": "os_environ.get('TOGA_GTKLIB', '') != ''", | ||
| "cover-if-libadwaita": "os_environ.get('TOGA_GTK', '') != '4' or " | ||
| "os_environ.get('TOGA_GTKLIB', '') != 'Adw'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is following the no-cover prefix convention perhaps better here? I thought it'd be better to be like "cover [only] if plain gtk" rather than the contrapositive "no cover if not plain gtk".
| if Adw is None: | ||
| assert isinstance(self.app._impl.native, Gtk.Application) | ||
| else: | ||
| assert isinstance(self.app._impl.native, Adw.Application) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, Adw.Application subclasses Gtk.Application... however, I used this stronger assertion because without Adw.Application, window bottom corners doesn't round at all.
Same with Adw.Window
| # libadwaita 1.6.0 is not in Ubuntu 24.04 yet; no-cover it. | ||
| if Adw is not None and ADW_VERSION >= (1, 6, 0): # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this no-cover perhaps makes you nervous? Should we land Adwaita 1.6.0 changes until all majors distros ship it? Right now Ubuntu 24.04 does not.
| GLIB_VERSION: tuple[int, int, int] = ( | ||
| GLib.MAJOR_VERSION, | ||
| GLib.MAJOR_VERSION, | ||
| GLib.MAJOR_VERSION, | ||
| GLib.MINOR_VERSION, | ||
| GLib.MICRO_VERSION, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe I didn't catch this before...
| - "linux-wayland-gtk4-adw" | ||
| - "linux-wayland-qt" | ||
| - "linux-x11-qt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI -- the Qt things worked before because Qt overrode all the variables in the include section.
| $ export TOGA_GTK=4 | ||
| ``` | ||
|
|
||
| The experimental GTK 4 backend also aims to provides support for integrating with desktop environment-specific libraries. At present, `libadwaita` is the only supported library of this kind. This functionality requires libadwaita 1.5 or newer. To enable libadwaita integration support, set: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libadwaita 1.5 changed the way dialog things are handled, and also the application "about" dialogs. The relevant APIs are deprecated in libadwaita 1.6.
All major distros ship libadwaita 1.5+. Most major distros ship libadwaita 1.6+.
This comment was marked as resolved.
This comment was marked as resolved.
|
@johnzhou721 Can you verify whether this is ready for review? Thanks. |
|
@kattni Verified. I explicitedly stated this and resolved the next comment about CI fails. |
|
@johnzhou721 When something is ready for review, that means that there are no other changes necessary. When there are commits after you state that it is ready for review, the assumption will be that it is not, in fact, ready for review. Therefore, I will verify with you whether it is actually ready, before handing it off. In the future, if you would like to avoid this, please ensure that you have all your intended changes completed before asking for a review, and if you do need to submit more changes, please restate that it is ready once you have completed those. |
Remove the 'invalid_size_while_hidden' attribute from BoxProbe.
|
@kattni Thanks for letting me know about what caused the confusion. I'll try to avoid doing this in the future, but in this case it was neccesary. For a specific explanation, the commit resolves an artifact from the merge; it was hard for me to test all the environments at once locally. However I understand that this is not an excuse — and I apologize for spending a bit of your time doing the verification. Since then I realized an error in the commit you referenced -- I've pushed another update, and reconfirming this is ready for review. |
|
@johnzhou721 Thank you for following up. I've handed this off to Russell. To be clear, I'm not saying that you can't submit subsequent commits, I'm simply saying that you need to let us know afterwards that it is again ready for review. |
Refs #3069.
Adds initial support for Window, Application, and ActivityIndicator for libadwaita. This PR thus makes all currently implemented GTK4 functionalities compatible with libadwaita.
Corners are now rounded -- yay!!!
PR Checklist: