Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Conversation

@shiraji
Copy link
Contributor

@shiraji shiraji commented Nov 22, 2015

see #1568

@shiraji
Copy link
Contributor Author

shiraji commented Nov 23, 2015

This is ready for review.

@WonderCsabo
Copy link
Member

This is very great work. Congrats!

Please note that addOnPageChangeListener is a "new" method. Previously only setOnPageChangeListener was present. So you have to check that which method is available, and use setOnPageChangeListener if addOnPageChangeListener is not present.

Also you should validate that ViewPager class is present or not.

We definitely should add functional test for this. You should check out the SeekBar test for example. AFAIK there is not way to access the listener in ViewPager directly or though a shadow class in Robolectric, so you should retrieve it by reflection (using FEST-Reflect).

@WonderCsabo
Copy link
Member

We also did not have the feature that finding the parameter by the name. I think we should not use that. Instead, we should define an order of the parameters when finding the parameter by type is ambiguous.

That way you could also use our simple ValidatorParameterHelper API. See an example here. You should also use that on all new annotation validations in this PR.

@shiraji
Copy link
Contributor Author

shiraji commented Nov 23, 2015

For finding the method exist or not, do you have any sample case for this?
Validating if ViewPager is present or not seems not that hard. It will be like this.

getProcessingEnvironment().getElementUtils().getTypeElement(CanonicalNameConstants.VIEW_PAGER) != null;

I'll take a look SeekBar for functional tests.

ValidatorParameterHelper looks nice! Let me use that.

For @PageScrolled, this would be the candidate order of parameters. ViewPager is the first parameter and rest is onPageScrolled parameter order.

        validatorHelper.param.inOrder() //
                .type(CanonicalNameConstants.VIEW_PAGER).optional() //
                .primitiveOrWrapper(TypeKind.INT).optional() //
                .primitiveOrWrapper(TypeKind.FLOAT).optional() //
                .primitiveOrWrapper(TypeKind.INT).optional() //
                .validate((ExecutableElement) element, validation);

What do you think?

@WonderCsabo
Copy link
Member

For checking a method is avaible, you can use similar code to this.

The rest is okay!

@shiraji
Copy link
Contributor Author

shiraji commented Nov 23, 2015

You are fast! Thanks for your help.

@shiraji shiraji changed the title Added page change [WIP] Added page change Nov 23, 2015
@shiraji shiraji changed the title [WIP] Added page change Added page change Nov 26, 2015
@shiraji
Copy link
Contributor Author

shiraji commented Nov 26, 2015

Now, it ready for reviewing, again.

I will go to trip a few days where I am not available for the Internet. Until I will come back from there, could you wait for merging? I will squash commits.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add more test cases (when not all parameters are present)? You can use the same variables because the a new Activity is created for each test.

@WonderCsabo
Copy link
Member

LGTM in general, i added some comments. Thanks!

@shiraji
Copy link
Contributor Author

shiraji commented Nov 26, 2015

Thanks! I will fix then as soon as I come back.

@shiraji
Copy link
Contributor Author

shiraji commented Nov 30, 2015

Fixed it. It's ready for review, again.

@WonderCsabo
Copy link
Member

Looks good to me. Please squash the commits and i can merge.

@shiraji
Copy link
Contributor Author

shiraji commented Dec 1, 2015

Squashed

WonderCsabo added a commit that referenced this pull request Dec 1, 2015
@WonderCsabo WonderCsabo merged commit 100aa9c into androidannotations:develop Dec 1, 2015
@WonderCsabo WonderCsabo added this to the 4.0 milestone Dec 1, 2015
@WonderCsabo
Copy link
Member

Thanks, great PR! Would you mind updating the wiki as well?

@shiraji
Copy link
Contributor Author

shiraji commented Dec 1, 2015

No problem! I will let you know as soon as I update wiki.

@shiraji shiraji deleted the 1568_add_page_change branch December 1, 2015 13:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants