Integrated Parceler into Extras #977
Conversation
|
Unfortunately i cannot build this. I got this error in eclipse and maven, too: I think this is the same bug as transfuse#34. I printed the dependency tree, and it seems the guava dep is indeed the problem: The solution for this is excluding the guava dependency from your Android dependency. |
|
Shoot, I was running into the same thing... thought it wasn't in Parceler. Does AA use Guava? |
|
Oops, yes... It uses Guava 13.0.1. |
|
Ah, I forgot to make the scope |
|
I am confused. Where did you forget the scope? EDIT: I was from mobile, i see now. You have rewritten your commit. :S |
There was a problem hiding this comment.
This is not okay. Not just @Parcel annotated types should use getParcelable but types what are implementing the Parcelable interface, too. This change breaks compilation of SaveInstanceStateActivityand FragmentArguments classes in the test app.
There was a problem hiding this comment.
Are you sure this breaks Parcelable interfaces? This method is called from the second else/if after it checks if the object is a Parcelable. Really all this call will do is set the boolean parcelerBean = true along with methodNameToSave = "put" + "Parcelable";and methodNameToRestore = "get" + "Parcelable";. I am, of course, open to different ways of integrating this stuff. This technique just seemed to fit with AA's existing architecture.
There was a problem hiding this comment.
Just check out this code:
if (isTypeParcelable(elementType)) {
methodNameToSave = "put" + "Parcelable";
methodNameToRestore = "get" + "Parcelable";
}
if (isTypeParcelerBean(elementType)){
methodNameToSave = "put" + "Parcelable";
methodNameToRestore = "get" + "Parcelable";
parcelerBean = true;
}
else {
methodNameToSave = "put" + "Serializable";
methodNameToRestore = "get" + "Serializable";
restoreCallNeedCastStatement = true;
if (hasTypeArguments) {
restoreCallNeedsSuppressWarning = true;
}
}So if there is no @Parcel annotation, always the serializable method will be used. You are missing an else i guess.
This is indeed breaks Parcelables in extras, i compiled the test project that's why i noticed the issue.
There was a problem hiding this comment.
My mistake. Fixed.
|
Unfortunately the 13.0.1 Guava version is still a problem - it does takes precedence over your 15.0, so new |
|
This looks to be a shortcoming of Maven. Would you guys care to upgrade Guava? |
|
By the way, this integration does not take into account beans identified via the |
|
I tried to add the |
|
@yDelouis Are there other generated classes in AA that will take advantage of copying between the target and generated classes? Keep in mind there is a handful of other annotations for Parceler to handle if this is the strategy you want to take. |
|
All non AA annotations will be copied on all generated classes. |
|
Is this PR waiting on this one to be resolved: johncarl81/parceler#30 ? |
|
This seems great.. But I'm sorry, I don't like being dependent on another Android framework (transfuse here). I was thinking about having our own implementation (transfuse free) based on your work :x I'll have to close this one then. |
|
Shoot, I was looking forward to this collaboration. I have a couple of points to clarify: First, Parceler depends on a common codebase as Transfuse: transfuse-bootstrap, transfuse-core, and transfuse-support. This common codebase is not the android specific portion of Transfuse (regular transfuse), however, it is an eat-your-own-dog-food initiative that resulted in a set of generalized libraries for compile-time DI and annotation processor support. Parceler uses these libraries for DI within the annotation processor to solve a number of other common problems with annotation processors. Second, this PR does not introduce Parceler or Parceler's dependencies as a dependency of AA. I was very careful to reference Parceler by String class name ie: codeModel().ref("org.parceler.Parcels")If you do a If you guys change your mind regarding this integration please let me know. Otherwise, I'm happy to help where I can if you run into issues with or have questions about your implementation. Cheers and good luck! |
|
@johncarl81 i think we can revisit this one. It would be nice to have our implementation of I think this PR was rejected, because the it seemed that AA will always need the |
|
@WonderCsabo, ok, sounds good. Since we ended this effort a few other libraries picked up Parceler support. I'd like to highlight FragmentArgs' implementation as it offers further decoupling which may be of interested for AA. FragmentArgs offers an interface to handle any extra serialization concerns. This plays well into how the Parcels utility class wraps / unwraps I imagine we could introduce a similar technique into the @Extra(value = "myParcelerExtra", converter = ParcelerConverter.class) ExampleParceler ep;If you like this technique I'd be happy to work on a PR. Also, I took a lesson from this conversation. From this I ended up shading the dependencies the Parceler processor had on Transfuse and Guava. This cleans up the dependency tree associated with Parceler, eliminates any library incompatibilities (remember we had to upgrade Guava), and removes the concern that Transfuse would be included along with Parceler. Parceler is now simply 2 jars, the Parceler processor and Parceler API. |
|
Well, i am not a real fan of shading, as it has lots of drawbacks, and kicks the dependency management concept in the ass. But it is needed sometimes, and who am i to tell when it is, so i accept your decision. :) I like the idea of decoupling, but i think adding the converter parameter always is inconvenient. A static method should be available to set the default one. I also i think we should still use Parceler for your annotated classes even if the client do not explicitly set the converter. |
|
In that case we could simply more forward with the previous approach that will decorate the get/set calls to the extra with I'd just like to emphasize that the target extra may have a generated @InjectExtra("parcelerExtra") Example e;
public class Example{}
@ParcelClass(Examle.class)
public class SomeOtherClass{}I'd imagine we would error in this case. Not sure if this is a concern for you, but it would be a gap in the integration. WDTY? |
Hmm, i a little bit confused here. Previous is your old implementation in this PR? And decorate is the converter or our get/set? Edit: ok, i guess you mean just the old implementation. About |
|
Yes, the "previous" implementation I am referring to is the old impl in this PR. By decorate Im referring to what Im doing with
I see the problem being that it's hard to detect that a given class is a candidate for wrapping if the target class isn't annotated directly with the Here's the possible implementations I see (and I've seen in the community):
I'm sure there are other approaches. My recommendation is to go with point 3 and secondarily 2. |
|
@johncarl81 yeah, i understood I had an idea: can the same annotation be claimed by two annotation processors? If so, we could process Also please note we copy all non-AA annotations to our generated subclasses, so if two annotation processors cannot claim the same annotation in a round, maybe we could claim it only the first round, and But i think we an handle |
|
@johncarl81 any news on this one? |
|
Sorry, I haven't given this any attention since we last talked. I'll try to get something going soon. |
|
@WonderCsabo, just reread this thread, yes, we could have AA recognize |
|
@johncarl81 we have a bunch of annotations which extract from / add objects to About |
Basic integration of Parcler including transparent serialization/deserialization when used as Extras or InstanceState. I was unable to test this PR fully, so please let me know if anything is out of order or if any changes need to be made.
#301