Sub windows (squashed from previous branch) #1254

rjwittams · 2020-09-25T13:36:02Z

Please read this description before going through the diff.

This was the first cut of sub windows that I did in a few days at the beginning of August, not really updated significantly since then, but it has taken this long to get to the point of making a PR due to dependent things and general delays.

This was not supposed to be fully finished or finalised, more of a proof of concept or a starting point that is expected to be expanded upon, because I thought it needed actual discussion before really doing a lot with it (making 'proper' tooltips or combo boxes, etc), or wiring through new events etc if that was the route people wanted to go. I did not think it would take quite so long to get to the point of being able to discuss it.

The general idea is that we make a window with a SubWindowHost in it via our surrounding widget pod, and then that widget pod synchronises its app data with the SubWindowHost via commands. It could be special events instead, and I'm unconvinced that the command stuff in here is exactly the right way to do it (specifically the command handling is all done just before the widget pods child gets it, as I didn't want to interfere with the targeted command unwrapping stuff (that I think could be unwrapping this command?). It might be fine, but it does look a bit different to all the other event handling in WidgetPod).

This whole mechanism is clearly driven by the 'focusing'/ lenses/app data model of current druid and its limitations. As that might be abandoned, possibly people do not want to spend time thinking about it.

It is likely to be far more productive to clone this, play with the sub_window.rs example and start from there than to read the diff from start to finish. The example has a couple of things in it - that once again, are not supposed to be fully finished or finalised in any way whatsoever:

Tooltips that can be tried out by mousing over the "Hello World" label.
Sub windows without titlebars that synchronise state with the main window and each other (change the text in the textboxes)
A draggable area on the sub window that is implemented in a very not final way (by setting screen position based on 'within window' mouse deltas if I remember correctly - its 2 months ago that I actually did this). It somehow works seemingly fine on the mac, but is horribly jerky on gtk in a vm. I reiterate, this is an example in order to test how the sub window mechanism works and discover its current limitations.

Things that I've not tackled:

Currently it is possible to set up a sub window with a 'wrong' widget pod - where you have a lens between the creation point and the pod. This could be detected by threading through the type id of data into the widget pod. (This might not be entirely correct, as I said, 2 months since I did this.) Right now it will give an error when synchronisation doesn't work, due to a dynamic cast out of the command not working. So its a late warning that something has gone wrong. I don't think it is possible to detect this is wrong at compile time (without the context having a type level idea of the surrounding pods Data type, or making lenswrap have a pod in it).
Maybe it should be sending the environment around as well (optionally)? Right now it will use the environment from the top level app state. I guess this could be surprising if someone was doing some window level themeing in their app or something . I haven't looked into if this would be expensive or annoying.

I haven't tested any of this on windows as I don't have it at the moment.

Maybe this can serve as a starting point or idea for someone else later on if it isn't what people want to spend time on at the moment.

druid/src/core.rs

jneem · 2020-09-26T03:24:56Z

Have you tried restricting subwindows so that the data binding stuff requires a special widget to work? Like, instead of allowing every WidgetPod to sync its data with subwindows, add a new SubWindowMaker widget for doing that. This would fix the "wrong Data type" problem, AFAICT.

It would also be much less flexible, of course, but it would still cover the main use-cases I can think of. For tooltips, you need a Controller to listen for the mouse hover anyway, so you could add a SubWindowMaker in there. Things like dropdowns would be composite widgets containing their own SubWindowMaker.

rjwittams · 2020-09-26T08:49:06Z

Have you tried restricting subwindows so that the data binding stuff requires a special widget to work? Like, instead of allowing every WidgetPod to sync its data with subwindows, add a new SubWindowMaker widget for doing that. This would fix the "wrong Data type" problem, AFAICT.

It would also be much less flexible, of course, but it would still cover the main use-cases I can think of. For tooltips, you need a Controller to listen for the mouse hover anyway, so you could add a SubWindowMaker in there. Things like dropdowns would be composite widgets containing their own SubWindowMaker.

That’s what I had initially. It was extremely odd in actual use.

rjwittams · 2020-09-26T15:45:01Z

To expand on this:

its effectively a child widget, but doesn't layout or paint.
all the users of it end up having to learn a new protocol of exactly how to interact with it in event and update. That basically would be copied around to each user along with any bugs and any changes to this protocol entail changing a complex interaction that wouldn't be easy to force to be correct via the type system, rather than just changing WidgetPod
its a completely different api style than making a new window or context menu.

cmyr · 2020-09-27T13:38:47Z

