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

Add ability to replace an item in a select view #768

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

squaresurf
Copy link

@squaresurf squaresurf commented Dec 4, 2023

This is a more efficient way to replace an item than calling remove_item, then insert_item

@gyscos
Copy link
Owner

gyscos commented Dec 5, 2023

Hi, and thanks for the PR!

Could SelectView::get_item_mut() solve your use-case? Or SelectView::iter_mut if you need to access several items. In particular, you should be able to swap two items efficiently this way.

I'm fine adding this method, if only for the S: Into<StyledString> convenience factor. Wondering if it should be set_item vs replace_item, also, we could return the old value? 🤷

@squaresurf
Copy link
Author

squaresurf commented Dec 7, 2023

Could SelectView::get_item_mut() solve your use-case? Or SelectView::iter_mut if you need to access several items. In particular, you should be able to swap two items efficiently this way.

I'm not sure why, but I couldn't seem to get a mutable reference properly where the compiler allowed me to mutate the references. Then I realized that I didn't care about the old values and just need an efficient way to replace an item.

Wondering if it should be set_item vs replace_item

Great idea. I'm happy to make this change.

also, we could return the old value? 🤷

I like this as it feels consistent with other set_item type APIs. I gave it a shot, but it turns out to be a bit complicated to return the values within the Rc. I've still not really used Rc much and am running into many issues with working with the type and the compiler. If you have a chance, would you mind adding a suggestion in the diff so I could see how to do this?

@squaresurf
Copy link
Author

I'm fine adding this method, if only for the S: Into convenience factor.

Do you mind explaining what you mean by this convenience? I'm not sure I understand this.

@gyscos
Copy link
Owner

gyscos commented May 24, 2024

I like this as it feels consistent with other set_item type APIs.

I'd be fine with a set_item method with the same signature; "replace" somehow implies that we're getting the old value back (for example like in std::mem::replace).

I'm not sure why, but I couldn't seem to get a mutable reference properly where the compiler allowed me to mutate the references. Then I realized that I didn't care about the old values and just need an efficient way to replace an item.

Here's how you can currently get mutable references to the label and content of a SelectView, and replace them:

let mut v = cursive::views::SelectView::new()  
    .item_str("Foo")  
    .item_str("Bar"); 
                              
let (label, content) = v.get_item_mut(1).unwrap();
*label = "foo".into(); 
*content = "foooo".into(); 

Here label is a &mut StyledString, so we need to call .into() on a &str or String to get there. This is what I meant where I said a function taking <S: Into<StyledString>> would hide the conversion from the user for a small convenience gain.

If you have a chance, would you mind adding a suggestion in the diff so I could see how to do this?

Once way is to simply return the Arc<T> directly and let users unwrap it as needed:

pub fn replace_item<S>(&mut self, id: usize, label: S, value: T) -> (StyledString, Arc<T>)
where                                                                                        
    S: Into<StyledString>,                                                                   
{                                                                                            
    let prev = std::mem::replace(&mut self.items[id], Item::new(label.into(), value));       
    self.last_required_size = None;                                                          
    (prev.label, prev.value)
}                                                                                            

Another option is to use Arc::into_inner to try and take the item out of the Arc. It will fail (return None) if there's a clone of this Arc still alive - it could happen if the user calls SelectView::selection() for example, and stores the result. We could call that and return the previous value as (StyledString, Option<T>), but I think it's best to just return Arc and let the user deal with this possibility more explicitly.

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

Successfully merging this pull request may close these issues.

2 participants