8000 Added parceler `@Extra` and `@InstanceState` support by johncarl81 · Pull Request #1559 · 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.

Added parceler @Extra and @InstanceState support #1559

Merged
yDelouis merged 4 commits into from
Oct 6, 2015
Merged

Added parceler @Extra and @InstanceState support #1559

yDelouis merged 4 commits into from
Oct 6, 2015

Conversation

@johncarl81
Copy link
Contributor
@johncarl81 johncarl81 commented Sep 19, 2015

This PR offers Parceler integration which wraps and unwraps beans annotated with @Parcel transparently when they are used in @Extra or @InstanceState aspects. This is a rework of this previous PR: #977

dodgex
dodgex reviewed Sep 19, 2015
View reviewed changes
...androidannotations-test/src/main/java/org/androidannotations/test/parceler/ParcelerBean.java
Copy link
Member
@dodgex dodgex Sep 19, 2015

Choose a reason for hiding this comment

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

no newline at end of file

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

Oops.. done.

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

#1560

@johncarl81 johncarl81 force-pushed the parceler branch 2 times, most recently from 8c4d13e to 4849b9c Compare September 19, 2015 06:40
dodgex
dodgex reviewed Sep 19, 2015
View reviewed changes
...roidannotations/src/main/java/org/androidannotations/internal/core/helper/IntentBuilder.java Outdated
Copy link
Member
@dodgex dodgex Sep 19, 2015

Choose a reason for hiding this comment

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

you already added this method in AnnotationHelper any chance to use that one?

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

I certainly could, but I wasn't sure how to get ahold of AnnotationHelper other than making isAnnotatedWith static. What do you suggest?

Copy link
Member
@dodgex dodgex Sep 19, 2015

Choose a reason for hiding this comment

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

I'd pass the AnnotationHelper to the intent builder.

Add it to the constructor, then update the ActivityIntentBuilder and ServiceIntentBuilder to constructors and thier instantiations. where they are instantiated there is annotationHelper field from parent class available.

but i think you should do that in a separate commit before doing the work that adds the parceler support

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

Got it... done.

Copy link
Member
@WonderCsabo WonderCsabo Sep 19, 2015

Choose a reason for hiding this comment

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

I think you still forgot to remove the method from here.

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

It got missed in the rebase.. should be removed now.

WonderCsabo
WonderCsabo reviewed Sep 19, 2015
View reviewed changes
...ations-core/androidannotations/src/main/java/org/androidannotations/helper/BundleHelper.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo Sep 19, 2015

Choose a reason for hiding this comment

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

Parcels.wrap/unwrap method can handle List<AnnotatedWithParcel> AFAIK, or even more. We should also allow those.

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 19, 2015

Choose a reason for hiding this comment

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

Parceler can also handle boxed primitives in collections:
List<Integer>, Map<Integer, String>
A mix:
Map<String, AnnotatedWithParcel>
And nested collections:
Map<List<String>, List<AnnotatedWithParcel>>
Would you like these cases handled as well?

Copy link
Member
@WonderCsabo WonderCsabo Sep 19, 2015

Choose a reason for hiding this comment

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

If you can add it, it would be great. However we should also check that we have Parceler on the classpath (when the user adds a List of primitives for examples, we cannot be sure because we are not checking any annotation).

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 20, 2015

Choose a reason for hiding this comment

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

I've been going with the assumption that if you annotate your bean with @Parcel that would mean Parceler is on the classpath. Perhaps we should just support collections with supported types and at least one bean annotated with @Parcel... would that be acceptable instead of checking explicitly that Parceler is on the classpath?

8000

Copy link
Member
@WonderCsabo WonderCsabo Sep 20, 2015

Choose a reason for hiding this comment

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

@johncarl81 it is very easy to check it is on the classpath: elementUtils.getTypeElement("org.parceler.Parcel") != null. We use this type of check for lots of things throughout AA.

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 20, 2015

Choose a reason for hiding this comment

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

Ok, that will work nicely, I'll move forward with that then.

Copy link
Member
@WonderCsabo WonderCsabo Sep 20, 2015

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 20, 2015

Choose a reason for hiding this comment

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

I'm finding that matching types following the critieria we discussed may be a bit overzelous. For instance, this is matching the ArrayList<String> listExtra in ExtraInjectedActivity. Should we be serializing this through Parceler?

Copy link
Member
@WonderCsabo WonderCsabo Sep 20, 2015

Choose a reason for hiding this comment

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

No, i think we should only serialize those objects through Parceler which are not in plain Android.

Copy link
Contributor Author
@johncarl81 johncarl81 Sep 20, 2015

Choose a reason for hiding this comment

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

Agreed... I think I have a solution, pushing shortly.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 19, 2015

Sorry, but i am not sure the refactor with passing the AnnotationHelper instance around is needed. You could just create a new AnnotationHelper in the intent builder constructor.

annotationHelper = new AnnotationHelper(environment);

@johncarl81 johncarl81 force-pushed the parceler branch 2 times, most recently from 1a3c81a to fdce23e Compare September 19, 2015 15:54
@yDelouis
Copy link
Contributor
yDelouis commented Sep 19, 2015