Will just chime in and say I haven't looked at the code much but playing around with the example it looks promising!

rjwittams · 2020-09-27T15:49:57Z

Other things I forgot to mention:

Could automatically close windows when the parent pod gets dropped. Not sure if that is the right behaviour though... things like view switcher make the widget lifecycle a bit ephemeral. However any subwindow left hanging around wouldn't be able to sync anymore. This points a bit at the 'path from the root' being implicit in the widget tree traversal (ie lensing) being a bit of an awkward fit here (that is what we are getting around). Probably best to close the sub windows for now.
It is possible to imagine using the same 'state teleporting' for non sub window things. Vue for example has added a general mechanism for this. I think going into it would derail things
I don't remember why I had syncing and non syncing subwindows, possibly a pointless option.

rjwittams · 2020-11-24T19:44:17Z

Is anyone interested in this? Its been 2 months.

cmyr · 2020-11-25T21:11:41Z

I'm very interested in this feature but I've just been too busy to really give it much concerted thought. I'll try and give this PR a closer look in the next day or two. Thanks for the bump!

cmyr

Overall I think this is a pretty clean solution. A few comments inline mostly related to API and what types exactly we will need to expose, but that's just a polish question.

A few other things:

before we merge we'll need to get docs and related up to the quality of the rest of the public API; I've avoided nitpicking any of that since it feels like this is very much a first draft.
sub windows should close when a parent window closes. The easiest thing to do here might be to just keep a map at the application root that tracks each window's subwindows, and we can check that when closing a window to ensure we also close its children?
this is also demonstrating the need for some mechanism that allows a window to be sized based on the size of its child. That is outside of the scope of this PR, but it's something we will need in order for this to become properly useful. As a hack we might be able to bake this into the SubWindowHost?
we're also really going to need to start having focus state take window focus into account; with this I can get multiple windows on screen, each with a focused text box and a blinking cursor. Also out of scope for this particular PR, but want to at least make note.

In any case I think this has a lot of promise and I'd be happy to see it get merged if you're still interested. Thanks!

druid/src/widget/sub_window.rs

druid/src/contexts.rs

rjwittams · 2020-12-04T12:20:56Z

I'll have to remind myself how all of this works. I do vaguely remember there being some issues with ordering of generating ids etc. Agree on the closing windows etc, I think the host can manage it without any extra data structures if the 'non syncing' case is removed. I can't remember if it was actually needed.

You are right that this was not meant to be fully finished, because there is no point polishing things before the general approach is agreed to be sensible. Getting that agreement or rejection shouldn't take months on features that are unavoidably required.

…w-squash # Conflicts: # CHANGELOG.md # druid/src/contexts.rs # druid/src/core.rs # druid/src/lib.rs # druid/src/win_handler.rs

rjwittams · 2021-01-04T00:36:20Z

I've updated it a bit. The main thing is the window closing. It does feel surprising that we don't have a window closed lifecycle that flows down to widgets, but I can see that might be complex to get right (e.g is it allowed to cancel the window being closed? Or delay it for an animation...)

cmyr · 2021-01-04T19:10:31Z

Cool, I'll take a look at this shortly. Window closed is a tricky one; there is a method called on the app delegate when a window is added or removed, but there's no ability to cancel that action. We could think about whether we need to send some event down to the widgets as well, but that's definitely tricky... do we send it before the window dies? afterwards? What are widgets supposed to do here?

The thinking at the time was that if you need to handle this logic then you should just use an AppDelegate, and do what you need to do there.

cmyr · 2021-01-04T20:16:01Z

@rjwittams is this ready for review? Or is there an open question about the window closing?

rjwittams · 2021-01-04T23:32:27Z

I'll try to do something for the window closing soon on this branch. But it should be fairly independent of what is here now I think... so up to you. I'll un draft it at that point

Add WindowDisconnected event. Close sub windows when their owning WidgetPod gets a WindowDisconnected event.

…k on mac.

cmyr

This looks very promising. A handful of little things inline, but overall I think this is very close.

Some higher-level things:

I find the example confusing: why am I seeing display resolutions? where do I click? what does what? I think longer term we'll want something a bit more intuitive, here.

related: when you show a tool-tip, the tool-tip window gets focus (on macOS). We'll need to figure this out when we try to actually implement tooltips.

In any case I'm not super worried about that; most of the major use-cases for this (tooltips, combo boxes, etc) are things where we will want to build the implementations ourselves on top of the sub-window machinery, and we can figure out an API for that, then.

