Skip to content

Removed casting of inflated layout to ViewGroup #307

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

Merged
merged 1 commit into from
Oct 7, 2016

Conversation

MaciejCiemiega
Copy link
Contributor

Hello,
in current implementation this cast prevents from inflating just simple TextView as a list item (such as android.R.layout.simple_list_item_1 etc.).

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@MaciejCiemiega
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@samtstern
Copy link
Contributor

@puf I remember a while ago you explained why it had to be this way. Is that still the case? If so could you enlighten us?

@puf
Copy link
Contributor

puf commented Sep 16, 2016

@samtstern that was in #96 . But I'm wondering if that case wasn't different, since here the View/Viewgroup seem local to onCreateViewHolder()

@MaciejCiemiega
Copy link
Contributor Author

It looks like it was like that since the beginning:

ViewGroup view = (ViewGroup) LayoutInflater.from(parent.getContext()).inflate(mModelLayout, parent, false);

@samtstern
Copy link
Contributor

@puf I suggest merging this one, we immediately do this:

Constructor<VH> constructor = mViewHolderClass.getConstructor(View.class);

Which means we are passing the ViewGroup to a method that takes a View, and we don't use the ViewGroup for any other purpose so I see no reason not to relax this.

@puf
Copy link
Contributor

puf commented Oct 7, 2016

I've been starting at this one for a while now, but can't remember why it seemed a bad idea before. The ViewGroup doesn't even escape from the method. Maybe that was another change after all. @samtstern: can you merge?

@puf puf assigned samtstern and unassigned puf Oct 7, 2016
@samtstern samtstern merged commit a86fdb5 into firebase:master Oct 7, 2016
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.

4 participants