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

Conversation

@naixx
Copy link
Contributor

@naixx naixx commented Jan 16, 2013

Here is a not finished yet PR for @FocusChange.
I've made several changes: extracted abstract processor, which generates the same code for @Click, @LongClick, @Touch. Everything that has same functionality as (T extends View).setOn***Listener() can use this, as well as @ChechedChange and other annotations.
I've also extracted everything that validates parameters to the separate class(it was i bit difficult to navigate, now everything is in one place). Concerning this, was it good idea to use public final field?

This PR also should eliminate stackoverflow problem, that @Touch had, when onTouch() was annotated.

There is no javadoc yet. Any feedback is welcome :)

@CheckedChange added

Copy link
Contributor

Choose a reason for hiding this comment

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

zeroOrOneMenuItemParameters should be renamed to zeroOrOneMenuItemParameter

@naixx
Copy link
Contributor Author

naixx commented Jan 18, 2013

@mathieuboniface please, check :)

@mathieuboniface
Copy link
Contributor

Ok, i'm starting a full review.

Github is saying "This pull request cannot be automatically merged." can you fix this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you test that this method is called when the checked state is changed (CompundButton.setChecked(boolean)) ?
You could checkout ClicksHandledActivity from the functional-test-1-5 module and ClicksHandledActivityTest from the functional-test-1-5-tests module as example.

@mathieuboniface
Copy link
Contributor

👏

Really great work !

We just need more tests on @CheckChange and @FocusChange

Thank you @naixx :)

@pyricau
Copy link
Contributor

pyricau commented Feb 27, 2013

Who's supposed to write the missing tests ?

By the way, I guess master has moved too far: This pull request cannot be automatically merged.

@naixx
Copy link
Contributor Author

naixx commented Mar 3, 2013

Hi! I can take a look in about a week

@pyricau
Copy link
Contributor

pyricau commented Mar 3, 2013

Thanks. Sorry I haven't been involved in this pull request, so I'd rather let @mathieuboniface finish this with you :)

@mathieuboniface
Copy link
Contributor

Hi @naixx,

I will work on merging that PR next week.

Have you worked on the tests ? If not, I can wrote those tests.

…istener_processor

Conflicts:
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/AndroidAnnotationProcessor.java
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/helper/ValidatorHelper.java
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/processing/ClickProcessor.java
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/processing/LongClickProcessor.java
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/processing/TouchProcessor.java
@naixx
Copy link
Contributor Author

naixx commented Mar 29, 2013

Hello! I've just merged develop and resolved some conflicts. It will be easier now. But it fails some tests when I run mvn install

@mathieuboniface
Copy link
Contributor

Hi Rodolph, thanks for trying to help on merging :)

I just merged your work into develop except the commit representing the merge of develop into your branch (#b8bf822) because it's really huge and so hard to understand.

mathieuboniface added a commit that referenced this pull request Apr 13, 2013
@mathieuboniface
Copy link
Contributor

Woow, the this merge was not so nice... I fixed this on 7a83417

@naixx
Copy link
Contributor Author

naixx commented Apr 13, 2013

@mathieuboniface Thanks, great!
Actually concerning b8bf822, overall diff could be rather small

@naixx naixx deleted the common_listener_processor branch April 13, 2013 23:47
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.

3 participants