There are some things we'll need to figure out: how do we correctly size the window to fit the text for a combo box or a tooltip? I have some ideas here, but we'll have to experiment. Ditto with positioning of the window, which is particularly important in the case of a combo box.

I suspect there may be a few other things that will shake out when we try to build on top of this, but I'm happy for that to be future work.

Thanks for finishing this up!

druid/examples/sub_window.rs

druid/src/event.rs

druid/src/sub_window.rs

druid/src/command.rs

druid/src/contexts.rs

druid/src/sub_window.rs

rjwittams · 2021-01-06T16:08:34Z

I find the example confusing: why am I seeing display resolutions? where do I click? what does what? I think longer term we'll want something a bit more intuitive, here.

This was because back in the mists of time this was linked up with the screen PR. I was also using this example to test mac monitor discovery.

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

cmyr · 2021-01-06T16:19:20Z

yea I can see that it worked for testing, and I think it's fine for now, it's just not a great example in the long-term. We can open an issue to revisit that when we merge this.

cmyr · 2021-01-06T16:20:07Z

And: everytime I try to edit docs through the github suggestions feature something breaks 😒

…w-squash

rjwittams · 2021-01-06T17:02:30Z

And: everytime I try to edit docs through the github suggestions feature something breaks 😒

Docs are v fiddly

cmyr

Okay a few more small notes but this looks basically there.

Do you have any planned next steps for this? There are some obvious things this opens up; a major one for me would be doing combo boxes. I'm happy to take that on myself, but don't want to step on any toes..

druid/src/contexts.rs

cmyr · 2021-01-07T00:37:10Z

druid/src/core.rs

        let prev_env = self.env.as_ref().filter(|p| !p.same(env));
+        let env_changed = prev_env.is_some();


there's an env_changed method available on UpdateCtx.

cmyr · 2021-01-07T00:41:41Z

druid/src/core.rs

        let prev_env = self.env.as_ref().filter(|p| !p.same(env));
+        let env_changed = prev_env.is_some();
+        let data_changed = self.old_data.as_ref().filter(|p| !p.same(data)).is_some();


won't this be incorrect in the case where there is no old data?

For some reason I thought that couldn't happen but not sure why...

cmyr · 2021-01-07T00:44:15Z

druid/src/event.rs

+    /// that the window *will* close just because this event is received; for instance, you should
+    /// avoid destructive side effects such as cleaning up resources.
+    ///
+    /// [`set_handled`]: struct.EventCtx.html#method.set_handled


we should now be able to use the intra-doc-link syntax, like:

Suggested change

/// [`set_handled`]: struct.EventCtx.html#method.set_handled

/// [`set_handled`]: crate::EventCtx::set_handled

cmyr

Okay, looks good.

Do you have any planned followup, for this? Are there any issues we should open, for instance? (maybe one about improving the example?)

rjwittams · 2021-01-07T14:16:37Z

I was planning to try out a few things.

Combo boxes/drop downs (need to look at html/gtk/etc to see whats expected... ). I think theres some common helper bits for slidey out bits, eg date pickers and colour pickers share parts of that too.

Draggy droppy tabs. Multiwindow docking layout (IDE like).

I was going to do these out of tree (nursery I guess), until they are a fait accompli to go in. I find the current model of maturing "large" (>100 lines?) code drops (reviewers programming mediated by PR comments and the original developer) really extremely unproductive. I think people just following up with their own commits (especially naming and style, doc fixes, personal taste) works a lot better. I'm not saying review has no merits but the cost of it as practiced on the PRs I've currently done is maybe 15 or 20x the effort of actual development. I think maybe this works with a swarm of people inside a large tech firm. Not so sure with the current resource levels and lack of 'deadline + goal pressure' (that kind of forces acceptance of 'good enough' and incremental improvement).

Obviously in order for this to work, some core changes may always be needed and I think it also drives druid itself to be more extensible from the outside if core widgets could be moved outside and still work. There may be some churn (e.g duplicating a widget to nursery to work on it then promote the changed one back when rematured).

Sub windows (squashed from previous branch)

368f3a4

jneem reviewed Sep 26, 2020

View reviewed changes

druid/src/core.rs Outdated Show resolved Hide resolved

cmyr reviewed Dec 3, 2020

View reviewed changes

druid/src/widget/sub_window.rs Outdated Show resolved Hide resolved

druid/src/contexts.rs Outdated Show resolved Hide resolved

rjwittams added 2 commits January 3, 2021 23:13

