add value_container for provide on_update to checkbox

[dzhou121]

No description provided.

[jrmoulton]

This looks good to me

[pieterdd]

I wonder if there's a way to simplify this part. Since any composited widget that returns a ValueContainer will follow the pattern set up here, any simplification will pay off for Floem devs as well as users who make their own components.

[dzhou121]

This part is probably quite different for different views I think.

[pieterdd]

When I get back from work tonight, I'd like to try rewriting this widget so it doesn't require additional signals and effects. I might arrive at the same conclusion, but it would be satisfying to achieve a simpler result.

[pieterdd]

It looks like I reduced the amount of additional signals from two to one and preserved the original ReadSignal parameter without breaking anything:

test.mp4

/// Renders a checkbox using the provided checked signal.
pub fn labeled_checkbox<S: Display + 'static>(
    checked: ReadSignal<bool>,
    label: impl Fn() -> S + 'static,
) -> ValueContainer<bool> {
    let inner_signal = create_rw_signal(checked.get_untracked());
    create_effect(move |_| {
        if checked.get() != inner_signal.get_untracked() {
            inner_signal.set(checked.get());
        }
    });

    value_container(
        h_stack((checkbox_svg(inner_signal.read_only()), views::label(label)))
            .class(LabeledCheckboxClass)
            .style(|s| s.items_center().justify_center()),
        move || inner_signal.get(),
    )
    .keyboard_navigatable()
    .on_click_stop(move |_| {
        inner_signal.set(!checked.get());
    })
}

If this aligns with your intentions, I might be able to further simplify the implementation - specifically the part about the inner signal that propagates changes from the input events to the on_update. It might be possible to create a helper function for this to minimize repetitive code.

[pieterdd]

Here's a new iteration that builds on your work: #289

/// Renders a checkbox using the provided checked signal.
pub fn labeled_checkbox<S: Display + 'static>(
    checked: ReadSignal<bool>,
    label: impl Fn() -> S + 'static,
) -> ValueContainer<bool> {
    let (inner_signal, set_inner_signal) = create_inner_signal(checked);

    value_container(
        h_stack((checkbox_svg(inner_signal), views::label(label)))
            .class(LabeledCheckboxClass)
            .style(|s| s.items_center().justify_center())
            .keyboard_navigatable()
            .on_click_stop(move |_| {
                set_inner_signal.set(!checked.get());
            }),
        inner_signal,
    )
}

[dzhou121]

That part I understand, but in what situation would someone need a closure instead of a ReadSignal? As far as I'm aware, checkbox(|| false) is a checkbox that can never be checked. So I'm not sure I understand the use case.

When you click it, it will trigger on_update

[jrmoulton]

That part I understand, but in what situation would someone need a closure instead of a ReadSignal? As far as I'm aware, checkbox(|| false) is a checkbox that can never be checked. So I'm not sure I understand the use case.

That closure will only be run once since it doesn't contain a signal. So it will only set the initial state. If it contained a signal then it would set it every time that the signal changed

[sudormrfbin]

That part I understand, but in what situation would someone need a closure instead of a ReadSignal?

One use case that I can think of is for some kind of derived signal -- a simple closure that computes a value using other "real" signals:

let count = create_rw_signal(0);

checkbox(|| count.get() == 42)
    .on_update(|checked| count.set(42));

[jrmoulton]

One use case that I can think of is for some kind of derived signal -- a simple closure that computes a value using other "real" signals:

let count = create_rw_signal(0);

checkbox(|| count.get() == 42)
    .on_update(|checked| count.set(42));

I run into this all the time

[pieterdd]

Thanks everyone for explaining. Now that I understand the purpose, I made the necessary modifications to support checkbox(|| false) at #290 and created a generic function create_value_container_signals with documentation to make it reusable. I hope it's useful.

[pieterdd]

This is not quite the same as before. Here's the new version:

I'd consider keeping the separate implementation.

[dzhou121]

close in favour of #290