make most window types have a child window #1919

JAicewizard · 2021-08-14T12:56:26Z

This does a couple of things.
It adds the notion of parent and child windows. all non-main windows have a parent window.
All non-main windows have their coordinate space shifted, so that the origin is the location of the main window.
This is usefull in almost all usecases.

This also removes the option to change the level after the fact. At least the GTK backend doesnt support this, and it would just be extra work to move all the widgets to the new window. I also dont think there is any usecase for this.

This solves positioning not working in wayland (#1757) and resolves the discussion over at #1824.

jneem

Nice! I'd probably wait a bit to see if anyone else objects...

jneem · 2021-08-14T14:02:03Z

druid-shell/src/backend/mac/window.rs

@@ -102,13 +102,23 @@ pub(crate) struct WindowHandle {
    /// a view. Also, this is better for hosted applications such as VST.
    nsview: WeakPtr,
    idle_queue: Weak<Mutex<Vec<IdleKind>>>,
+    parent: Option<Box<crate::WindowHandle>>,


I think the Box is unnecessary.

no because mac is different. It would be an infinitely big struct.

All the others already have a reference in between the window handle and this option, mac doesnt.

why does mac need to store parent handle though?

To change the coordinate space relative to the parent

Oh, I see, it's because on mac you've put the parent directly in the WindowHandle. Why not put it in ViewState, so it's more similar to the other platforms?

I dont exactly know how to get that from the WindowHandle :(

I can't test because I don't have a mac, but looking at set_focused_text_field gives a hint, I think:

// Here, self is a &WindowHandle if let Some(view) = self.nsview.load().as_ref() { let state: *mut c_void = *view.get_ivar("viewState"); let state = &mut (*(state as *mut ViewState)); }

Oh yes I see, thanks! pushed the changes

JAicewizard · 2021-08-14T20:24:24Z

Id actualy first like to do some discussion about popups, to popups also need to have a parent? and do they also need to be in the parents coordinate space? Thinking about this again it seems that popups maybe should use the global coordinate space.

JAicewizard · 2021-08-14T20:28:11Z

nvm I forgot that we have modal as a 3rd window type, not popup :) Ignore my last message

CHANGELOG.md

Co-authored-by: Manmeet Maan <[email protected]>

maan2003 · 2021-08-16T11:05:22Z

I believe x11 backend position translation is TODO?

JAicewizard · 2021-08-16T11:14:09Z

X11 tooltips dont work, I could add the set_position conversion but it wouldn't do anything.

maan2003 · 2021-08-16T11:38:41Z

no, I think they work now (at least the sub_window example does)

JAicewizard · 2021-08-16T12:02:57Z

also the tooltip? I get

Aug 16 13:12:37.291 ERROR druid::win_handler: failed to create sub window: '[druid-shell/src/backend/x11/application.rs:430] self.state'
Aug 16 13:12:37.291 ERROR druid_shell::backend::x11::application: Error handling event: CONFIGURE_NOTIFY - failed to get window: No window with id 69206063```

JAicewizard · 2021-08-16T12:09:43Z

For me it gets opened as a non-floating window, but that's fine-ish.

cmyr

Okay as discussed I do have some longer-term ambitions here, but in the meantime this seems reasonable!

cmyr · 2021-08-16T14:52:27Z

druid-shell/src/backend/mac/window.rs

+        // TODO: Maybe collin can get this into a state where modal windows follow the parent?
+        // There is an API to do child windows, (https://developer.apple.com/documentation/appkit/nswindow/1419152-addchildwindow)


I'm not sure you can move the parent while a modal is presented?

Tooltips and dropdowns suffer from the same issue, we expect them to be closed when moving a window but ATM that is upto the user to program. It might still be worth implementing for those? if not Ill remove the comment.

ATM that is upto the user to program.

is it possible? We don't have any windowmove events

uhh oh. If I am going to be implementing proper parent tracking, I can only do this for GTK and with some research probably X11. Others will have to help me on different platforms.

I think we should give windowmove events and then the user should do whatever they like (move with the window or close the popup)

Ok, lets put that in another MR to not bloat this one.

druid-shell/src/backend/mac/window.rs

Co-authored-by: Colin Rofls <[email protected]>

jneem · 2021-09-16T20:36:53Z

This can be merged, right? (modulo conflicts)

JAicewizard · 2021-09-18T12:35:10Z

I fixed the conflicts, I think this should be ok to merge now.

CryZe · 2021-09-20T09:49:36Z

This PR somehow killed DropDown entirely on at least Windows. It doesn't do anything anymore (no window spawns).

Update: WS_CHILD somehow needs to be WS_POPUP now for the window to appear.

JAicewizard · 2021-09-20T09:58:39Z

WindowLevel::DropDown? I didnt even change anything with those, the parent is completely ignored. Are you sure that it is this PR specifically?

EDIT: Could be that I broke something during the merge conflicts?

CryZe · 2021-09-20T10:07:35Z

Nvm, you are right. This was indeed already broken on master one commit before your PR got merged. I'll look into deeper bisecting this later today (or I'll just send a PR, since even on the older commit when it still worked, it was far from looking like a drop down in the first place)

sjoshid · 2021-09-20T13:36:35Z

Tooltip in sub-windows example definitely got worse in Windows. Reverting the latest commit on sub_windows.rs example does fix it, but maybe it breaks in Gtk?

JAicewizard · 2021-09-20T14:14:05Z

druid/examples/sub_window.rs

-                                    ctx.window().get_position()
-                                        + window_pos.to_vec2()
-                                        + cursor_size.to_vec2(),
+                                    (window_pos.to_vec2() + cursor_size.to_vec2()).to_point(),


@sjoshid these lines? That means the coordinate space for tool-tips isn't calculated correctly. what doesnt work anymore?

Apologies for being vague. Yes those lines. The location of tooltip is way off target.

Could it be some DPI scaling? I cant really test but ill check if I can spot something related to that.

The problem, at least on Windows, is that the cursor position received in TooltipController is in window coords. So we need to go from window coords to screen coords because screen coords is what Windows expect when you create a tooltip.

Yes that conversion happens in win/window.rs. So that should be handled. Instead of adding the parent window coordinates in the users code we do it in druid_shell.

No because those coordinates are in global space while they should be in window space. they are converted to global space by druid_shell not the client code. This was part of the change, so it should work. Maybe it cant get the parents state? that could be an issue.

The solution i gave in previous comment is basically the old code. That was dumb on my part. lol. The previous code didnt make much sense to me.

Where in shell is this conversion happening?

https://github.com/linebender/druid/pull/1919/files#diff-1a1a9e4151b4aa7f5d4959110f78d6f99d7fa46ac42ce3d05ac2992ab3dfe6e9R1852

Check https://xi.zulipchat.com/#narrow/stream/147926-druid/topic/Tooltip.20broken.20in.20Windows/near/254600089

* Implementation differs slightly from linebender#1919. Unlike linebender#1919, this does NOT store parent window handle in `WindowState`. If you want to access parent window handle AFTER creating window use [GetParent](https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getparent). * Handles scaling. * Tooltip example in nursery tested with these changes. If this is accepted, I'll make changes in nursery too.

implement #1919 for x11 backend

JAicewizard added 3 commits August 14, 2021 14:49

make most window types have a child window

45baf0d

Fix some CI issues

cb8ffac

Import fmt

66c256b

jneem approved these changes Aug 14, 2021

View reviewed changes

Add TODOs for window following

7901598

cmyr self-requested a review August 15, 2021 18:28

JAicewizard added 4 commits August 15, 2021 21:29

put the parent option in the state part on mac

ac50a3b

Add to changelog

22cbb85

reverse the implementation

ecd9748

fmt

e322d7a

maan2003 reviewed Aug 16, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

e42a981

Co-authored-by: Manmeet Maan <[email protected]>

This comment has been minimized.

Sign in to view

cmyr approved these changes Aug 16, 2021

View reviewed changes

Update druid-shell/src/backend/mac/window.rs

8f4482a

Co-authored-by: Colin Rofls <[email protected]>

Merge branch 'master' into 1757

f850885

JAicewizard added 4 commits September 18, 2021 15:16

Add equality to all window handles

10db81d

Fix windows using rc

31a657b

fix x11 code

3a5fed7

fix web equality

625707b

jneem merged commit 53321e2 into linebender:master Sep 18, 2021

This was referenced Sep 18, 2021

Do we need WindowHandle::set_level? #1824

Closed

calling WindowDesc::set_position has no effect on GTK when run on wayland #1757

Closed

JAicewizard deleted the 1757 branch September 18, 2021 18:47

JAicewizard commented Sep 20, 2021

View reviewed changes

This was referenced Sep 21, 2021

Windows tooltip changes #1982

Merged

Tooltip broken in Windows #1986

Closed

maan2003 mentioned this pull request Oct 4, 2021

x11: relative positioning of child window #2000

Merged

maan2003 added a commit that referenced this pull request Oct 6, 2021

x11: relative positioning of child window (#2000)

1801164

implement #1919 for x11 backend

		// TODO: Maybe collin can get this into a state where modal windows follow the parent?
		// There is an API to do child windows, (https://developer.apple.com/documentation/appkit/nswindow/1419152-addchildwindow)

make most window types have a child window #1919

make most window types have a child window #1919

Conversation

JAicewizard commented Aug 14, 2021

jneem left a 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

Choose a reason for hiding this comment

JAicewizard Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAicewizard commented Aug 14, 2021 • edited Loading

JAicewizard commented Aug 14, 2021

maan2003 commented Aug 16, 2021

JAicewizard commented Aug 16, 2021

maan2003 commented Aug 16, 2021

JAicewizard commented Aug 16, 2021

This comment has been minimized.

JAicewizard commented Aug 16, 2021

cmyr left a 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

Choose a reason for hiding this comment

jneem commented Sep 16, 2021

JAicewizard commented Sep 18, 2021

CryZe commented Sep 20, 2021 • edited Loading

JAicewizard commented Sep 20, 2021 • edited Loading

CryZe commented Sep 20, 2021 • edited Loading

sjoshid commented Sep 20, 2021

Choose a reason for hiding this comment

sjoshid Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoshid Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

JAicewizard Sep 20, 2021 • edited Loading

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

JAicewizard Aug 15, 2021 •

edited

Loading

JAicewizard commented Aug 14, 2021 •

edited

Loading

CryZe commented Sep 20, 2021 •

edited

Loading

JAicewizard commented Sep 20, 2021 •

edited

Loading

CryZe commented Sep 20, 2021 •

edited

Loading

sjoshid Sep 20, 2021 •

edited

Loading

sjoshid Sep 20, 2021 •

edited

Loading

JAicewizard Sep 20, 2021 •

edited

Loading