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

lens::Index does not work on arrays #1138

Closed
smmalis37 opened this issue Aug 18, 2020 · 8 comments · Fixed by #1171
Closed

lens::Index does not work on arrays #1138

smmalis37 opened this issue Aug 18, 2020 · 8 comments · Fixed by #1171
Labels
D-Easy just needs to be implemented enhancement adds or requests a new feature

Comments

@smmalis37
Copy link
Contributor

As per the title, when attempting to use a lens::Index on an array value I receive errors about Index and IndexMut not being implemented for the array. This is technically true, as they're only implemented on slices, not arrays. Arrays should coerce into slices, but something in all the generics is preventing that from happening. Using lens::Deref doesn't help here, as this is a real coercion and not a Deref impl. I ended up creating a lens::Field that simply indexes in its two closures, which feels unnecessary but works.

@luleyleo
Copy link
Collaborator

Have you tried the lens! macro? Maybe that would help you.

@smmalis37
Copy link
Contributor Author

smmalis37 commented Aug 18, 2020

I did. Both lens!(Foo, array).index(i) and lens!(Foo, array[i]) gave me various errors.

@luleyleo
Copy link
Collaborator

According to the docs, you should use it somewhat like lens!([u8], [4]), but I've never used it myself. Would be great to have an example of the code causing the error, if that does not work either.

@smmalis37
Copy link
Contributor Author

Here's a minimal example:

use druid::{widget::*, *};

fn main() -> Result<(), PlatformError> {
    AppLauncher::with_window(WindowDesc::new(main_window)).launch(State::default())
}

#[derive(Clone, Default, Data, Lens)]
struct State {
    data: [String; 9],
}

fn main_window() -> impl Widget<State> {
    let mut column = Flex::column();

    for x in 0usize..9 {
        column.add_child(TextBox::new().lens(/* What goes here? */));
    }

    column
}

The following all give errors:

State::data.index(x)
lens!(State, data).index(x)
lens!(State, data[x])
lens!(State, data).then(lens!([String], [x]))

and there's probably others I tried and have forgotten. The only thing I could come up with that worked is

lens::Field::new(move |s| &s.data[x], move |s| &mut s.data[x])

@luleyleo
Copy link
Collaborator

luleyleo commented Aug 18, 2020

After patching the lens! macro to use move closures, this works:

State::data.then(lens!([String; 9], [x])))

Another option would be adding a Ref lens, as counterpart do Deref:

/// `Lens` for invoking `AsRef` and `AsMut` on a type
///
/// See also `LensExt::as_ref`.
#[derive(Debug, Copy, Clone)]
pub struct Ref;

impl<T: ?Sized, U: ?Sized> Lens<T, U> for Ref
where
    T: AsRef<U> + AsMut<U>,
{
    fn with<V, F: FnOnce(&U) -> V>(&self, data: &T, f: F) -> V {
        f(data.as_ref())
    }
    fn with_mut<V, F: FnOnce(&mut U) -> V>(&self, data: &mut T, f: F) -> V {
        f(data.as_mut())
    }
}

Used like this:

State::data.then(lens::Ref).then(lens::Index::new(x))
// or with `LensExt`
State::data.as_ref().index(x)

And the latter seems quite elegant to me.

@luleyleo luleyleo added D-Easy just needs to be implemented enhancement adds or requests a new feature labels Aug 23, 2020
@smmalis37
Copy link
Contributor Author

smmalis37 commented Aug 26, 2020

It's a shame that an as_ref lens is necessary here, because the idea that arrays aren't directly indexable in Rust is just plain weird. However it's true, and I can't think of anything better.

@cmyr
Copy link
Member

cmyr commented Aug 27, 2020

I think the as_ref lens is an excellent solution.

@smmalis37
Copy link
Contributor Author

smmalis37 commented Sep 7, 2020

If I had waited a few releases this might not have been needed lol: rust-lang/rust#74989. I still think its worthwhile to have for other AsRef types though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-Easy just needs to be implemented enhancement adds or requests a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants