Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c-jiph opened this issue Sep 7, 2023 · 2 comments
Open
Labels

Comments

@c-jiph
Copy link

c-jiph commented Sep 7, 2023

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

  • Operating system used: macOS 13.5.1
  • Backend used: ncurses
  • Current locale: en_US.UTF-8
  • Cursive version: 75bc082
@c-jiph c-jiph added the bug label Sep 7, 2023
@gyscos
Copy link
Owner

gyscos commented Sep 10, 2023

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
Copy link
Author

c-jiph commented Sep 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants