gyscos / cursive

A Text User Interface library for the Rust programming language
MIT License
4.17k stars 240 forks source link

[BUG] Calling find_name() in the wrong order makes things unfindable #752

Open c-jiph opened 10 months ago

c-jiph commented 10 months ago

Describe the bug The repro below panics with "tv" being None when unwrapped. If the two find_name() calls are swapped there is no problem.

I don't think this is mentioned as a something to be avoided anywhere. I am still fairly new to Rust so my apologies if there is something idiomatic I'm missing here.

To Reproduce

use cursive::{
    view::Nameable,
    views::{Dialog, TextView},
};

fn main() {
    let mut siv = cursive::default();

    siv.add_layer(Dialog::around(TextView::new("tv").with_name("tv")).with_name("d"));

    let _a = siv.find_name::<Dialog>("d").unwrap();
    let _b = siv.find_name::<TextView>("tv").unwrap();
}

Expected behavior Order of calls to find_name() makes no difference. If the problem was just I should drop the result of find_name() before making another call, that would be fine with me provided it were documented/enforced.

Environment

gyscos commented 10 months ago

Hi, and thanks for the report!

If the problem was just I should drop the result of find_name() before making another call, that would be fine with me provided it were documented/enforced.

We can and should indeed better document it!

The result from find_name "locks" the view you found, and prevents finding child views while this lock is kept. (Why do we lock the view? So we can implement DerefMut, which lets users directly call methods on the result.)

This is why it's usually preferred to use call_on_name (it's harder to mix up locks this way).

I'm not sure we could enforce it though: it's valid to use find_name on disjoint views, it's only an issue when one is nested another the other one.

c-jiph commented 10 months ago

Thanks for getting back to me.

I'm not sure we could enforce it though: it's valid to use find_name on disjoint views, it's only an issue when one is nested another the other one.

I was imagining find_name would return some kind of guard which would mark the view as "locked". Then whenever find_name is called it would search from child to root checking to make sure everything is "unlocked", panicing if not. Once the guard is dropped the lock would be released.

Having said that, I hadn't considered call_on_name and TBH wasn't really sure why this would be used until now. Does call_on_name have the same issue if it's re-called in a nested context? If not it sounds like this is also valid way to enforce things and perhaps the find_name doc can just recommend call_on_name.

Either way some mention in the docs would be most helpful.