Questions for v2.x

[kingsleyadio]

Hello, I've been reviewing the library, and I must say, I really love the approach it has taken. I'm working with the Kotlin version mostly.

I see that the v2.x branch has improved a lot over the current v1, most importantly:

1. No new object creation is needed anymore vis PaperParcels.wrap
2. Reflection is now totally gone.

However, 1 seems to come with the penalty of more boilerplate. Is there a way, anyway really, we can possibly avoid the explicit CREATOR and writeToParcel hooks.

Is it possible to use @Transient in place of the new @Exclude annotation?

Also, the documentation doesn't mention anything about class and field level type adapters anymore. Are those still supported? Just curious though, as I do not personally use them

Thank you.

[grandstaish]

Hey - thanks for the feedback.

Yeah the boilerplate will be a tough selling point for the library, but without "wrapper" classes the boilerplate is necessary. Currently I'm generating this boilerplate with Intellij Live Templates, which works pretty well. I might look into a reflection-based solution too for users who don't mind the minor perf hit and prefer no boilerplate, but it probably won't be part of the core library (unless everyone disagrees with me, in which case I'll consider switching up the API before release).

About the @Transient thing: I had a user raise an issue that @Transient has other semantics, and wanted another option: https://github.com/grandstaish/paperparcel/issues/60. His reasoning is valid, but it would be nicer to just exclude transient fields like I do in V1. Do you have an alternative opinion to this guy?

No more class/field scopes for adapters. I couldn't think of a valid use case for them, so I removed them. Simplifies the documentation and the code. Maybe I should call this out somewhere.

[kingsleyadio]

Hi. Thanks for the response. Sorry this is coming a little late.

Yeah. With the PaperParcelable interface, boilerplate has actually been drastically reduced. Well, leaving the teeny tiny little CREATOR. With Kotlin, that also means an extra companion object :( Reflection sounds like an option, but I think the current solution (relative to the reflection alternative) is good enough.

For @Transient, I see the point there. I think your suggestion is fine. If the field is already transient, there shouldn't be a need to annotate with @Exclude. So, both can co-exist just fine.

Yes. I agree. Maybe a section in the doc for users migrating from v1.

That said, I hope the v2 release drops soon :)

---

Meanwhile. I've been able to use v1 with both data and normal classes even though the doc didn't mention any explicit support for normal classes. Will that behavior also continue with v2?

Thank you.

[grandstaish]

V1 used reflection for the wrapper class lookup. I have re-added the PaperParcelable interface into V2 (for kotlin users as an optional dependency) using the same mechanism. The cost of this reflection is actually pretty minimal because the reflection is cached.

Good point about @Transient and @Exclude co-existing. I'm still debating whether or not to include @Exclude in the final release though because it is such an edge case, but I'll definitely make sure that @Transient works.

And yep - I should point out in the docs that this isn't just for data classes. Thanks!

[kingsleyadio]

Yes. I agree reflection in this case isn't actually expensive

Regarding @Transient. It is possible I want to exclude a field from persistence but keep it parcelable. And vice versa. So, I'm thinking, maybe we might even need a new @Include annotation for situations where the field is already transient. On that same note, I think @Exclude is fine for situations where I don't want a field to be parcelable, but it should still remain persistable.

Tiny clarification: Do you mean, using PaperParcelable, one can now avoid the CREATOR as well? Or is that just writeToParcel and describeContent as it was earlier?

[grandstaish]

AFAIK we can't avoid writing the CREATOR ourselves because it needs to exist on the actual Parcelable class so it can be invoked via whatever native class-lookup Android is doing. I'll investigate this further though - writing a static field in kotlin is not the nicest looking code.

[grandstaish]

Hey - if you're interested I have a solution to the whole transient/exclude thing. There's an open PR for it, and the explanation/usage can be found here. I think this should cater to everyone since it is consistent with how V1 works (e.g. transient excluded by default), but it is fully configurable. Let me know if you have any concerns before I merge it in 👍

[kingsleyadio]

Ah. Awesome! This addresses the issues quite nicely. I particularly like the excludeFieldsWithAnnotations

However,

1. I don't see a situation where more than a single exclusion annotation could be required. Same reasoning for class level adapters. Maybe a simple excludeFieldsWithAnnotation = Ann.class can be preferred?
2. Can the same approach be applied for inclusion?
3. Personally, I'd love a one-off configuration. Applying on every single model feels overwhelming

[grandstaish]

1. That is a good point. I think I'll change that.
2. Originally I thought that too, but the default option would need to be some kind of catch-all for all annotations, and the API (and implementation) gets a little weird. I decided to stick with how gson does it because it's easy.
3. Hmm, I thought about this, but couldn't think of a nice way to fit it into an API. Do you have an idea for how a one-off config might look?

[grandstaish]

