maciejhirsz / kobold

Easy declarative web interfaces.
https://docs.rs/kobold/
Mozilla Public License 2.0
385 stars 7 forks source link

error[E0507]: cannot move out of `on_________`, a captured variable in an `FnMut` closure #58

Closed ltfschoen closed 1 year ago

ltfschoen commented 1 year ago

In this "draft" Pull Request, i am getting the following errors in the last commit since trying to introduce new changes mentioned below. the last commit where it was working before any of these changes is this one

click to show error ```bash error[E0507]: cannot move out of `onchange`, a captured variable in an `FnMut` closure --> examples/invoice/src/main.rs:342:42 | 299 | let onchange = state.bind(move |state, e: Event| { | -------- captured outer variable ... 331 | for (0..indexes.len()-1).map(move |index| | ------------ captured by this `FnMut` closure ... 342 | {onchange} | ^^^^^^^^ move occurs because `onchange` has type `impl Fn(kobold::event::Event) + 'static`, which does not implement the `Copy` trait error[E0507]: cannot move out of `onmouseover`, a captured variable in an `FnMut` closure --> examples/invoice/src/main.rs:343:42 | 313 | let onmouseover = state.bind(move |state, e: MouseEvent| { | ----------- captured outer variable ... 331 | for (0..indexes.len()-1).map(move |index| | ------------ captured by this `FnMut` closure ... 343 | {onmouseover} | ^^^^^^^^^^^ move occurs because `onmouseover` has type `impl Fn(kobold::event::Event) + 'static`, which does not implement the `Copy` trait error[E0507]: cannot move out of `onkeypress`, a captured variable in an `FnMut` closure --> examples/invoice/src/main.rs:344:42 | 317 | let onkeypress = state.bind(move |state, e: KeyboardEvent| { | ---------- captured outer variable ... 331 | for (0..indexes.len()-1).map(move |index| | ------------ captured by this `FnMut` closure ... 344 | {onkeypress} | ^^^^^^^^^^ move occurs because `onkeypress` has type `impl Fn(kobold::event::Event) + 'static`, which does not implement the `Copy` trait error[E0507]: cannot move out of `onblur`, a captured variable in an `FnMut` closure --> examples/invoice/src/main.rs:345:42 | 307 | let onblur = state.bind(move |state, e: Event| { | ------ captured outer variable ... 331 | for (0..indexes.len()-1).map(move |index| | ------------ captured by this `FnMut` closure ... 345 | {onblur} | ^^^^^^ move occurs because `onblur` has type `impl Fn(kobold::event::Event) + 'static`, which does not implement the `Copy` trait error[E0507]: cannot move out of `ondblclick`, a captured variable in an `FnMut` closure --> examples/invoice/src/main.rs:366:49 | 355 | let ondblclick = state.bind(move |s, _| s.entry.editing = true); | ---------- captured outer variable ... 362 | for (0..indexes.len()-1).map(move |index| | ------------ captured by this `FnMut` closure ... 366 |

The most obvious solution for me was to update struct Event and give it #[derive(Copy)] as shown below:

#[derive(Copy)]
pub struct Event<T = HtmlElement, E = web_sys::Event> {

but that gave error:

error[E0204]: the trait `Copy` may not be implemented for this type
  --> crates/kobold/src/event.rs:19:10
   |
19 | #[derive(Copy)]
   |          ^^^^
20 | pub struct Event<T = HtmlElement, E = web_sys::Event> {
21 |     event: web_sys::Event,
   |     --------------------- this field does not implement `Copy`

Update: I realised when I did this that I was still using kobold "0.6.0" instead of "0.7.0", which doesn't have the same code, but even after I updated to "0.7.0" I still got the same errors

To provide some context of what I'm doing, I'm customizing the EntryView component, so it gets the state.details.table (which is a table like in its mock here where it just has a first row of label columns and second row with corresponding data columns.

Then here I populate a valid_labels Vec with its labels from the first row, then populate a values Vec with the data from the second row, and I populate an indexes Vec with the corresponding indexes of each column where the data came from (since i need to loop through them with for (0..indexes.len()-1).map(move |index| in the view!. So each of those Vec have the same length.

Then I use Branch2::A if they are editing, or Branch2::B otherwise.

I'm having trouble in Branch2::A for editing at the moment, relevant code snippet below:

let onchange = state.bind(move |state, e: Event<InputElement>| {
    if let Some(data_index) = e.target().get_attribute("data-index") {
        let index: usize = data_index.parse::<usize>().unwrap();
        state.details.table.rows[0][index] = Text::Owned(e.target().value().into());
        state.entry.editing = false;
    }
});

let onblur = state.bind(move |state, e: Event<InputElement>| {
    if e.target().value() != "" {
        state.update(e.target().value())
    }
});

let onmouseover = state.bind(move |state, e: MouseEvent<InputElement>| {
    let _ = e.target().focus();
});

let onkeypress = state.bind(move |state, e: KeyboardEvent<InputElement>| {
    if e.key() == "Enter" && e.target().value() != "" {
        state.update(e.target().value());

        Then::Render
    } else {
        Then::Stop
    }
});

Branch2::A(
    view! {
        <div>
            {
                for (0..indexes.len()-1).map(move |index|
                    view! {
                        <div.edit>
                            { values[index].clone() }
                            <input.edit
                                value={ values[index].clone() }
                                type="text"
                                placeholder="<Enter biller address>"
                                // if i use `data-index` it gives error
                                // `expected expression`
                                data_index={ index.to_string() }
                                {onchange}
                                {onmouseover}
                                {onkeypress}
                                {onblur}
                            />
                        </div>
                    }
                )
            }
        </div>
    }
)

I try to use for (0..indexes.len()-1).map(move |index| to generate an editable input field for each column of data. I try to store a reference to the index in a data attribute like this data_index={ ref index.to_string() }. I show the current value being edited in the input field with value={ ref values[index] } as shown. Note: I tried to use the HTML syntax data-index there, but that gave an error expected expression, hence why i used data_index (with an _ underscore instead of dash -). Maybe I also need to add that data-* attribute in the Kobold macros here https://github.com/maciejhirsz/kobold/blob/master/crates/kobold_macros/src/gen/element.rs#L260

Then since there are multiple input fields (for each column from the table), I need the onchange handler to cater for each one, so I get the value of the data_index attribute from the input element, since it contains the index that corresponds to the column with the data that we want to populate if there are any changes, so I can use the index variable like this state.details.table.rows[0][index]. code snippet below

let onchange = state.bind(move |state, e: Event<InputElement>| {
    if let Some(data_index) = e.target().get_attribute("data_index") {
        let index: usize = data_index.parse::<usize>().unwrap();
        state.details.table.rows[0][index] = Text::Owned(e.target().value().into());
        ...
    }
});

Here I'm not sure whether I should use get_attribute("data_index") or get_attribute("data-index") (i.e. dash or underscore).

The errors shown above disappear if I comment out the following code, but I want to use that code:

<input.edit
  ...
  // {onchange}
  // {onmouseover}
  // {onkeypress}
  // {onblur}

...

// let ondblclick = state.bind(move |s, _| s.entry.editing = true);

...

// <label {ondblclick} >
//     { values[index].clone() }
// </label>
maciejhirsz commented 1 year ago

Remove the move keyword from the closure in the iterator:

for (0..indexes.len()-1).map(|index|

Instead of:

for (0..indexes.len()-1).map(move |index|

If that doesn't work, you'll need to declare the event handlers inside the map instead of trying to capture copies from the outside.

Bonus tip 1:

let onmouseover = state.bind(move |state, e: MouseEvent<InputElement>| {
    let _ = e.target().focus();
});

Can be just:

let onmouseover = |e: MouseEvent<InputElement>| {
    let _ = e.target().focus();
};

Since it doesn't do anything with the state.

On latest master I've shadowed the focus method to not return a result (it's guaranteed to work) so you can make it even simpler:

let onmouseover = |e: MouseEvent<InputElement>| e.target().focus();

Bonus tip 2:

Not sure what that -1 is doing there, .. is an exclusive range already, 0..5 iterates over 5 values: 0,1,2,3, and 4.

maciejhirsz commented 1 year ago

@ltfschoen I made a discord server: https://discord.gg/tgNX5VYSWF

Feel free to ask Rust-related questions there. I'm happy to answer them, but I'd like to keep the Issues on GH clean for, well, issues and discussions related to Kobold, not stuff like move semantics or ownership rules of Rust :upside_down_face:.

(Also not at the keyboard a lot today so might be slow).

ltfschoen commented 1 year ago

thanks, all your feedback worked and in this commit