[X11] Create windows with a parent window #2246

t-sin · 2022-04-06T23:06:22Z

This PR can create windows with a user-specified parent window. It's intended to realize what discussed in issue #159 for X11.

This is a retry #2096. In that time I was not aware CI fails so passed long time, the master branch proceeds so much from that PR so I cannot follow changes.

TODO

Tested on all platforms changed
- X11
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

examples/child_window.rs

egui_sdl2_gl cannot create window with parent and with OpenGL context, it's because of the specification of SDL2 itself. so I should not use egui_sdl2_gl. Now I use egui-glow (internally uses egui-winit), but this don't support parent window, but I create Pull-Request to winit upstream (rust-windowing/winit#2246). For the merging that PR, as workarround, SoyBoy uses a folk of egui-glow on my GitHub accnount.

t-sin · 2022-04-15T22:49:18Z

All checks have passed! It's truly ready for review 🎊

examples/child_window.rs

t-sin · 2022-04-20T12:51:42Z

This PR followed the HEAD of master. Including document updates.

t-sin · 2022-05-05T23:24:54Z

What does this PR stop? I’ll try to resolve that if I can.

madsmtm · 2022-05-06T09:01:18Z

We don't have a maintainer for X11 atm, so there's not really anyone to neither review nor accept the PR, sadly

t-sin · 2022-05-07T01:26:05Z

Oh... It make sense.

dhardy

Not from your PR, but maybe you could also fix these errors?

#[doc(hidden)] is ignored on trait impl items

The code here... really, there's not much to review, besides the example.

Being able to visualise things is always nice, so I added softbuffer as a dev-dependency and modified the example. See my commits here: https://github.com/dhardy/winit/commits/with-parent-window-in-x11

That is, child windows are created at the mouse coordinates as a child of the window clicked in. Seems to work fine to me.

Maybe pull my commits into this PR? Although it's somewhat orthogonal; @madsmtm? (I also copied the softbuffer example into winit/examples.)

dhardy · 2022-05-13T07:51:24Z

CHANGELOG.md

@@ -40,10 +40,12 @@ And please only add new entries to the top of this list, right below the `# Unre
 - On iOS, send `RedrawEventsCleared` even if there are no redraw events, consistent with other platforms.
 - **Breaking:** Replaced `Window::with_app_id` and `Window::with_class` with `Window::with_name` on `WindowBuilderExtUnix`.
 - On Wayland and X11, fix window not resizing with `Window::set_inner_size` after calling `Window:set_resizable(false)`.
+<<<<<<< HEAD


Left over from a merge?

Oh sorry. Removed!

dhardy · 2022-05-13T14:44:36Z

src/platform/unix.rs

@@ -274,6 +274,9 @@ pub trait WindowBuilderExtUnix {

    #[cfg(feature = "x11")]
    fn with_x11_screen(self, screen_id: i32) -> Self;
+    #[cfg(feature = "x11")]
+    /// Build window with X11's parent window. Only relevant on X11.
+    fn with_x11_parent(self, parent_id: usize) -> Self;


One other point worth making: is it possible to use the winit::WindowId here?

For X11 only, this doesn't matter much. If this feature gets generalised to other platforms (hopefully), then it would be better to use the WindowId.

usize is likely the wrong type to use here. X11 window IDs are u32s, as far as I can tell (although x11-dl's typedefs do make things confusing, and the definition of XID in the headers wasn't fun to track down).

As of #2351, we can make a WindowId from arbitrary u64s, so using it instead should be fine.

One other point worth making: is it possible to use the winit::WindowId here?

As of #2351, we can make a WindowId from arbitrary u64s, so using it instead should be fine.

Good idea, thanks! I'll do that.

dhardy · 2022-05-24T20:20:07Z

Further note: the child windows created by this PR are limited to the area of their parent. In general, it can be useful to have child windows without this restriction (e.g. menus may extend beyond the bounds of the parent).

t-sin · 2022-06-14T17:35:57Z

Sorry I did my project... I can do this PR starting this week end maybe.

madsmtm · 2022-06-23T16:38:47Z

Re adding softbuffer to our examples: I think it makes more sense to direct people toward it via. documentation, mostly since they don't support the same platforms as we do (glutin would be a better fit here), and is not a crate we're really in control of.

kchibisov · 2022-06-23T17:28:35Z

Re adding softbuffer to our examples: I think it makes more sense to direct people toward it via. documentation, mostly since they don't support the same platforms as we do (glutin would be a better fit here), and is not a crate we're really in control of.

Using softbuffer should resolve issues with examples on Wayland, so it should simplify testing.

maroider · 2022-06-23T17:33:54Z

I'm personally not entirely sold on softbuffer's current API, but I think the benefits of adding it to the examples (i.e. no more complaints about windows not showing up on wayland) are worthwhile.

kchibisov · 2022-06-23T20:04:20Z

Yeah, I just want to have something that puts pixels into window, I'm not recommending softbuffer in particular.

dhardy · 2022-06-28T16:48:44Z

mostly since they don't support the same platforms as we do (glutin would be a better fit here)

Will there ever be a perfect fit? Moreover, I'm skeptical that using a complex hardware-accelerated API as the default for visualizations is the most reliable choice, even if GL does work basically everywhere.

Given some direction I'm happy to author a different PR regarding softbuffer-in-winit, but it's simple enough that anyone here could do this.

kchibisov · 2022-06-28T17:17:26Z

Given some direction I'm happy to author a different PR regarding softbuffer-in-winit, but it's simple enough that anyone here could do this.

If you feel like it you can go for it for sure.

CHANGELOG.md

src/platform/x11.rs

madsmtm · 2022-09-02T12:55:25Z

src/platform/x11.rs

@@ -171,6 +172,9 @@ pub trait WindowBuilderExtX11 {
    fn with_x11_visual<T>(self, visual_infos: *const T) -> Self;

    fn with_x11_screen(self, screen_id: i32) -> Self;
+    #[cfg(feature = "x11")]


Nit: Also remove this

This is still not removed.

kchibisov

Could you rebase. Should be good after that.

t-sin · 2022-09-04T01:43:18Z

Rebased!

kchibisov · 2022-09-05T20:20:02Z

src/platform/x11.rs

@@ -171,6 +172,9 @@ pub trait WindowBuilderExtX11 {
    fn with_x11_visual<T>(self, visual_infos: *const T) -> Self;

    fn with_x11_screen(self, screen_id: i32) -> Self;
+    #[cfg(feature = "x11")]


This is still not removed.

kchibisov · 2022-09-05T20:20:37Z

src/platform/x11.rs

@@ -171,6 +172,9 @@ pub trait WindowBuilderExtX11 {
    fn with_x11_visual<T>(self, visual_infos: *const T) -> Self;

    fn with_x11_screen(self, screen_id: i32) -> Self;
+    #[cfg(feature = "x11")]
+    /// Build window with X11's parent window. Only relevant on X11.


We're inside the X11 extension.

Suggested change

/// Build window with X11's parent window. Only relevant on X11.

/// Build window with parent window.

thanks! fixed.

kchibisov · 2022-09-05T20:22:01Z

src/platform_impl/linux/x11/window.rs

@@ -120,7 +120,16 @@ impl UnownedWindow {
        pl_attribs: PlatformSpecificWindowBuilderAttributes,
    ) -> Result<UnownedWindow, RootOsError> {
        let xconn = &event_loop.xconn;
-        let root = event_loop.root;
+        // root should be a type ffi::Window and it is finally c_ulong.


Remove this comment entirely, there's no value in it. The WindowID thing is clear.

kchibisov · 2022-09-05T20:26:42Z

src/platform_impl/linux/x11/window.rs

+            // XID is defined as 32-bit value in the X11 protocol so
+            // there's no problem about higher bits truncation.
+            // cf. https://www.x.org/docs/XProtocol/proto.pdf


This comment also feels a bit strange. You can say WindowId is XID under the hood which doesn't exceed u32, so the conversion is lossless.

Because of X11/Wayland module separation

t-sin · 2022-09-18T01:05:06Z

Thanks @kchibisov for reviewing my some nits. I think these are fixed now (checked all diffs in the "Files changed" tab), but cargo check fails in my machine so I wait CI results...

EDITED: and sorry for late because of my private travelling...

t-sin marked this pull request as ready for review April 6, 2022 23:21

t-sin commented Apr 6, 2022

View reviewed changes

examples/child_window.rs Outdated Show resolved Hide resolved

msiglreith added the DS - x11 label Apr 13, 2022

t-sin force-pushed the with-parent-window-in-x11 branch 2 times, most recently from 573f967 to b2c3199 Compare April 15, 2022 22:41

t-sin mentioned this pull request Apr 15, 2022

Support for creating child windows #159

Open

dhardy reviewed Apr 16, 2022

View reviewed changes

examples/child_window.rs Show resolved Hide resolved

t-sin force-pushed the with-parent-window-in-x11 branch 2 times, most recently from bd5621d to ef6e8c8 Compare April 20, 2022 12:40

t-sin force-pushed the with-parent-window-in-x11 branch from ef6e8c8 to d36181a Compare April 25, 2022 02:59

t-sin force-pushed the with-parent-window-in-x11 branch from d36181a to a517678 Compare May 2, 2022 09:10

t-sin force-pushed the with-parent-window-in-x11 branch from a517678 to 904be7d Compare May 13, 2022 06:09

dhardy reviewed May 13, 2022

View reviewed changes

dhardy mentioned this pull request Jun 1, 2022

Layers iced-rs/iced#30

Closed

dhardy mentioned this pull request Jun 29, 2022

Examples: less redundancy, improve fullscreen #2350

Merged

5 tasks

madsmtm reviewed Sep 1, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/platform/x11.rs Outdated Show resolved Hide resolved

t-sin force-pushed the with-parent-window-in-x11 branch 2 times, most recently from aded31f to 7dbf8fc Compare September 2, 2022 11:48

madsmtm reviewed Sep 2, 2022

View reviewed changes

t-sin force-pushed the with-parent-window-in-x11 branch from 7dbf8fc to 5efd863 Compare September 3, 2022 14:57

kchibisov requested changes Sep 3, 2022

View reviewed changes

t-sin force-pushed the with-parent-window-in-x11 branch from 5efd863 to d5b0d66 Compare September 4, 2022 01:23

kchibisov requested changes Sep 5, 2022

View reviewed changes

t-sin added 16 commits September 18, 2022 09:48

Add an attribute to store its parent window to Unix WindowBuilder

edee7d0

Use parent ID when creating a new X11 window

aaeef3c

Add test/example code for child windows

182250f

Use platform_impl::WindowId to specify the parent window on X11

9dd9722

Support 32-bit archtecture

b9190a2

Archtecture specific conversion by its pointer width

83f1d9f

Fix errors by `cargo clippy --example child_window

2ba6432

Don't import WindowId with the feature wayland

08a3c71

Use casting with higher bits truncation instead of try_into()

522de7c

Remove unneccesary pointer-width conditional

5a780c3

Rename with_x11_parent() -> with_parent()

a7b69f2

Because of X11/Wayland module separation

The child window example follows the new platform module structure

40ab6b1

Update FEATURES.md

4577d0f

Update CHANGELOG.md

3b44e02

Remove #[cfg(feature = "x11")] because this file is X11 specific

87dad50

Rewrite some comments for WindowID conversions

c920d89

t-sin force-pushed the with-parent-window-in-x11 branch from d5b0d66 to c920d89 Compare September 18, 2022 00:58

t-sin mentioned this pull request Sep 21, 2022

Follow winit's new parent window interface t-sin/soyboy-sp.vst3#37

Open

kchibisov approved these changes Oct 9, 2022

View reviewed changes

kchibisov merged commit 71094e5 into rust-windowing:master Oct 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[X11] Create windows with a parent window #2246

[X11] Create windows with a parent window #2246

t-sin commented Apr 6, 2022 •

edited

Loading

t-sin commented Apr 15, 2022

t-sin commented Apr 20, 2022

t-sin commented May 5, 2022

madsmtm commented May 6, 2022

t-sin commented May 7, 2022

dhardy left a comment •

edited

Loading

dhardy May 13, 2022

t-sin Jul 5, 2022

dhardy May 13, 2022 •

edited

Loading

maroider Jul 3, 2022 •

edited

Loading

t-sin Jul 5, 2022

dhardy commented May 24, 2022

t-sin commented Jun 14, 2022

madsmtm commented Jun 23, 2022

kchibisov commented Jun 23, 2022

maroider commented Jun 23, 2022

kchibisov commented Jun 23, 2022

dhardy commented Jun 28, 2022

kchibisov commented Jun 28, 2022

madsmtm Sep 2, 2022

t-sin Sep 3, 2022

kchibisov Sep 5, 2022

kchibisov left a comment

t-sin commented Sep 4, 2022

kchibisov Sep 5, 2022

kchibisov Sep 5, 2022

t-sin Sep 25, 2022

kchibisov Sep 5, 2022

t-sin Sep 25, 2022

kchibisov Sep 5, 2022

t-sin Sep 25, 2022

t-sin commented Sep 18, 2022 •

edited

Loading

	/// Build window with X11's parent window. Only relevant on X11.
	/// Build window with parent window.

[X11] Create windows with a parent window #2246

[X11] Create windows with a parent window #2246

Conversation

t-sin commented Apr 6, 2022 • edited Loading

TODO

t-sin commented Apr 15, 2022

t-sin commented Apr 20, 2022

t-sin commented May 5, 2022

madsmtm commented May 6, 2022

t-sin commented May 7, 2022

dhardy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy May 13, 2022 • edited Loading

Choose a reason for hiding this comment

maroider Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented May 24, 2022

t-sin commented Jun 14, 2022

madsmtm commented Jun 23, 2022

kchibisov commented Jun 23, 2022

maroider commented Jun 23, 2022

kchibisov commented Jun 23, 2022

dhardy commented Jun 28, 2022

kchibisov commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

t-sin commented Sep 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-sin commented Sep 18, 2022 • edited Loading

t-sin commented Apr 6, 2022 •

edited

Loading

dhardy left a comment •

edited

Loading

dhardy May 13, 2022 •

edited

Loading

maroider Jul 3, 2022 •

edited

Loading

t-sin commented Sep 18, 2022 •

edited

Loading