Oh, I remembered why I did "annotations" instead of "annotation" in that API; it's because of the way that default values in java annotations work. The default value cannot be null (it must be a constant value), but it can be an empty array.

Fortunately though, when using the API, you can actually omit the curly braces if you only have one annotation like so:

@PaperParcel(excludeFieldsWithAnnotations = Ann.class)

[kingsleyadio]

1. Okay. That's fine. Except of course, if @Exclude would ship with the library, then, there can be a sensible default
2. If there were a global configuration, that could easily be an opt-in model => everything (with exception of exclusions) is included by default. No need to process any annotation. I might be missing something?
3. Now, given all the magic happens via the annotation processor, I'm thinking we can in fact, have a config annotation (itself annotated for discovery) where all the global configuration happens. Something along the lines of:

@ProcessorAnnotation(
  excludeAnnotations = A.class,
  excludeModifiers = {Modifier.TRANSIENT,...}, ...)
public @interface ConfigAnnotation {}

A possible implementation of @ProcessorAnnotation could be

@Target(ElementType.ANNOTATION_TYPE)
public @interface ProcessorAnnotation {
    Class<? extends Annotation>[] excludeAnnotations() default {};
    int[] excludeModifiers() default {Modifier.TRANSIENT, Modifier.STATIC};
    boolean inclusionsEnabled() default false;
    Class<? extends Annotation>[] includeAnnotation() default {};
}

Does that look like something that can fly?

[grandstaish]

1) If I ship with @Exclude, then I'd consider not even having an excludeFieldsWithAnnotation API because if that is overridden then the @Exclude annotation becomes error prone (e.g. people may use it expecting it to omit data and miss the fact that it doesn't because someone else overrode it). I'll keep thinking about the best way to handle this.

2. In your example, your default "include" annotations list is empty. This translates (in my head at least!) to "nothing is included" (e.g. include no fields at all). The processor itself could make an exception for the empty-list-case and ignore the API if it is an empty list, but that makes the API a little harder to reason about and document.
3. I like where you're going with this. Although annotating an arbitrary annotation would be a little undiscoverable imo. I'm thinking of moving it away from @PaperParcel and moving it into a separate "options" annotation as per your suggestion, but allowing that to be used on both class and package elements. That way you could have all of the models within a single package have the same configuration. Not quite global but this is one of the reasons for package-level annotations. The downside would be that it doesn't work as well for feature-based packaging, when people have their model objects scattered all over the place.

Btw, I'm really appreciating the feedback, thank you!

[kingsleyadio]

1. I think we both agree on this :) Favor excludeFieldsWithAnnotations over @Exclude
2. If the inclusionsEnabled were set to false (as would be by default), the processor should translate this to Include ALL fields that are not in the exclusion rules. Once the field is enabled, the processor should validate that at least one annotation has been set in includeAnnotations which then translates to Include ONLY fields annotated with whatever annotation.... This is similar to excludeFieldsWithoutPackAnnotation except that @Pack doesn't have to be provided by the library
3. My reasoning here is similar to Dagger2 components, where a @Component annotation is applied on an interface that would then help in configuring what modules will make up the object graph. Here, the library provides a @ProcessorAnnotation for example. Whoever needs a global configuration simply applies this on an arbitrary annotation/interface. The processor discovers the annotated annotation/interface, and loads the configuration options

You're welcome. I'm happy to be of help

[grandstaish]

Ahhh, my mistake, I missed the inclusionsEnabled boolean. I agree it would make the API a little more consistent so I'm leaning towards making the change.

For the last point, my issue is mostly around how obvious the API is to discover. It would be pretty convenient to do what you're suggesting if you were writing an app with a small team (or by yourself), but would be less obvious in larger teams. Since the config is tucked away in some random class, it might not make immediate sense to people who pick up the code later why the PaperParcel defaults aren't working.

Having said that, let me suggest another idea that might solve that while keeping config consistent and reusable. It's basically just a tweak of your above suggestion:

1. PaperParcel provides the @ProcessorConfig annotation (name TBC) that you mention above.
2. Create your own annotation @ConfigAnnotation (call it whatever you want) and apply the rules to it as you did above.
3. That annotation can be put on any @PaperParcel class to set the rules:

@ConfigAnnotation
@PaperParcel
public class MyClass implements Parcelable {
  ...
}

Note that @ProcessorConfig (or whatever it'll be called) could also be applied directly to @PaperParcel classes if the user wanted, but this would be a nice way to get re-usability out of a set of rules.

[kingsleyadio]

Awesome! :100:

[kingsleyadio]

I see your point about being discoverable. I was thinking in a different context. I love the idea of reusability here. It makes a whole lot of sense. The user can decide to do a global re-usable configuration, or well, if he so wishes, then to each class its own

[grandstaish]

Thanks for your help again @kingsleyadio. Closing as I've just released the first beta. May not be useful for you until kapt2(3?) stabilizes though.