-
Notifications
You must be signed in to change notification settings - Fork 567
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
Rework (and rename) ext event #1401
Conversation
I decided to rename form |
druid/examples/async_event.rs
Outdated
use druid::widget::prelude::*; | ||
use druid::{AppLauncher, Color, Selector, Target, WidgetExt, WindowDesc}; | ||
|
||
// If you want to send commands to over event sinks you have to give it some kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commands to over event sinks
not sure what you're saying here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to submit commands to an event sink
is what I mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great improvement! I'm fine with the name change. I could see people being confused that this isn't async
in the general Rust ecosystem use of that word, but at the same time that's probably the word people are looking for for this kind of functionality?
yeah I thought about that too, I can rename it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few little notes but I think this is an improvement!
druid/examples/async_event.rs
Outdated
.expect("launch failed"); | ||
} | ||
|
||
fn generate_colours(event_sink: druid::ExtEventSink) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I personally use international spellings frequently, we have decided to use Color
instead of Colour
in druid so I think it probably makes sense to be consistent with that here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sry, I try to keep is US English as well, but my nature is very much still with colour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me too, but sometimes we must defer to the imperialists. 😢
Co-authored-by: Colin Rofls <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This mostly just adds comments.
I also got a more satisfying colour generation comming in another PR for this and the
identity
example.I also think that this and the
identity
one are too similair to not be based on each other. Both use comands and have colours shifting going on. Both should handle the colour shifting the same way (not using timers like theidentity
example) so I will be opening a PR about completely redoing that one. It will also make it cleaner I think.EDIT: I also will be opening a PR to remove the
blocking_functions
example since it really just has a lot of clutter and revolves around the same concept as this example.sorry this took a while, my factorio addiction is acting up again.