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

Added Enum to ViewGroup derive macro #21

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

StefanBossbaly
Copy link
Contributor

First cut of the Enum Macro.

Currently it works on named and unamed enum variants. Each field must implement View + Drawable. I included an example that use a random number generator (tinyrng to stick to the no-std depandancies) to pick what variant should be displayed to prove that any can be selected.

One of the problems I am currently running into is that it does now work with LinearLayout. The problem is that LinearLayout does not implement Clone + Copy which causes the impl Transform to fail since the translate is given a &self and usally calls translate which accepts a mut self. Have to see if there is another way to implement it but it seems like I will need to add Clone + Copy to LinearLayout (which in turn means it has to be added to Views and Chain). Not sure if that is doable on Views since it holds a &'a mut [T] . Any feedback/suggestions is greatly appericated. Thanks.

@StefanBossbaly StefanBossbaly marked this pull request as draft July 1, 2023 04:13
@bugadani
Copy link
Owner

bugadani commented Jul 1, 2023

Thanks for working on this!

I think we shouldn't introduce a new macro and instead we should include enums in ViewGroup. Not quite the same thing, but I just find it a bit weird to need two separate macros to essentially turn complex types into something workable. Also some of it overlaps when you have an enum variant with multiple views in it.

I'm not sure how I feel about Clone. I would like to avoid that if possible, but I need to read this PR first.

Lastly, you don't need randomness and 5 enum variants in the example. I would suggest to keep it simple and easy to read, as it serves as a guide for users.

)
}
Fields::Unit => {
panic!("Can't have unit enum. Contents of enum must impl View + Drawable")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it might make sense to support units, I think, in case you don't want to display anything for a particular case. Is there any difficulty here besides generating code that does nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would units just return a dummy view? I know that ViewGroups must have a length of at least 1 or the library will crash. Is there a View struct that currently exists is just a placeholder/dummy? Since we are returning a ref how would we keep that value alive for the lifetime of the enum? I would imagine it would look something like this

impl embedded_layout::view_group::ViewGroup for ... {
    fn len(&self) -> usize {
        match self {
            ...
            Self::UnitEnum => 1,
        }
    }

    fn at(&self, index: usize) -> &dyn embedded_layout::View {
        match self {
            ...
            Self::UnitEnum => {
                match index {
                    1 =>  // What do we return here, a dummy view?
                    _ => panic!(),
                }
            }
        }
    }

    fn at_mut(&mut self, index: usize) -> &mut dyn embedded_layout::View {
        match self {
            ...
            Self::UnitEnum => {
                match index {
                    1 =>  // What do we return here, a dummy view?
                    _ => panic!(),
                }
            }
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bugadani I didn't address this in the code yet. Was waiting to see what you had to say about how to implement a view that has a no-op draw function.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or the library will crash

Oh nevermind then, I'll do this later.

Copy link
Owner

@bugadani bugadani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to implementing Clone for LinearLayout, but it looks like it's not necessary for this PR. We can keep it, but ideally it would be nicer to add it in a different PR.

Self::#variant_name { #(#field_idents,)* } => {
match index {
#(#fields_index)*
_ => panic!("Invalid index!"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to pick a panic message that is a bit more informative. Also, this message should be:

  • generated as an #[inline(never)] function to avoid including the string multiple times
  • it should be the same as the message for Unnamed variants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StefanBossbaly much better, thanks! This comment and the one about unit enums still stand, though. I can do these changes if you want me to, let me know if you do :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bugadani This comment should be addressed. Both at() and at_mut() are now #[inline(never)] and return the same message (Invalid index! Index exceeds the value returned by len())

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think I communicated very well. But never mind, I don't want to force you to a lengthy back-and-forth on this.

macros/src/lib.rs Outdated Show resolved Hide resolved
src/layout/linear/mod.rs Outdated Show resolved Hide resolved
@bugadani bugadani linked an issue Jul 1, 2023 that may be closed by this pull request
Currently works for both Named and Unamed structures. Right now all
fields have to implement View + Drawable. In addition they have to
implement Clone + Copy otherwise Transform impl will fail since
translate will attempt to move out a value behind a shared reference.
This makes it currently incompatible with LinerarLayout.

Fixes bugadani#20
@StefanBossbaly
Copy link
Contributor Author

I think we shouldn't introduce a new macro and instead we should include enums in ViewGroup. Not quite the same thing, but I just find it a bit weird to need two separate macros to essentially turn complex types into something workable. Also some of it overlaps when you have an enum variant with multiple views in it.

Move code into the current ViewGroup macro.

Lastly, you don't need randomness and 5 enum variants in the example. I would suggest to keep it simple and easy to read, as it serves as a guide for users.

Removed RNG. Sorry got a bit carried away 😄

I'm not opposed to implementing Clone for LinearLayout, but it looks like it's not necessary for this PR. We can keep it, but ideally it would be nicer to add it in a different PR.

Moved.

@StefanBossbaly StefanBossbaly marked this pull request as ready for review July 1, 2023 15:39
@StefanBossbaly StefanBossbaly changed the title First cut of the Enum Macro Added Enum to ViewGroup derive macro Jul 1, 2023
@bugadani bugadani merged commit 4872070 into bugadani:master Jul 1, 2023
5 checks passed
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.

Add better support for dynamic view groups
2 participants