Statically typed selectors #908

luleyleo · 2020-05-08T21:06:40Z

This changes Selector to Selector<T> where T is the selectors payload.

There was a Zulip discussion about why Commands rely on purely dynamic typing while Env uses statically typed Keys.

I found myself typing 'Must have ThisType as payload` for pretty much every selector I've defined, so adding a type to it seemed like the logical consequence.

This also fixes some issues with menu items submitting wrong commands or showing wrong labels.

This still lacks in documentation and I haven't thoroughly checked if everything works, but at least for multiwin it does work better now.

Also I have plans to introduce OneShotSelector<T> for one-shot commands and I'm not sure if it should be part of this PR or go into a separate one.

I've opened this as a draft for now so that I have a better overview of the changes made and where I have to add documentation. Also I can see if there are errors on mac or windows and if someone wants to give some feedback, that would be welcome as well.

druid/src/command.rs

cmyr

Okay, a bunch of thoughts and observations; I think the general idea is good but there's also a lot going on here, and it's hard to address it all. It might make some sense to try and break this up a bit if that's not too tricky, such as (for instance) having the one-shot changes be a separate patch?

druid/src/command.rs

cmyr · 2020-05-12T14:05:33Z

CHANGELOG.md

@@ -47,6 +47,7 @@ While some features like the clipboard, menus or file dialogs are not yet availa
 - Timer events will only be delivered to the widgets that requested them. ([#831] by [@sjoshid])
 - `Event::Wheel` now contains a `MouseEvent` structure. ([#895] by [@teddemunnik])
 - `AppDelegate::command` now receives a `Target` instead of a `&Target`. ([#909] by [@xStrom])
+- `SHOW_WINDOW` and `OPEN_WINDOW` no longer require a `WindowId` as payload, but they must `Target` the correct window. ([#???] by [@finnerale])


maybe this can just be a separate patch?

I suppose this could be split into

Updating SHOW_WINDOW and OPEN_WINDOW to use Target

Fix open and save menu items

Make Selector type safe

Split Selector into Selector and SelectorOnce

Add type-safe? methods to EventCtx to create menus and windows.

Splitting the last two out would cause quite some duplicated / wasted work tho, as SelectorOnce influences error and return types, while menus creation decides over adding wrapper types or new methods.

Even the first one means that I'll have to first declare it to take WidgetId and than remove that in a separate PR again.

yea, if things are truly entwined than doing it together makes sense, but if there are good dividing points that does make review a lot easier.

druid/examples/identity.rs

cmyr · 2020-05-12T14:24:24Z

druid/src/app.rs

+///
+/// This includes a function that can build the root widget, as well as other
+/// window properties such as the title.
+pub struct AppStateWindowDesc {


Naming: I would call this AnyWindowDesc.

Philosophy: I get that this is a problem that needs to be solved for this PR to work, but I also think it's worth thinking about on its own.

What we need:

some kind of type erasure or 'implicit T' for certain types of objects, like window descriptions and menus

not to confuse the user

to clutter the namespace as little as possible

What's the best way to do this? I'm not totally sure.

If we could have some internal type that did what this type is doing, that would be great. One way this might work is if we had convenience methods for things like making a new window or setting a menu; for instance creating a new window might just use a method on EventCtx, and then the user never needs to create the command or actually interact with the type erased type?

It might also be that we require a public type (that appears in the signature of the selector, for instance) but we don't actually need the user to ever interact with it?

I though about some kind of wrapper type but I don't think it is possible with the Rust type system.
What I wanted to make more obvious: MenuDesc and others must represent the application state.
Adding methods for this to EventCtx seems like a possible solution.
Doing so could even allow equipping EventCtx with the TypeIds of eg. MenuDesc<AppState> to check it right away and panic if the type is wrong. That way the back trace will show exactly where the mismatch happened.
But if we want to keep it as a Command I doubt we can get around some kind of specific wrapper type.

the nice thing is that on EventCtx the method could take a concrete MenuDesc.

How so? I mean EventCtx does not know what the AppState is, so how is it supposed to be typed for it?
If this could be done statically, than that would be absolutely my favorite.

mm you're right I was thinking that EventCtx was paramaterized over T, but even in that case it doesn't tell us what the root data is.

cmyr · 2020-05-12T14:29:50Z

druid/src/command.rs

-    /// The argument was expected to be reusable and wasn't, or vice-versa.
-    WrongVariant,
-    /// The argument could not be downcast to the specified type.
-    IncorrectType,


this is still possible in the case of a name collision, isn't it?

Yes, and it will panic just like Env does.

cmyr · 2020-05-12T14:30:32Z

druid/src/command.rs

+/// variant of some generic item represents the application state.
+/// Examples are `MenuDesc<T>` and `AppStateMenuDesc`.
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub struct AppStateTypeError {


do we have an 'errors' module? I might move that there, I'm not sure.

I'd also maybe be more general, and call it a DowncastError or something?

I don't think we have one, but it could be interesting to see if it would be beneficial.
As for the name, DowncastError is less specific but would be more in line with the other names.

cmyr · 2020-05-12T14:37:50Z

druid/src/command.rs

-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct Selector(&'static str);
+#[derive(Debug, PartialEq, Eq)]
+pub struct OneShotSelector<T>(SelectorSymbol, PhantomData<*const T>);


I'm not totally sure about this; what it comes down to is whether or not we expect external people to need to use this, or whether it's an implementation detail used in a few places inside druid. Again, I think that API simplicity is important, and I think the more concepts we introduce the more work we need to do to explain what they're for and how to use them. My feeling is that 95+% of the time, the user doesn't need to know about one-shot commands?

A kind of gross way we could encode one-shotness would be in the selector symbol itself, like we could have some keyword that prefaced one-shot commands...

Well the ideas I had where:

Leave it as it is.

IMO the worst choice as it makes it less obvious how a command is supposed to be used, and would require you to look up every commands doc in order to find out how the hell you are supposed to submit it and how to retrieve the object. Also Command::get and Command::take will either have to panic or require an additional error, which would add to the interface at least as much noise as a new type.

Selector(key: &str, is_once: bool) and a Selector::once constructor.

Not much better than the previous one.

Encoding it in the selector symbol.

Again, does not add anything really.

Having a separate type for it.

Allows Command::get to return Option instead of Result.

Submitting and retrieving becomes a no-brainer as the compiler checks it for you.

Could allow merging Command::new and Command::once if we want to.

No need to look up a selectors docs to know how to use it (unless it does something very special)

Those who do not care about one shot commands do not have to use this type

Which is why I very much like this design.

I wonder if another option is to encode it in the type param? Like have some type SingleUse<T>, and then you're required to explicitly take from that... I don't know if I like this, just spitballing.

cmyr · 2020-05-12T14:46:48Z

druid/src/menu.rs

+/// A platform-agnostic description of an application, window, or context
+/// menu. The user has to guarantee that this represents a `MenuDesc<T>` where `T` is the users
+/// `AppState`.
+pub struct AppStateMenuDesc {


ditto, just AnyMenuDesc if we're going this route

cmyr · 2020-05-12T14:47:26Z

druid/src/menu.rs

+}
+
+impl AppStateMenuDesc {
+    pub(crate) fn realize<T: Any>(&self) -> Result<&MenuDesc<T>, AppStateTypeError> {


don't mind this name, but I might just call it downcast or to_concrete? (ditto the other one)

downcast seems reasonable.

luleyleo · 2020-05-28T17:54:23Z

Splitting this up was a great idea, and resulted in a far better design.

Thanks a lot @cmyr and @xStrom for your help and reviews!

This was completed with #993 an can thus be closed as successful.

xStrom mentioned this pull request May 9, 2020

Optimize clippy trivially_copy_pass_by_ref for 64-bit. #909

Merged

xStrom reviewed May 11, 2020

View reviewed changes

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

luleyleo added 19 commits May 12, 2020 10:23

First step at rustifying selectors.

e308b1a

Rebase onto current master.

9eeac53

Correct 'Save...' menu item on Windows

20e19de

Fix clippy warnings.

f77794a

Fix Clone and Copy impl for Selector.

da0ed08

Update examples in comments.

778d7d9

Run cargo fmt.

d555325

Fix another doc example error.

5b60f3d

Fix trivial copy copy pass by ref.

13d4ae0

Change Selector to honor PhantomData best practice.

c2a55a2

More doc leanups.

9cdb7d8

Replace some matches with simpler ifs.

9123bb5

Go back to Any bounds instead of static.

9405582

Introduce OneShotSelector.

b6f5601

Silence pointless clippy warning.

a42d880

Default to Selector<()>.

65ca89b

Correct some typos.

55f623e

Rebase onto master.

18fa50d

Correct another typo.

c66836d

cmyr reviewed May 12, 2020

View reviewed changes

This was referenced May 14, 2020

Update CLOSE and SHOW _WINDOW commands to utilize Target. #928

Merged

Correct built-in open and save menu items. #930

Merged

Replace generic commands with methods on EventCtx and DelegateCtx #931

Merged

Remove one shot commands. #959

Merged

luleyleo mentioned this pull request May 26, 2020

Statically typed selectors, take two. #993

Merged

luleyleo closed this May 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statically typed selectors #908

Statically typed selectors #908

luleyleo commented May 8, 2020 •

edited

Loading

cmyr left a comment

cmyr May 12, 2020

luleyleo May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020

cmyr May 12, 2020

cmyr May 12, 2020

cmyr May 12, 2020

luleyleo May 12, 2020 •

edited

Loading

luleyleo commented May 28, 2020

Statically typed selectors #908

Statically typed selectors #908

Conversation

luleyleo commented May 8, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luleyleo May 12, 2020 • edited Loading

Choose a reason for hiding this comment

luleyleo commented May 28, 2020

luleyleo commented May 8, 2020 •

edited

Loading

luleyleo May 12, 2020 •

edited

Loading