Consider making EditView::on_edit take an FnMut

[Powersource]

Basically this entire Q&A https://stackoverflow.com/questions/41990175/problems-with-mutability-in-a-closure
user's comment at the bottom sums it up:

It would still be a good idea to check with the cursive developers whether on_edit really requires a Fn or it could be relaxed to accept a FnMut. If the current constraint is there for a good reason, cheating it with RefCell might eventually lead to a panic at run-time.

[gyscos]

EditView stores a Rc<Fn(...)>, because when running, the callback itself can have access to the EditView, and change (or call again) the callback. Since Rc doesn't allow mutable access to the inner variable, this is the main reason not to store a FnMut.

Now, EditView could conceivable be modified to store something like a Rc<RefCell<FnMut(...)>> to provide the desired mutability. Basically, this means that giving it a callback that looks for the EditView itself and runs the callback again (by calling the view's on_event method for instance) would panic (well - we could put a check there to see if the callback is already borrowed, and just ignore the event if it is, but that's not a very clean solution and leads to surprising behaviors).

In the end, pushing the RefCell to the closure itself is more flexible, and the user is more aware of the exact runtime risks. It is unfortunate though that this pushes more complexity to the user.

That being said, I'm open to a better design allowing for nicer mutability constraints. But the main issue - that a callback with access to the Cursive root can call itself recursively, which is incompatible with FnMut - is hard to avoid.

TLDR: possible solutions:

• Current situation: it is valid for the callback to call on_event on its own view, and call itself recursively (hopefully not endlessly). Mutability is handled entirely by the user to fit his needs.
• Use a FnMut callback without access to the Cursive root, preventing any recursive callback. But not having access to &mut Cursive sounds pretty limited, and I don't think it's worth the extra complexity.
• "Disable" the callback while it is running: calling on_event from the callback will behave as if no callback was registered (thus preventing recursive calls). I suppose with good documentation (and a few big warnings), this could be acceptable.
  The problem is if someone wants recursive callback calls, and do not care about the mutability. I may end up adding both alternatives: either a recursive-able Fn, or a FnMut that doesn't recurse.

[gyscos]

I pushed a on_edit branch with the "disable" solution (where RefCell<FnMut> are stored, and recursive callbacks are "disabled", IE calling on_event from the on_edit callback will behave as if no on_edit was set). You can try it and see if it helps your problem.
You can reference a branch in Cargo.toml:

[dependencies]
cursive = { git="https://github.com/gyscos/cursive", branch="on_edit" }

I tried to store either a RefCell<FnMut> or a Fn, depending on what the user wants to store, but issues like rust-lang/rust#29625 make this a bit more difficult than it should.

[gyscos]

I ended up keeping on_edit as it was, and adding as_edit_mut that calls on_edit with:

let callback = RefCell::new(callback);
move |s, text, cursor| {
    if let Ok(mut callback) = callback.try_borrow_mut() {
        (&mut *callback)(s, text, cursor);
    }
}

It ends up being a rather small change and rather isolated from the rest of the code, which would point to it not being necessary in Cursive itself; but it's a bit too complex and verbose to force this on users...

Ideally, I'd just add a method like that:

fn immutablify<Args, F: FnMut<Args>>(f: F) -> impl Fn(Args) {
    let f = RefCell::new(f);
    move |args: Args| {
        if let Ok(f) = f.try_borrow_mut() {
            (&mut *f)(args);
        }
    }
}

But:

• This uses "impl trait", which doesn't look stable quite yet
  • This could be worked around by boxing the return - non optimal, but still ok
• This requires some voodoo with the Args and FnMut traits that are probably either unstable or plain impossible for now.

So for now, I'll go with the on_edit_mut solution.

[gyscos]

It should be in master as of 0bbc107.

[jD91mZM2]

Seems to be the same with MenuTree's leaf and add_global_callback, et.c

[gyscos]

Ah yes, any place that takes a callback method will need special handling :-/

[jD91mZM2]

Oh, ok.

[gyscos]

That was one of the main attractive points of a immutablify method... I'll see if a immutablify! macro would be easier to implement.