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

Remove one shot commands. #959

Merged
merged 7 commits into from
May 18, 2020

Conversation

luleyleo
Copy link
Collaborator

Fourth part of #908 .

I gave the suggestion by @cmyr a try and replaced one shot commands with a SingleUse wrapper type and it seems to work really well.

To summarize the change:

ctx.submit_command(
    Command::one_shot(commands::NEW_WINDOW, desc),
    Target::Global,
);

let desc = cmd.take_object::<WindowDesc<T>>().unwrap();

is now

ctx.submit_command(
    Command::new(commands::NEW_WINDOW, SingleUse::new(desc)),
    Target::Global,
);

let desc = cmd.get_object::<SingleUse<WindowDesc<T>>>().unwrap().take().unwrap();

which can easily be utilized by typed selectors.

@luleyleo luleyleo added S-needs-review waits for review breaking change pull request breaks user code labels May 18, 2020
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks decent overall.

druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/win_handler.rs Outdated Show resolved Hide resolved
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool yea I think this is an improvement. A few comments inline, but looks good!

/// `SingleUse` is intended for cases where a command payload should only be
/// used once; an example would be if you have some resource that cannot be
/// cloned, and you wish to send it to another widget.
pub struct SingleUse<T>(Mutex<Option<T>>);
Copy link
Member

Choose a reason for hiding this comment

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

this type isn's actually exposed anywhere, so I don't think external crates can access it?

Also I would expect this to need to be in an Arc, since the Event itself still needs to be cloneable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I've reexported it from lib now.

Not sure what you mean regarding the Arc. When using SingleUse the payload will be stored the exact same way as it used to with Arg::OneShot, minus the Box<Any> wrapper as it is now directly stored as Arc<Any>.

Copy link
Member

Choose a reason for hiding this comment

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

ah okay I missed that the Arc has moved into the signature of the object field on Command.

Reusable(Arc<dyn Any>),
OneShot(Arc<Mutex<Option<Box<dyn Any>>>>),
}
/// `SingleUse` is intended for cases where a command payload should only be
Copy link
Member

Choose a reason for hiding this comment

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

huh I think I lost a comment here?? anyway, to try again:

Doc comments are kind of like git commit messages; you want the first paragraph to be a short general overview that doesn't need a lot of context, and then a blank line, and then a more detailed explanation.

This is because the first paragraph is displayed beside the type name in the module or crate level docs.

For this type, for instance, I would try a first paragraph of something to the effect of,

"A wrapper type for Command arguments that should only be used once."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I should have actually built the docs 😅

I've added the suggested head paragraph, and while I was at it also an example how to make use of SingleUse.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

One little non-blocker, thanks for this!

druid/src/app_delegate.rs Outdated Show resolved Hide resolved
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks good!

@luleyleo luleyleo removed the S-needs-review waits for review label May 18, 2020
@luleyleo luleyleo merged commit 746409c into linebender:master May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants