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

Add GTK handles #105

Closed
wants to merge 1 commit into from
Closed

Add GTK handles #105

wants to merge 1 commit into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Dec 20, 2022

This PR adds handles representing display and window handles for GTK 3 and 4.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks, only a minor nit.

src/unix.rs Outdated Show resolved Hide resolved
src/unix.rs Outdated
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct Gtk4DisplayHandle {
/// A pointer to a `GtkApplication`.
pub display: *mut c_void,
Copy link
Member

Choose a reason for hiding this comment

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

Both GTK3 and GTK4 can be used without a GtkApplication. I guess NULL could be used in that case? What would the GtkApplication be needed for?

Copy link
Member

Choose a reason for hiding this comment

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

In any case, shouldn't this actually be a GdkDisplay?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, GdkDisplay is probably the closest thing to a "display handle" in GTK. It has subclasses like which in turn holds things like wl_display.

Normally the display is just global state you can get with gdk_display_get_default(). I don't know if GTK can ever work with a display that isn't the "default".

Copy link
Member Author

Choose a reason for hiding this comment

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

Since GdkDisplay is at the level below GTK, I wanted to avoid exposing it. I can add it anyways, if you want.

Both GTK3 and GTK4 can be used without a GtkApplication. I guess NULL could be used in that case? What would the GtkApplication be needed for?

Yes, it can be null without any issues. GtkApplication can be used to tinker with the menu bar, which may actually be important in many cases.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds somewhat reasonable, though I think we're using RawDisplayHandle wrongly for this, then? Like, ideally we should have Raw{Application, Display, Window, Surface, ...}Handle?

Anyhow, at the very least the variable should be renamed from display to application.

Copy link
Member

Choose a reason for hiding this comment

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

If the window is associated with an application, you can get it with gtk_window_get_application(). So it seems redundant, and possibly better to just leave the display handle empty like WebDisplayHandle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I still think that we should have a pointer to it, if for no other reason than to elide a call to gtk_window_get_application().

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Could you add a CHANGELOG entry for that as well.

src/gtk.rs Outdated Show resolved Hide resolved
src/gtk.rs Outdated Show resolved Hide resolved
@ids1024
Copy link
Member

ids1024 commented Dec 27, 2022

If this takes a "A pointer to a GtkWidget", how exactly would it be used? An arbitrary widget isn't necessarily something you can add a child widget to, etc.

@notgull
Copy link
Member Author

notgull commented Dec 28, 2022

The more that I think about this, the more I think that we should discuss first how higher-level toolkits like GTK and Qt should integrate into this crate. I'll close this for now.

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

Successfully merging this pull request may close these issues.

None yet

4 participants