winit icon indicating copy to clipboard operation
winit copied to clipboard

[X11] Create windows with a parent window

Open t-sin opened this issue 2 years ago • 24 comments

This PR can create windows with a user-specified parent window. It's intended to realize what discussed in issue https://github.com/rust-windowing/winit/issues/159 for X11.

This is a retry https://github.com/rust-windowing/winit/pull/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

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

t-sin avatar Apr 06 '22 23:04 t-sin

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

t-sin avatar Apr 15 '22 22:04 t-sin

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

t-sin avatar Apr 20 '22 12:04 t-sin

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

t-sin avatar May 05 '22 23:05 t-sin

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

madsmtm avatar May 06 '22 09:05 madsmtm

Oh... It make sense.

t-sin avatar May 07 '22 01:05 t-sin

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).

dhardy avatar May 24 '22 20:05 dhardy

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

t-sin avatar Jun 14 '22 17:06 t-sin

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.

madsmtm avatar Jun 23 '22 16:06 madsmtm

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.

kchibisov avatar Jun 23 '22 17:06 kchibisov

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.

maroider avatar Jun 23 '22 17:06 maroider

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

kchibisov avatar Jun 23 '22 20:06 kchibisov

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.

dhardy avatar Jun 28 '22 16:06 dhardy

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.

kchibisov avatar Jun 28 '22 17:06 kchibisov

Quite aside from the use of softbuffer (which is now #2353), what is the status of this PR?

  • Winit has no X11 maintainer; nevertheless there isn't much code here and I left some review comments (still unaddressed)
  • Does Winit have any policies on additional platform-specific APIs?
  • From what I understand, WindowId on X11 has type x11-dl::xlib::Window which is the same value returned by fn xlib_window, thus it may make sense to pass a winit::WindowId into with_x11_parent (or it may not: WindowId is an enum over X11 and Wayland identifiers)

dhardy avatar Jul 03 '22 09:07 dhardy

what is the status of this PR?

Aside from the lack of an X11 maintainer, I suppose we're waiting on @t-sin to respond to your review and resolve the conflicts that have been introduced.

Winit has no X11 maintainer

Yep, this does suck, and it's a position I may have to fill myself going forward, unless a more interested (and actually qualified) candidate appears.

Does Winit have any policies on additional platform-specific APIs?

IIRC, it's mostly just that such APIs are best suited for things we cannot create a meaningful cross-platform abstraction over. I don't think there's anything written down about APIs we'd like to have a cross-platform abstraction over, but currently haven't invested the time in creating. I'm not personally opposed to adding a temporary platform-specific API for such cases at present time.

maroider avatar Jul 03 '22 10:07 maroider

Status reporting. I'm working visualized child window creations and found a bug https://github.com/rust-windowing/winit/pull/2363. This PR depends that PR so I'm waiting some reviews.

t-sin avatar Jul 05 '22 17:07 t-sin

Visualizing child window is finished. I've done for all review points, so this PR is ready for review. But CI fails in many platforms...

t-sin avatar Jul 05 '22 22:07 t-sin

Now CI fails in like i386-linux. I'm not familiar with X11, XID sometimes typedefs as a pointer...? CI fails in i386-linux because XID is not u32.

To solve it roughly, it needs implementing From<WindowId> for u32 or for usize I think, but any idea?

t-sin avatar Jul 05 '22 23:07 t-sin

To solve it roughly, it needs implementing From<WindowId> for u32 or for usize I think, but any idea?

(id as u64).into().

kchibisov avatar Jul 05 '22 23:07 kchibisov

Now CI on Linux is passed. I don't know why Windows CI fail.

t-sin avatar Jul 19 '22 13:07 t-sin

It looks like it's all softbuffer's fault.

maroider avatar Jul 20 '22 09:07 maroider

hmm... @dhardy may I remove softbuffer related code from this PR? Original example in this PR does not visualize each child windows but can demonstrate how to create child windows. Or I want to discuss about it.

t-sin avatar Jul 20 '22 13:07 t-sin

Of course. I had hoped softbuffer would be a simple and reliable dependency, but clearly it is not.

dhardy avatar Jul 21 '22 07:07 dhardy

All CI have passed.

t-sin avatar Jul 23 '22 02:07 t-sin

I stop resolving conflicts in this PR. Notify me when it can be review for merge.

t-sin avatar Aug 15 '22 14:08 t-sin

Rebasing has done!

t-sin avatar Sep 01 '22 12:09 t-sin

Rebased!

t-sin avatar Sep 04 '22 01:09 t-sin

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 avatar Sep 18 '22 01:09 t-sin