Could you please update the javadoc of the annotations @Extra, @FragmentArg and @SavedInstanceState to specify that these annotations are compatible with classes annotated with @Parceler ?

@yDelouis
Copy link
Contributor
yDelouis commented Sep 19, 2015

And I think you forgot to handle inner Extra annotations like @OnActivityResult.Extra. You have to modify the class ExtraParameterHandler.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 19, 2015

@yDelouis Those extra annotations are already handled by the BundleHelper.

@yDelouis
Copy link
Contributor
yDelouis commented Sep 20, 2015

@WonderCsabo, you're right, sorry.

@johncarl81 johncarl81 force-pushed the parceler branch 3 times, most recently from 190d955 to 3dba0f3 Compare September 20, 2015 17:13
WonderCsabo
WonderCsabo reviewed Sep 20, 2015
View reviewed changes
...ns-core/androidannotations/src/main/java/org/androidannotations/helper/AnnotationHelper.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo Sep 20, 2015

Choose a reason for hiding this comment

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

Can you move all Parceler-related things to another helper class (which can extend AnnotationHelper)?

Copy link
Member
@WonderCsabo WonderCsabo Sep 20, 2015

Choose a reason for hiding this comment

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

I would move PARCEL_ANNOTATION to CanonicalNameConstants and use that constant everywhere.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 20, 2015

This is OK, however i realized METHOD_SUFFIX_BY_TYPE_NAME is outdated and we also do not support SparceArray<T extends Parcelable>. We have to fix that later.

@johncarl81 johncarl81 force-pushed the parceler branch 5 times, most recently from f208bb5 to b12377d Compare September 20, 2015 22:35
@johncarl81
Copy link
Contributor Author
johncarl81 commented Sep 29, 2015

Ok, let me know if you like that language.

By the way, if you guys would like great lo 9E88 oking javadocs with less ceremony I'd recommend Asciidoclet

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 29, 2015

We also have to add info for:

  • OnActivityResult.Extra
  • Receiver.Extra
  • ReceiverAction.Extra
  • ServiceAction.Extra (actually we already miss parameter type info on that :( )

@johncarl81
Copy link
Contributor Author
johncarl81 commented Sep 29, 2015

I don't see ServiceAction.Extra...

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 29, 2015 via email

@johncarl81
Copy link
Contributor Author
johncarl81 commented Sep 29, 2015

Alright, I think I got them all and rebased against develop... let me know if there's anything else.

8090
@johncarl81
Copy link
Contributor Author
johncarl81 commented Oct 6, 2015

Ping

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 6, 2015

Sorry for not adding feedback, i am very busy at work :(

I think this can be merged. @yDelouis ?

@yDelouis
Copy link
Contributor
yDelouis commented Oct 6, 2015

I think so ! Thanks !

yDelouis added a commit that referenced this pull request Oct 6, 2015
@yDelouis
Merge pull request #1559 from johncarl81/parceler
7cd7718 
Added parceler `@Extra` and `@InstanceState` support
@yDelouis yDelouis merged commit 7cd7718 into androidannotations:develop Oct 6, 2015
@yDelouis yDelouis added this to the 4.0 milestone Oct 6, 2015
@yDelouis yDelouis mentioned this pull request Oct 6, 2015
Closed
@johncarl81 johncarl81 deleted the parceler branch October 6, 2015 16:21
@johncarl81
Copy link
Contributor Author
johncarl81 commented Oct 6, 2015

Fantastic!

Thanks @WonderCsabo for your help on this effort, both on this PR and your attention to Eclipse support in Parceler. Without your diligence this feature would have never reached this point.

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 7, 2015

@johncarl81 i hope i could help, and thank you for implementing this in AA, it was not a quick task.

@yDelouis we have to update the wiki. I think we can add this information in the third party integration section. Is that OK?

@WonderCsabo WonderCsabo mentioned this pull request Oct 7, 2015
Closed
@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 9, 2015

@johncarl81 i created a wiki page for Parceler integration, i hope everything is correct.

@johncarl81
Copy link
Contributor Author
johncarl81 commented Oct 9, 2015

Looks spot on @WonderCsabo.

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 9, 2015 via email

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 14, 2015

@johncarl81 FYI i tried this in Gradle, and the JCodeModel#ref method throwed an NoClassDefFoundError about android.os.Parcelable when you are referencing the Parcels utility class. So i changed to use JCodeModel#directClass instead 1234280.

@johncarl81 1DF8
Copy link
Contributor Author
johncarl81 commented Oct 14, 2015

Ah, good catch.
On Oct 14, 2015 2:56 PM, "Csaba Kozák" notifications@github.com wrote:

@johncarl81 https://github.com/johncarl81 FYI i tried this in Gradle,
and the JCodeModel#ref method throwed an NoClassDefFoundError about
android.os.Parcelable when you are referencing the Parcels utility class.
So i changed to use JCodeModel#directClass instead 1234280
1234280
.

—
Reply to this email directly or view it on GitHub
#1559 (comment)
.

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

None yet

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@johncarl81 @WonderCsabo @yDelouis @dodgex
0