Added conditional function 'disabled_if' to disable the button based on a true boolean

[soucosmo]

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.

[B0ney]

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));

[soucosmo]

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

[B0ney]

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)

[hecrj]

This PR seems to have the changes from #2361. Can we get rid of those here?

[alex-ds13]

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.

[B0ney]

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.

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.

[alex-ds13]

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.

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()

[B0ney]

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()

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 Options 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.

[hecrj]

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.

[alex-ds13]

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.

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!

[B0ney]

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.

Yeah, this approach is so much better.

[soucosmo]

This PR seems to have the changes from #2361. Can we get rid of those here?

Yes, of course you can!

[soucosmo]

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.

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.

[alex-ds13]

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.

[soucosmo]

Removed files from previous pull request that included an example for keyed_column