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

API error #721

Open
smallB007 opened this issue Mar 18, 2023 · 6 comments
Open

API error #721

smallB007 opened this issue Mar 18, 2023 · 6 comments

Comments

@smallB007
Copy link
Contributor

API design is incorrect. In order to find something I shouldn't require to have mutable access to neither the container that holds it nor the thing itself. Unfortunately at this moment I need mut access to perform find.

pub fn is_search_bar_visible(s: &mut Cursive) -> bool {
    s.call_on_name(
        "hideable_info_stack_view_name",
        |hideable: &mut HideableView<ResizedView<StackView>>| {
            let stack_view = hideable.get_inner_mut().get_inner_mut();//If I have just get_inner it doesn't compile because find_* inside requires mut.
            let pos = stack_view.find_layer_from_name(STACKED_SEARCH_BAR).unwrap();
            hideable.is_visible()
        },
    )
    .unwrap()
}
@gyscos
Copy link
Owner

gyscos commented Mar 27, 2023

Hi, and thanks for the report.

Indeed, the entire chain of methods around call_on_any could be duplicated to support non-mutable access. (As is often the story with rust mutable/non-mutable methods).

I felt like the extra code duplication was not quite justified given that users very often have mutable access to the views, but there may be exceptions of course.

@smallB007
Copy link
Contributor Author

Hi,
Thank you for reply. As much as I do understand your reasons I actually think that the issue lies in methods like find_*, where they take &mut, which doesn't make sense. I am pretty certain that it can be refactored in way that searching for "things" shouldn't require &mut.

@gyscos
Copy link
Owner

gyscos commented Mar 27, 2023

It could, but as I said it would require a non-mutable version of the current call_on_any chain of methods.
And if you wanted a mutable ref to the thing you found, it would need a mutable version anyway.

As I mentioned, it's possible to have such non-mutable duplicates of the various functions, it's just rarely required.

@smallB007
Copy link
Contributor Author

If the find method didn't require &mut you wouldn't have to have code dups. All that would happen: the find method would have better API.

@gyscos
Copy link
Owner

gyscos commented Mar 27, 2023

Note that find_layer_from_name (and find_name) use call_on_any behind the scenes, and not the other way around. So to make find_layer_from_name and find_name take a &self, we'd need a call_on_any method that also takes &self, while still having a call_on_any_mut for cases when you do want to mutate what you find.

And you'd probably still want a find_name_mut that takes a &mut self so it could return mutable reference, without forcing interior mutability in the API.

If this use-case pattern is very common, then the code duplication could be justified, absolutely. I'd also welcome any PR to help improve the API!

@smallB007
Copy link
Contributor Author

Thank you for the reply. If I find a free time I'll try to look at your code and see if it can be refactored/structured in better way.
Best regards

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

No branches or pull requests

2 participants