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

Avoid Clone bound on Message #191

Closed
wants to merge 3 commits into from
Closed

Avoid Clone bound on Message #191

wants to merge 3 commits into from

Conversation

Eroc33
Copy link

@Eroc33 Eroc33 commented Feb 16, 2020

As dmarcuse mentioned in #77 if you have some message variants with uncloneable contents you currently end up having to wrap things in Arc which isn't very ergonomic. Having widgets accept Fn instead of Message avoids this issue.

@Eroc33 Eroc33 changed the title Allow message to not be cloneable Avoid Clone bound on Message Feb 16, 2020
@hecrj
Copy link
Member

hecrj commented Feb 16, 2020

I am not convinced we should make the API more complicated because of the Arc use case.

You can currently have non-cloneable messages just fine by separating your UI interactions from your non-cloneable messages. I detailed how to do this here: #155 (comment)

@hecrj hecrj added the question Further information is requested label Feb 16, 2020
@hecrj hecrj added this to the 0.1.0 milestone Feb 16, 2020
@dmarcuse
Copy link

dmarcuse commented Feb 17, 2020

To me, the API doesn't really seem any more complicated - it's just a closure as opposed to a value. I think making it more consistent with other widgets (slider, text input, etc) which accept functions makes the API more ergonomic as well. As you noted here it is possible to define a second message type that satisfies the Clone bound, but this isn't really obvious (at least not to me) and takes a lot more effort than it would to use closures.

I understand that avoiding ways to introduce non-determinism is a goal of the project, but seeing as other widgets already use closures, I don't think it's worth sacrificing ergonomics for.

@Eroc33
Copy link
Author

Eroc33 commented Feb 17, 2020

Thanks for pointing out that alternative way to structure Messages. I think that will work for my case. However I do agree with dmarcuse that using closures is a much more obvious/discoverable way to allow this. If you're determined not to use closures for this would you be willing to accept a PR adding an example of the pattern discused in #155 (comment) (assuming I can think of a non-contrived example)? (Currently I don't think this is discoverable enough for a new user of iced)

@hecrj
Copy link
Member

hecrj commented Feb 17, 2020

it's just a closure as opposed to a value

I think this is a big distinction. There is no need for a widget to accept a closure with no arguments just so we can construct many messages. A Clone bound is exactly meant to convey this!

I think making it more consistent with other widgets (slider, text input, etc) which accept functions makes the API more ergonomic

I think it would be inconsistent! Widgets use closures when they provide some data that is relevant to construct a message. Most of the time, you are not even meant to use the closure syntax:

let text_input = TextInput::new(
    state,
    "Type something to continue...",
    value,
    StepMessage::InputChanged,
)

Forcing our users to type on_press(|| Message::ButtonPressed) just for buttons does not seem right! A different user could ask: why does on_press accept a closure instead of requiring Clone? In the end, this seems very subjective to me.

takes a lot more effort than it would to use closures

Simplicity isn't always convenient. Structuring your Message type properly is key in an application that uses the Elm Architecture.

Moreover, there are also some long term features that will force messages to satisfy Clone, like a time-travelling debugger. Although we may add that as a requirement in a different trait, I believe it's something we should keep in mind: there are many reasons why a Clone bound may be necessary and giving it up at the root level will have consequences in the long run.

If you're determined not to use closures for this would you be willing to accept a PR adding an example of the pattern discused in #155 (comment) (assuming I can think of a non-contrived example)?

Of course! Feel free to join the Zulip server and share your ideas there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants