multiple windows, round two #149

cmyr · 2019-09-12T16:17:28Z

This is my attempt to get very basic multi-win support working.

This is simpler and less flexible than the original approach in #127. My thinking is that we should do as little as possible, and then figure out what missing features we will need for runebender.

This will require some follow-up work; in particular I want to add a NewWindow event that announces a new window, and passes along the window id so that other windows can send that window messages.

I'm not especially confident in this design; it's hard to really anticipate problems until I've used it more, but I hope it's a reasonable starting point.

I'm not totally sure I have the event/update logic correct.

In any case, I want to get this up now as a checkpoint. I'm going to start trying to weld menus onto this, and we'll see how that goes? If I'm having a lot of trouble I may need to reconsider some things here. 🤷‍♂

This is a squash of a bunch of wip commits: Starting on multi-window Most flow is in place, but not wired to actual window creation yet. Fix lifetime issues These only show up on Windows (DirectWrite backend). Wire window size propagation Events on window size need to be propagated to the WindowPod so they can be available during layout. There are other ways to do this. Instead of being in WindowPod, maybe the size info should be in WindowState (where prev_paint_time is, for similar reasons), then passed down through LayoutCtxRoot. But this way, at least the RootWidget gets notifications of window size changing, which seems potentially useful. Small progress towards working multi-window Get new window containers working Build the hello example using containers suitable for multi-window. Still WIP, there's no way to create new windows yet. Starting to plumb creation of multiple windows This is basically working but has a few rough spots. One missing feature is an event flow for closing windows, so their state can be cleaned up.

This removes WindowPod and bakes in the simplest case.

This might be leaking memory, but that beats a crash in the near-term.

raphlinus

I tested a bit on Windows as well (but see comment inline about key event matching which I had to bodge). The good news is that it seems pretty robust; I wasn't able to trigger any crashes. But there are signs it isn't quite right yet, I get this logged:

2019-09-14 13:52:18,273 ERROR [druid_shell::windows] dropped message 0x2, hwnd=0x220034, wparam=0x0, lparam=0x0

I did a modest (not incredibly deep) pass through the rest and it seems pretty good to me. I like that it's simpler than my PR (exposing less of the internal event dispatching) while also adding the Command abstraction. Basically I'm comfortable with this going in soon, seems like the major issue is stability around window lifetimes. Do we want to debug and fix that in this PR, or get the API in place and treat it as a separate bug?

raphlinus · 2019-09-13T00:26:02Z

druid-shell/src/mac/mod.rs

+                //but if we don't set this we segfault after close. Maybe we should
+                //have an appdelegate that handles the windowShouldClose notification?
+                //See https://developer.apple.com/documentation/appkit/nswindow/1419062-releasedwhenclosed?language=objc
+                msg_send![window, setReleasedWhenClosed: NO];


Yeah, this stuff is really messy. Does this fix the segfault when the window is closed with the radio buttons. Ideally I'd like to solve this in a systematic way, but am willing to accept some leaking just to get things working for now.

this doesn't fix that segfault, which I haven't tried tracking down yet. It does fix a segfault that was happening when we called close() ourselves, though.

raphlinus · 2019-09-13T00:30:04Z

druid-shell/src/windows/util.rs

@@ -224,6 +230,28 @@ pub fn init() {
            func(PROCESS_SYSTEM_DPI_AWARE); // TODO: per monitor (much harder)
        }
    }
+
+    unsafe {
+        let class_name = CLASS_NAME.to_wide();


Orthogonal to the main point of this PR, but I think I'd like to converge on the wio wide trait instead of having our own. Note that we need the to_wide_null variant (the default doesn't null-terminate).

raphlinus · 2019-09-13T00:33:31Z

examples/multiwin.rs