Merge remote-tracking branch 'remotes/upstream/master' into sub-windo…

50a32c4

…w-squash # Conflicts: # CHANGELOG.md # druid/src/contexts.rs # druid/src/core.rs # druid/src/lib.rs # druid/src/win_handler.rs

Update druid version. Address review comments

28b0617

Fix to mac window close handling.

d962c74

Add WindowDisconnected event. Close sub windows when their owning WidgetPod gets a WindowDisconnected event.

rjwittams added the S-needs-review waits for review label Jan 5, 2021

rjwittams marked this pull request as ready for review January 5, 2021 17:48

rjwittams requested a review from cmyr January 5, 2021 17:48

Add WindowCloseRequested event and make window close cancellation wor…

510032b

…k on mac.

rjwittams mentioned this pull request Jan 5, 2021

Implement close request interception #1159

Closed

3 tasks

cmyr reviewed Jan 6, 2021

View reviewed changes

rjwittams and others added 2 commits January 6, 2021 16:09

Update druid/src/event.rs

ae8732b

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

Update druid/src/event.rs

f3793f2

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

Merge remote-tracking branch 'remotes/upstream/master' into sub-windo…

8884240

…w-squash

rjwittams added 2 commits January 6, 2021 18:49

Changes to allow env updates to flow down to sub windows

913261e

Doc fix

d5b3942

rjwittams requested a review from cmyr January 6, 2021 20:01

cmyr reviewed Jan 7, 2021

View reviewed changes

Review fixes

e4a1b17

cmyr approved these changes Jan 7, 2021

View reviewed changes

cmyr merged commit 2ff6318 into linebender:master Jan 7, 2021

maan2003 removed the S-needs-review waits for review label May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sub windows (squashed from previous branch) #1254

Sub windows (squashed from previous branch) #1254

rjwittams commented Sep 25, 2020

jneem commented Sep 26, 2020

rjwittams commented Sep 26, 2020 •

edited

Loading

rjwittams commented Sep 26, 2020 •

edited

Loading

cmyr commented Sep 27, 2020

rjwittams commented Sep 27, 2020

rjwittams commented Nov 24, 2020

cmyr commented Nov 25, 2020

cmyr left a comment

rjwittams commented Dec 4, 2020

rjwittams commented Jan 4, 2021

cmyr commented Jan 4, 2021

cmyr commented Jan 4, 2021

rjwittams commented Jan 4, 2021

cmyr left a comment

rjwittams commented Jan 6, 2021

cmyr commented Jan 6, 2021

cmyr commented Jan 6, 2021

rjwittams commented Jan 6, 2021

cmyr left a comment

cmyr Jan 7, 2021

cmyr Jan 7, 2021

rjwittams Jan 7, 2021

cmyr Jan 7, 2021

cmyr left a comment

rjwittams commented Jan 7, 2021

		let prev_env = self.env.as_ref().filter(\|p\| !p.same(env));
		let env_changed = prev_env.is_some();

	/// [`set_handled`]: struct.EventCtx.html#method.set_handled
	/// [`set_handled`]: crate::EventCtx::set_handled

Sub windows (squashed from previous branch) #1254

Sub windows (squashed from previous branch) #1254

Conversation

rjwittams commented Sep 25, 2020

jneem commented Sep 26, 2020

rjwittams commented Sep 26, 2020 • edited Loading

rjwittams commented Sep 26, 2020 • edited Loading

cmyr commented Sep 27, 2020

rjwittams commented Sep 27, 2020

rjwittams commented Nov 24, 2020

cmyr commented Nov 25, 2020

cmyr left a comment

Choose a reason for hiding this comment

rjwittams commented Dec 4, 2020

rjwittams commented Jan 4, 2021

cmyr commented Jan 4, 2021

cmyr commented Jan 4, 2021

rjwittams commented Jan 4, 2021

cmyr left a comment

Choose a reason for hiding this comment

rjwittams commented Jan 6, 2021

cmyr commented Jan 6, 2021

cmyr commented Jan 6, 2021

rjwittams commented Jan 6, 2021

cmyr left a comment

Choose a reason for hiding this comment

cmyr Jan 7, 2021

Choose a reason for hiding this comment

cmyr Jan 7, 2021

Choose a reason for hiding this comment

rjwittams Jan 7, 2021

Choose a reason for hiding this comment

cmyr Jan 7, 2021

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

rjwittams commented Jan 7, 2021

rjwittams commented Sep 26, 2020 •

edited

Loading

rjwittams commented Sep 26, 2020 •

edited

Loading