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

Redesign druid::widget::ListIter trait? #1644

Open
changhe3 opened this issue Mar 12, 2021 · 6 comments
Open

Redesign druid::widget::ListIter trait? #1644

changhe3 opened this issue Mar 12, 2021 · 6 comments
Labels
discussion needs feedback and ideas

Comments

@changhe3
Copy link

changhe3 commented Mar 12, 2021

Currently this trait is not very efficient when you want to implement ListIter<(K, V)> on OrdMap<K, V> or HashMap<K, V>, as you have to clone the keys and values each time you call the callback cb.

@changhe3
Copy link
Author

changhe3 commented Mar 12, 2021

I did find this implementation which only implements ListIter<V>, thus not requiring you to clone the value, as a workaround though.

@arthmis
Copy link
Collaborator

arthmis commented Mar 12, 2021

What was the implementation you found?

@changhe3
Copy link
Author

@cmyr
Copy link
Member

cmyr commented Mar 15, 2021

Anything that is part of your Data should be cheap to clone.

I do agree that these types should be extensively reworked or replaced, but I don't think that the cloning, on its own, would be sufficient rationale. :)

@ratmice
Copy link
Collaborator

ratmice commented Dec 28, 2021

Currently this trait is not very efficient when you want to implement ListIter<(K, V)> on OrdMap<K, V> or HashMap<K, V>,

I'm not sure how to link directly to PR comments, but it's worth considering why it doesn't already implement ListIter<(K, V)>, It seems there was an implementation in #1641 which was removed before merge.
The comments there are enlightening, particularly in regards to ListIter mutating keys, which then causes reordering

In the comments on druid/src/widget/list.rs
#1641 (comment)

@xStrom xStrom added the discussion needs feedback and ideas label May 8, 2022
@rbtcollins
Copy link

A closely related thing is this choice: https://github.com/linebender/druid/blob/c02452ddeebc527992e8f112f434f23ce24c934d/druid/src/widget/list.rs#LL129C3-L129C33 - the projection through OrdMap<K,V> only passes V to the interior widget.

e.g.

for some OrdMap<String,String>

 LensWrap::new(
                List::new(|| {
                    Label::dynamic(|data), _| {
                        format!("List item: {:?}", data)
                    })
                }),
                lens_pointing_at_the_collection,
            ),

Now the reason this is a nuisance is that things like toml files that deserialise collections won't include the collection K inside the V, forcing postprocessing after serde to put the data into a form accessible to druid.

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

No branches or pull requests

6 participants