+        data: &mut T,
+        env: &Env,
+    ) -> Option<Action> {
+        if !ctx.has_focus() {


If this becomes a standard druid widget, then I think always grabbing focus shouldn't be the behavior. I think the closure can actually do this if it likes.

raphlinus · 2019-09-14T20:51:24Z

examples/multiwin.rs

+
+    EventInterceptor::new(wrapper, |event, ctx, _data, _env| {
+        if let Event::KeyDown(e) = event {
+            if HotKey::new(SysMods::Cmd, "n").matches(e) {


This doesn't work on Windows. The reason is that the event text is \u{000e} (ie ASCII ctrl-N). This is orthogonal to multiple windows but gets in the way of testing. We should make sure to track this issue; it's quite tricky to get right in detail. In any case, I've locally modified my copy.

Hmm, that's confusing, I guess I don't know enough about how WM_CHAR and WM_KEYUP work.

raphlinus · 2019-09-14T21:41:31Z

src/command.rs

+#[derive(Debug, Clone)]
+pub struct Command {
+    pub selector: Selector,
+    pub object: Option<Arc<dyn Any>>,


Discussion question: is the option dynamic or static (ie some selectors just don't need arguments)? In the latter case, it might be a convention to use () as the argument. On the other hand, that's maybe less efficient, as it would still need to be boxed...

My intention was that the expected type of the argument is determined by the selector. Also I'm not sure if this needs to be pub; it should only be accessed through get_object, I think..

src/command.rs

src/lib.rs

src/win_handler.rs

cmyr · 2019-09-16T21:08:07Z

Oops didn't notice this review come in, thanks!

There are definitely some crashes to be teased out, by my inclination here is to merge and continue from there, as long as we think this is a reasonable general approach.

raphlinus · 2019-09-16T22:23:39Z

Yeah, I'm fine with that. I'll mark this as approved.

raphlinus

Looks good, thanks!

- Remove PartialEq for Command/Selector - remove `pub` from Selector.object - rename send_ to do_ in win_handler - cleanup some comments & remove some dead code

cmyr force-pushed the multi_win2 branch 3 times, most recently from 4b7a73f to 1df5997 Compare September 13, 2019 12:42

raphlinus and others added 9 commits September 13, 2019 13:45

Add command.rs and basic Command & Selector types

2f22a3b

Simplify multi-win

05c9743

This removes WindowPod and bakes in the simplest case.

[mac] Fix crash when closing window

82b7dd9

This might be leaking memory, but that beats a crash in the near-term.

[wip] Wire up Command-based window management

c212cbc

Update all windows after event

5388caf

Docs and cleanup

1463276

Remove UiState & UiMain

9caed0e

Cleanup & unify window creation code

1c061b2

cmyr force-pushed the multi_win2 branch from 0b2496a to 1c061b2 Compare September 13, 2019 18:05

raphlinus reviewed Sep 14, 2019

View reviewed changes

cmyr force-pushed the multi_win2 branch from 250b95e to 41b1739 Compare September 16, 2019 21:06

raphlinus approved these changes Sep 16, 2019

View reviewed changes

cmyr force-pushed the multi_win2 branch from 41b1739 to 1c2c902 Compare September 17, 2019 14:07

cmyr mentioned this pull request Sep 17, 2019

Windows backend reports incorrect text for ctrl+key #158

Open

raphlinus approved these changes Sep 17, 2019

View reviewed changes

Review feedback

bce4c2f

- Remove PartialEq for Command/Selector - remove `pub` from Selector.object - rename send_ to do_ in win_handler - cleanup some comments & remove some dead code

cmyr force-pushed the multi_win2 branch from 1c2c902 to bce4c2f Compare September 17, 2019 18:14

cmyr merged commit a92f85f into master Sep 17, 2019

This was referenced Sep 17, 2019

dropped message when closing window on windows #160

Open

First draft of multiple window support #127

Closed

multiple window support #100

Closed

cmyr deleted the multi_win2 branch November 18, 2019 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiple windows, round two #149

multiple windows, round two #149

cmyr commented Sep 12, 2019

raphlinus left a comment

raphlinus Sep 13, 2019

cmyr Sep 16, 2019

raphlinus Sep 13, 2019

raphlinus Sep 13, 2019

raphlinus Sep 14, 2019

cmyr Sep 16, 2019

raphlinus Sep 14, 2019

cmyr Sep 16, 2019

cmyr commented Sep 16, 2019

raphlinus commented Sep 16, 2019

raphlinus left a comment

multiple windows, round two #149

multiple windows, round two #149

Conversation

cmyr commented Sep 12, 2019

raphlinus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr commented Sep 16, 2019

raphlinus commented Sep 16, 2019

raphlinus left a comment

Choose a reason for hiding this comment