8000 Fix/1422 for corner cases. by ened · Pull Request #1427 · androidannotations/androidannotations · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Fix/1422 for corner cases. #1427

Closed
ened wants to merge 1 commit into from
Closed

Fix/1422 for corner cases. #1427

ened wants to merge 1 commit into from

Conversation

@ened
Copy link
Contributor
@ened ened commented May 24, 2015

No description provided.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 24, 2015

I think the first commit is accidental. Also, you have checkstyle violations.

WonderCsabo
WonderCsabo reviewed May 24, 2015
View reviewed changes
...dAnnotations/androidannotations/src/main/java/org/androidannotations/handler/ResHandler.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo May 24, 2015

Choose a reason for hiding this comment

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

Maybe if you just create an if (element.getAnnotation(DrawableRes.class) != null) then call a method and handle all @DrawableRes related things, it would be cleaner and better structured.

@WonderCsabo
Copy link
Member
WonderCsabo 8000 commented May 24, 2015

There is a problem: activityCompilesOnMinSdk21WithoutContextCompat and activityCompilesOnMinSdkLower21CompileSdkHigher21WithoutContextCompat fail, because ActivityIntentBuilder.hasActivityOptionsInFragment returns true, hence generates a call to a version of startActivityForResult which does not exist and a compilation error is raised. However i am not really sure why the test can find that kind of Fragment class in the classpath at all. 😕

@ened ened force-pushed the fix/1422 branch 2 times, most recently from c1f556e to be21184 Compare May 25, 2015 01:55
@ened
Copy link
Contributor Author
ened commented May 25, 2015

Unfortunately I'm not able to reproduce the failing test for activityCompilesOnMinSdk21WithoutContextCompat or activityCompilesOnMinSdkLower21CompileSdkHigher21WithoutContextCompat as mentioned by you.

ened
ened reviewed May 25, 2015
View reviewed changes
...dAnnotations/androidannotations/src/main/java/org/androidannotations/handler/ResHandler.java Outdated
Copy link
Contributor Author
@ened ened May 25, 2015

Choose a reason for hiding this comment

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

Should we inline this method?

Copy link
Member
@WonderCsabo WonderCsabo May 25, 2015

Choose a reason for hiding this comment

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

Nope, this is fine.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015 via email

@ened
Copy link
Contributor Author
ened commented May 25, 2015

No, I'm using TextMate & maven for this. Any chance you can help me out for the Eclipse-testing part, please? ;-)

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015

OK, i found out what is the problem with the classpath. The POM defines 4.1.1.4 version of the Androi 8000 d dependency. However, in our setup, we also add the eclipse-dependencies project to the classpath, which brings android 1.6 dependency, and overrides 4.1.1.4. That is why Eclipse cannot find the newer Activity methods. Maven does, because we do not have the eclipse-dependencies in Maven builds obviously.

Actually the POM also defined the 1.6 version Android dep, but 1b9f70e changed it to 4.1.1.4. I think it was a bad idea, because in our tests, we simulate we do not have Fragment in classpath, then we simulate we have it and add it manually. If we have 4.1.1.4 in the test classpath, we cannot simulate the cases without Fragment, and adding that manually makes no sense. @yDelouis, you are the author of that commit, i think we should revert the version incrementation. That means we have to change some tests, though.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015

@ened i think we should refactor ResHandler in a separate commit. We should make it abstract, and the process method should call an abstract processResource method and pass it idRef etc. Just like AbstractListenerHandler. Then create AnimationResHandler, DrawableResHandler etc, and a DefaultResHandler. Then register the correct classes in AnnotationHandlers. This way we do not mix the different cases into one class as we do now.

WonderCsabo
WonderCsabo reviewed May 25, 2015
View reviewed changes
...dAnnotations/androidannotations/src/main/java/org/androidannotations/handler/ResHandler.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo May 25, 2015

Choose a reason for hiding this comment

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

This is not needed, since ElementFilter.methodsIn() already returns ExecutableElements.

@ened
Copy link
Contributor Author
ened commented May 25, 2015

@WonderCsabo got it.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015

@ened please squash the current two commits.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015

@ened did you read #1427 (comment)?

@ened
Copy link
Contributor Author
ened commented May 25, 2015

@WonderCsabo yes, I'm on it, later tonight hopefully.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 25, 2015

OK, thanks, just add some reactions to let me know you saw it. :)

@ened
Copy link
Contributor Author
ened commented May 25, 2015

@WonderCsabo added the abstraction. Regarding the "missing" unit test, it turns out we have it already, I think: activityCompilesWithRegularDrawable.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 26, 2015

Thanks for creating an abstraction. However, that should have go in a separate commit. Any chance you can split the commit into two?

@WonderCsabo WonderCsabo mentioned this pull request May 26, 2015
Open
@ened
Copy link
Contributor Author
ened commented May 26, 2015

@WonderCsabo Is it a big a deal? I've been re-basing the entire time. Will have to undo that.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 26, 2015

Thanks for all your work on this. I will split it myself, then.

@WonderCsabo
Copy link
Member
WonderCsabo commented May 26, 2015

Merged and rebased as of 1d36b13. Thanks!

@WonderCsabo WonderCsabo added this to the 4.0 milestone May 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ened @WonderCsabo
0