Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statically typed selectors #908

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this can just be a separate patch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Deprecated

Expand Down
25 changes: 10 additions & 15 deletions druid/examples/blocking_function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ use druid::{

use druid::widget::{Button, Either, Flex, Label};

const START_SLOW_FUNCTION: Selector = Selector::new("start_slow_function");
const START_SLOW_FUNCTION: Selector<u32> = Selector::new("start_slow_function");

const FINISH_SLOW_FUNCTION: Selector = Selector::new("finish_slow_function");
const FINISH_SLOW_FUNCTION: Selector<u32> = Selector::new("finish_slow_function");

struct Delegate {
eventsink: ExtEventSink,
Expand Down Expand Up @@ -61,20 +61,15 @@ impl AppDelegate<AppState> for Delegate {
data: &mut AppState,
_env: &Env,
) -> bool {
match cmd.selector {
START_SLOW_FUNCTION => {
data.processing = true;
wrapped_slow_function(self.eventsink.clone(), data.value);
true
}
FINISH_SLOW_FUNCTION => {
data.processing = false;
let number = cmd.get_object::<u32>().expect("api violation");
data.value = *number;
true
}
_ => true,
if cmd.is(START_SLOW_FUNCTION) {
data.processing = true;
wrapped_slow_function(self.eventsink.clone(), data.value);
}
if let Some(number) = cmd.get(FINISH_SLOW_FUNCTION) {
data.processing = false;
data.value = *number;
}
true
}
}

Expand Down
6 changes: 3 additions & 3 deletions druid/examples/ext_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use druid::kurbo::RoundedRect;
use druid::widget::prelude::*;
use druid::{AppLauncher, Color, Data, LocalizedString, Rect, Selector, WidgetExt, WindowDesc};

const SET_COLOR: Selector = Selector::new("event-example.set-color");
const SET_COLOR: Selector<Color> = Selector::new("event-example.set-color");

/// A widget that displays a color.
struct ColorWell;
Expand Down Expand Up @@ -53,8 +53,8 @@ impl ColorWell {
impl Widget<MyColor> for ColorWell {
fn event(&mut self, ctx: &mut EventCtx, event: &Event, data: &mut MyColor, _env: &Env) {
match event {
Event::Command(cmd) if cmd.selector == SET_COLOR => {
data.0 = cmd.get_object::<Color>().unwrap().clone();
Event::Command(cmd) if cmd.is(SET_COLOR) => {
data.0 = cmd.get(SET_COLOR).unwrap().clone();
ctx.request_paint();
}
_ => (),
Expand Down
13 changes: 4 additions & 9 deletions druid/examples/identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use druid::{

const CYCLE_DURATION: Duration = Duration::from_millis(100);

const FREEZE_COLOR: Selector = Selector::new("identity-example.freeze-color");
const FREEZE_COLOR: Selector<Color> = Selector::new("identity-example.freeze-color");
const UNFREEZE_COLOR: Selector = Selector::new("identity-example.unfreeze-color");

/// Honestly: it's just a color in fancy clothing.
Expand Down Expand Up @@ -114,15 +114,10 @@ impl Widget<OurData> for ColorWell {
self.token = ctx.request_timer(CYCLE_DURATION);
}

Event::Command(cmd) if cmd.selector == FREEZE_COLOR => {
self.frozen = cmd
.get_object::<Color>()
.ok()
.cloned()
.expect("payload is always a Color")
.into();
Event::Command(cmd) if cmd.is(FREEZE_COLOR) => {
self.frozen = cmd.get(FREEZE_COLOR).cloned();
}
Event::Command(cmd) if cmd.selector == UNFREEZE_COLOR => self.frozen = None,
Event::Command(cmd) if cmd.is(UNFREEZE_COLOR) => self.frozen = None,
_ => (),
}
}
Expand Down
35 changes: 20 additions & 15 deletions druid/examples/multiwin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use druid::{

use log::info;

const MENU_COUNT_ACTION: Selector = Selector::new("menu-count-action");
const MENU_COUNT_ACTION: Selector<usize> = Selector::new("menu-count-action");
const MENU_INCREMENT_ACTION: Selector = Selector::new("menu-increment-action");
const MENU_DECREMENT_ACTION: Selector = Selector::new("menu-decrement-action");
const MENU_SWITCH_GLOW_ACTION: Selector = Selector::new("menu-switch-glow");
Expand Down Expand Up @@ -56,7 +56,7 @@ trait EventCtxExt {

impl EventCtxExt for EventCtx<'_> {
fn set_menu<T: 'static>(&mut self, menu: MenuDesc<T>) {
let cmd = Command::new(druid::commands::SET_MENU, menu);
let cmd = Command::new(druid::commands::SET_MENU, menu.into_app_state_menu_desc());
let target = self.window_id();
self.submit_command(cmd, target);
}
Expand Down Expand Up @@ -155,7 +155,10 @@ impl AppDelegate<State> for Delegate {
match event {
Event::MouseDown(ref mouse) if mouse.button.is_right() => {
let menu = ContextMenu::new(make_context_menu::<State>(), mouse.pos);
let cmd = Command::new(druid::commands::SHOW_CONTEXT_MENU, menu);
let cmd = Command::new(
druid::commands::SHOW_CONTEXT_MENU,
menu.into_app_state_context_menu(),
);
ctx.submit_command(cmd, Target::Window(window_id));
None
}
Expand All @@ -171,39 +174,40 @@ impl AppDelegate<State> for Delegate {
data: &mut State,
_env: &Env,
) -> bool {
match (target, &cmd.selector) {
(_, &sys_cmds::NEW_FILE) => {
let new_win = WindowDesc::new(ui_builder)
match target {
_ if cmd.is(sys_cmds::NEW_FILE) => {
let new_win = WindowDesc::<State>::new(ui_builder)
.menu(make_menu(data))
.window_size((data.selected as f64 * 100.0 + 300.0, 500.0));
let command = Command::one_shot(sys_cmds::NEW_WINDOW, new_win);
let command =
Command::one_shot(sys_cmds::NEW_WINDOW, new_win.into_app_state_menu_desc());
ctx.submit_command(command, Target::Global);
false
}
(Target::Window(id), &MENU_COUNT_ACTION) => {
data.selected = *cmd.get_object().unwrap();
Target::Window(id) if cmd.is(MENU_COUNT_ACTION) => {
data.selected = *cmd.get(MENU_COUNT_ACTION).unwrap();
let menu = make_menu::<State>(data);
let cmd = Command::new(druid::commands::SET_MENU, menu);
let cmd = Command::new(sys_cmds::SET_MENU, menu.into_app_state_menu_desc());
ctx.submit_command(cmd, id);
false
}
// wouldn't it be nice if a menu (like a button) could just mutate state
// directly if desired?
(Target::Window(id), &MENU_INCREMENT_ACTION) => {
Target::Window(id) if cmd.is(MENU_INCREMENT_ACTION) => {
data.menu_count += 1;
let menu = make_menu::<State>(data);
let cmd = Command::new(druid::commands::SET_MENU, menu);
let cmd = Command::new(sys_cmds::SET_MENU, menu.into_app_state_menu_desc());
ctx.submit_command(cmd, id);
false
}
(Target::Window(id), &MENU_DECREMENT_ACTION) => {
Target::Window(id) if cmd.is(MENU_DECREMENT_ACTION) => {
data.menu_count = data.menu_count.saturating_sub(1);
let menu = make_menu::<State>(data);
let cmd = Command::new(druid::commands::SET_MENU, menu);
let cmd = Command::new(sys_cmds::SET_MENU, menu.into_app_state_menu_desc());
ctx.submit_command(cmd, id);
false
}
(_, &MENU_SWITCH_GLOW_ACTION) => {
_ if cmd.is(MENU_SWITCH_GLOW_ACTION) => {
data.glow_hot = !data.glow_hot;
false
}
Expand All @@ -220,6 +224,7 @@ impl AppDelegate<State> for Delegate {
) {
info!("Window added, id: {:?}", id);
}

fn window_removed(
&mut self,
id: WindowId,
Expand Down
38 changes: 16 additions & 22 deletions druid/examples/open_save.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

use druid::widget::{Align, Button, Flex, TextBox};
use druid::{
AppDelegate, AppLauncher, Command, DelegateCtx, Env, FileDialogOptions, FileInfo, FileSpec,
commands, AppDelegate, AppLauncher, Command, DelegateCtx, Env, FileDialogOptions, FileSpec,
LocalizedString, Target, Widget, WindowDesc,
};

Expand Down Expand Up @@ -77,30 +77,24 @@ impl AppDelegate<String> for Delegate {
data: &mut String,
_env: &Env,
) -> bool {
match cmd.selector {
druid::commands::SAVE_FILE => {
if let Ok(file_info) = cmd.get_object::<FileInfo>() {
if let Err(e) = std::fs::write(file_info.path(), &data[..]) {
println!("Error writing file: {}", e);
}
}
true
if let Some(Some(file_info)) = cmd.get(commands::SAVE_FILE) {
if let Err(e) = std::fs::write(file_info.path(), &data[..]) {
println!("Error writing file: {}", e);
}
druid::commands::OPEN_FILE => {
if let Ok(file_info) = cmd.get_object::<FileInfo>() {
match std::fs::read_to_string(file_info.path()) {
Ok(s) => {
let first_line = s.lines().next().unwrap_or("");
*data = first_line.to_owned();
}
Err(e) => {
println!("Error opening file: {}", e);
}
}
return true;
}
if let Some(file_info) = cmd.get(commands::OPEN_FILE) {
match std::fs::read_to_string(file_info.path()) {
Ok(s) => {
let first_line = s.lines().next().unwrap_or("");
*data = first_line.to_owned();
}
Err(e) => {
println!("Error opening file: {}", e);
}
true
}
_ => false,
return true;
}
false
}
}
41 changes: 40 additions & 1 deletion druid/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ use crate::widget::LabelText;
use crate::win_handler::{AppHandler, AppState};
use crate::window::WindowId;
use crate::{
theme, AppDelegate, Data, DruidHandler, Env, LocalizedString, MenuDesc, Widget, WidgetExt,
command::AppStateTypeError, theme, AppDelegate, Data, DruidHandler, Env, LocalizedString,
MenuDesc, Widget, WidgetExt,
};
use std::any::{self, Any};

/// A function that modifies the initial environment.
type EnvSetupFn<T> = dyn FnOnce(&mut Env, &T);
Expand Down Expand Up @@ -54,6 +56,43 @@ pub struct WindowDesc<T> {
pub id: WindowId,
}

/// A description of a window to be instantiated. The user has to guarantee that this
/// represents a `WindowDesc<T>` where `T` is the users `AppState`.
///
/// This includes a function that can build the root widget, as well as other
/// window properties such as the title.
pub struct AppStateWindowDesc {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inner: Box<dyn Any>,
type_name: &'static str,
}

impl<T: Any> WindowDesc<T> {
/// This turns a typed `WindowDesc<T>` into an untyped `AppStateWindowDesc`.
/// Doing so allows sending `WindowDesc` through `Command`s.
/// It is up to you, to ensure that this `T` represents your application
/// state that you passed to `AppLauncher::launch`.
pub fn into_app_state_menu_desc(self) -> AppStateWindowDesc {
AppStateWindowDesc {
inner: Box::new(self),
type_name: any::type_name::<Self>(),
}
}
}

impl AppStateWindowDesc {
pub(crate) fn realize<T: Any>(self) -> Result<WindowDesc<T>, AppStateTypeError> {
let inner: Result<Box<WindowDesc<T>>, _> = self.inner.downcast();
if let Ok(inner) = inner {
Ok(*inner)
} else {
Err(AppStateTypeError::new(
any::type_name::<WindowDesc<T>>(),
self.type_name,
))
}
}
}

impl<T: Data> AppLauncher<T> {
/// Create a new `AppLauncher` with the provided window.
pub fn with_window(window: WindowDesc<T>) -> Self {
Expand Down
Loading