iced
iced copied to clipboard
Added conditional function 'disabled_if' to disable the button based on a true boolean
Added function to disable the button in a more elegant way, simply entering the Boolean value true to make the button inactive.
If false is entered, nothing will be affected.
Rust's bool
has a then_some
/then
method. This can be used with on_press_maybe
to achieve the same thing:
let enabled = false;
button("hi").on_press_maybe(enabled.then_some(Message::Hi));
Here's a brief explanation of what motivated me in this PR
let user_input = String::new(); // it will most likely already exist somewhere
button("hi").on_press(Message::Hi).disabled_if(user_input.is_empty());
with on_press_maybe I would need to do this in its cleanest form
let user_input = String::new();
button("hi").on_press_maybe((!user_input.is_empty()).then_some(Message::Hi));
This implies a conversion from true to false, which in the end ends up being a more "complicated" solution.
For me, "disabled if" is more natural and concise, as it fits well with button modeling
Ah I see. While you can use variables to help reduce cognitive load in the latter example:
let password_entered = !password.is_empty();
button("Login")
.on_press_maybe(password_entered.then_some(Message::Login))
I do think your proposed method would definitely improve developer experience. Actually, I really like the explicit separation:
button("Login")
.disable_if(password.is_empty())
.on_press(Message::Login)
I do think your proposed method would definitely improve developer experience. Actually, I really like the explicit separation:
button("Login") .disable_if(password.is_empty()) .on_press(Message::Login)
That's not going to work the way you think it will. That won't disable the button. The .disable_if
disables the button, but the .on_press
afterwards enables it back on again. This means that order matters for these functions so either it has to be said so on the documentation for .disable_if
or it needs to be done some other way without messing with the on_press
value of button.
One way to do it would be by adding a disabled
field to Button
which would be false by default like this:
pub struct Button<'a, Message, Theme = crate::Theme, Renderer = crate::Renderer>
where
Renderer: crate::core::Renderer,
Theme: Catalog,
{
content: Element<'a, Message, Theme, Renderer>,
on_press: Option<Message>,
width: Length,
height: Length,
padding: Padding,
clip: bool,
disabled: bool,
class: Theme::Class<'a>,
}
/// Creates a new [`Button`] with the given content.
pub fn new(
content: impl Into<Element<'a, Message, Theme, Renderer>>,
) -> Self {
let content = content.into();
let size = content.as_widget().size_hint();
Button {
content,
on_press: None,
width: size.width.fluid(),
height: size.height.fluid(),
padding: DEFAULT_PADDING,
clip: false,
disabled: false,
class: Theme::default(),
}
}
And then the .disable_if
function would look like this:
pub fn disable_if(mut self, disable: bool) -> Self {
self.disabled = disable;
self
}
Of course that would mean that you would have to change the on_event
, draw
and mouse_interaction
on these lines: (P.S. There might be other places that need changing, I didn't check that exhaustively...)
fn on_event(...) -> ... {
...
match event {
Event::Mouse(mouse::Event::ButtonPressed(mouse::Button::Left))
| Event::Touch(touch::Event::FingerPressed { .. }) => {
if self.on_press.is_some() { //HERE needs to change to `if !self.disabled && self.on_press.is_some() {`
....
}
Event::Mouse(mouse::Event::ButtonReleased(mouse::Button::Left))
| Event::Touch(touch::Event::FingerLifted { .. }) => {
if let Some(on_press) = self.on_press.clone() { //HERE this entire if statement needs to be wrapped on another `if !self.disabled {`
....
}
...
}
fn draw(...) {
....
let status = if self.on_press.is_none() { //HERE needs to change to `if self.disabled || self.on_press.is_none()`
Status::Disabled
} else if is_mouse_over {
....
}
fn mouse_interaction(...) -> ... {
let is_mouse_over = cursor.is_over(layout.bounds());
if is_mouse_over && self.on_press.is_some() { //HERE needs to change to `if is_mouse_over && !self.disabled && self.on_press.is_some() {`
mouse::Interaction::Pointer
} else {
mouse::Interaction::default()
}
}
Even though this is a bigger change I feel it would result in a better experience for the developer, because you could setup the on_press
message whenever you wanted and then use .disable_if
to control the status of the button without having to care about the order on which you call these functions. This way the example above would work.
I also think the name should be .disable_if
and not .disabled_if
since it better shows the intent of what you're doing.
I do think your proposed method would definitely improve developer experience. Actually, I really like the explicit separation:
button("Login") .disable_if(password.is_empty()) .on_press(Message::Login)
That's not going to work the way you think it will. That won't disable the button. The
.disable_if
disables the button, but the.on_press
afterwards enables it back on again. This means that order matters for these functions so either it has to be said so on the documentation for.disable_if
or it needs to be done some other way without messing with theon_press
value of button.One way to do it would be by adding a
disabled
field toButton
which would be false by default like this:pub struct Button<'a, Message, Theme = crate::Theme, Renderer = crate::Renderer> where Renderer: crate::core::Renderer, Theme: Catalog, { content: Element<'a, Message, Theme, Renderer>, on_press: Option<Message>, width: Length, height: Length, padding: Padding, clip: bool, disabled: bool, class: Theme::Class<'a>, }
/// Creates a new [`Button`] with the given content. pub fn new( content: impl Into<Element<'a, Message, Theme, Renderer>>, ) -> Self { let content = content.into(); let size = content.as_widget().size_hint(); Button { content, on_press: None, width: size.width.fluid(), height: size.height.fluid(), padding: DEFAULT_PADDING, clip: false, disabled: false, class: Theme::default(), } }
And then the
.disable_if
function would look like this:pub fn disable_if(mut self, disable: bool) -> Self { self.disabled = disable; self }
Of course that would mean that you would have to change the
on_event
,draw
andmouse_interaction
on these lines: (P.S. There might be other places that need changing, I didn't check that exhaustively...)fn on_event(...) -> ... { ... match event { Event::Mouse(mouse::Event::ButtonPressed(mouse::Button::Left)) | Event::Touch(touch::Event::FingerPressed { .. }) => { if self.on_press.is_some() { //HERE needs to change to `if !self.disabled && self.on_press.is_some() {` .... } Event::Mouse(mouse::Event::ButtonReleased(mouse::Button::Left)) | Event::Touch(touch::Event::FingerLifted { .. }) => { if let Some(on_press) = self.on_press.clone() { //HERE this entire if statement needs to be wrapped on another `if !self.disabled {` .... } ... } fn draw(...) { .... let status = if self.on_press.is_none() { //HERE needs to change to `if self.disabled || self.on_press.is_none()` Status::Disabled } else if is_mouse_over { .... } fn mouse_interaction(...) -> ... { let is_mouse_over = cursor.is_over(layout.bounds()); if is_mouse_over && self.on_press.is_some() { //HERE needs to change to `if is_mouse_over && !self.disabled && self.on_press.is_some() {` mouse::Interaction::Pointer } else { mouse::Interaction::default() } }
Even though this is a bigger change I feel it would result in a better experience for the developer, because you could setup the
on_press
message whenever you wanted and then use.disable_if
to control the status of the button without having to care about the order on which you call these functions. This way the example above would work.I also think the name should be
.disable_if
and not.disabled_if
since it better shows the intent of what you're doing.
Oh right! Thanks for pointing that out.
Instead of having these throughout the codebase:
self.disabled || self.on_press.is_none()
// or
!self.disabled && self.on_press.is_some()
We can introduce a private helper method to get the on_press
value while taking disabled
into account.
fn message(&self) -> Option<Message> {
(!self.disabled)
.then_some(self.on_press)
.flatten()
}
So hopefully the only thing we need to do is use the self.message()
helper instead of self.on_press
.
We can introduce a private helper method to get the
on_press
value while takingdisabled
into account.fn message(&self) -> Option<Message> { (!self.disabled) .then_some(self.on_press) .flatten() }
So hopefully the only thing we need to do is use the
self.message()
helper instead ofself.on_press
.
This might not be the best solution because we would have to clone the option self.on_press.clone()
every time we wanted to check if it was disabled or not. But a simple is_disabled()
check would work and simplify the code still:
fn is_disabled(&self) -> bool {
self.disabled || self.on_press.is_none()
}
Then we would just use it as:
self.is_disabled()
// or
!self.is_disabled()
Or we can also always have an .is_enabled
function to make it more clear.
self.is_disabled()
// or
self.is_enabled()
This might not be the best solution because we would have to clone the option
self.on_press.clone()
every time we wanted to check if it was disabled or not. But a simpleis_disabled()
check would work and simplify the code still:fn is_disabled(&self) -> bool { self.disabled || self.on_press.is_none() }
Then we would just use it as:
self.is_disabled() // or !self.is_disabled()
Or we can also always have an
.is_enabled
function to make it more clear.self.is_disabled() // or self.is_enabled()
Ah, I forgot to add &
to Option<Message>
when I was writing it out:
fn message(&self) -> Option<&Message> {
(!self.disabled)
.then_some(self.on_press.as_ref())
.flatten()
}
I still think using Option
s instead would be a better fit. As it 1) avoids adding more (and arguably more brittle) logic - it could be as simple as replacing on_press
. And 2) avoids writing code like self.is_enabled()
, !self.is_enabled()
, etc.
I think it's as simple as making disabled_if
set on_press
to None
and the is_disabled
flag to true
, then ignore any future on_press
calls if is_disabled
is set.
I think it's as simple as making
disabled_if
seton_press
toNone
and theis_disabled
flag totrue
, then ignore any futureon_press
calls ifis_disabled
is set.
That is definitely much simpler! 😄 The only difference is that it wouldn't keep the on_press
message stored. But there doesn't seem to be any advantage in doing so either, so I guess it's better to have it as simple as can be!
I think it's as simple as making
disabled_if
seton_press
toNone
and theis_disabled
flag totrue
, then ignore any futureon_press
calls ifis_disabled
is set.
Yeah, this approach is so much better.
This PR seems to have the changes from #2361. Can we get rid of those here?
Yes, of course you can!
I do think your proposed method would definitely improve developer experience. Actually, I really like the explicit separation:
button("Login") .disable_if(password.is_empty()) .on_press(Message::Login)
That's not going to work the way you think it will. That won't disable the button. The
.disable_if
disables the button, but the.on_press
afterwards enables it back on again. This means that order matters for these functions so either it has to be said so on the documentation for.disable_if
or it needs to be done some other way without messing with theon_press
value of button.One way to do it would be by adding a
disabled
field toButton
which would be false by default like this:pub struct Button<'a, Message, Theme = crate::Theme, Renderer = crate::Renderer> where Renderer: crate::core::Renderer, Theme: Catalog, { content: Element<'a, Message, Theme, Renderer>, on_press: Option<Message>, width: Length, height: Length, padding: Padding, clip: bool, disabled: bool, class: Theme::Class<'a>, }
/// Creates a new [`Button`] with the given content. pub fn new( content: impl Into<Element<'a, Message, Theme, Renderer>>, ) -> Self { let content = content.into(); let size = content.as_widget().size_hint(); Button { content, on_press: None, width: size.width.fluid(), height: size.height.fluid(), padding: DEFAULT_PADDING, clip: false, disabled: false, class: Theme::default(), } }
And then the
.disable_if
function would look like this:pub fn disable_if(mut self, disable: bool) -> Self { self.disabled = disable; self }
Of course that would mean that you would have to change the
on_event
,draw
andmouse_interaction
on these lines: (P.S. There might be other places that need changing, I didn't check that exhaustively...)fn on_event(...) -> ... { ... match event { Event::Mouse(mouse::Event::ButtonPressed(mouse::Button::Left)) | Event::Touch(touch::Event::FingerPressed { .. }) => { if self.on_press.is_some() { //HERE needs to change to `if !self.disabled && self.on_press.is_some() {` .... } Event::Mouse(mouse::Event::ButtonReleased(mouse::Button::Left)) | Event::Touch(touch::Event::FingerLifted { .. }) => { if let Some(on_press) = self.on_press.clone() { //HERE this entire if statement needs to be wrapped on another `if !self.disabled {` .... } ... } fn draw(...) { .... let status = if self.on_press.is_none() { //HERE needs to change to `if self.disabled || self.on_press.is_none()` Status::Disabled } else if is_mouse_over { .... } fn mouse_interaction(...) -> ... { let is_mouse_over = cursor.is_over(layout.bounds()); if is_mouse_over && self.on_press.is_some() { //HERE needs to change to `if is_mouse_over && !self.disabled && self.on_press.is_some() {` mouse::Interaction::Pointer } else { mouse::Interaction::default() } }
Even though this is a bigger change I feel it would result in a better experience for the developer, because you could setup the
on_press
message whenever you wanted and then use.disable_if
to control the status of the button without having to care about the order on which you call these functions. This way the example above would work.I also think the name should be
.disable_if
and not.disabled_if
since it better shows the intent of what you're doing.
Hahaha, I have difficulty expressing myself sometimes, my native language is Brazilian Portuguese, for me it is natural as "desabilitado se (for verdadeiro)".
About the call order being ..on_press(..).disabled_if(...) is mentioned in the documentation, in my opinion, it is something extremely simple, I thought about including something more elaborate to be used in any order, but the real It's just that this type of behavior replacement happens frequently and in several libs and languages in general, adjusting the way you said would be adding more stress to the processor and RAM for something that the next programmer can control in a simpler way, I'll say it again , this is specified in the documentation, so in my humble opinion it is not a problem.
I also think the name should be
.disable_if
and not.disabled_if
since it better shows the intent of what you're doing.Hahaha, I have difficulty expressing myself sometimes, my native language is Brazilian Portuguese, for me it is natural as "desabilitado se (for verdadeiro)".
For me it's fine either way, but I prefer to use the verb since when reading the code it would read like a sentence. In Portuguese it would be 'desabilitar_se(verdadeiro)'. Maybe @hecrj can give his opinion.
About the call order being ..on_press(..).disabled_if(...) is mentioned in the documentation, in my opinion, it is something extremely simple, I thought about including something more elaborate to be used in any order, but the real It's just that this type of behavior replacement happens frequently and in several libs and languages in general, adjusting the way you said would be adding more stress to the processor and RAM for something that the next programmer can control in a simpler way, I'll say it again , this is specified in the documentation, so in my humble opinion it is not a problem.
Most people don't read the docs so I bet you there will be a couple of issues showing up saying "hey disable_if doesn't work", so every time we can prevent non-issues from being created is a plus. Also there's the possibility that in some complex apps a mut button can be created somewhere with .disable_if
and changed elsewhere and people might forget about the ordering.
The solution proposed by @hecrj is really simple and solves this problems.