dropbox / djinni

A tool for generating cross-language type declarations and interface bindings.
Apache License 2.0
2.88k stars 487 forks source link

Better parcelable code generation #393

Closed paulocoutinhox closed 6 years ago

paulocoutinhox commented 6 years ago

Hi,

I want suggest some enhancements to parcelable code generation, based on problems that i get today using the native library.

DATE FIELD (ex: expire date is a Date obj):

// write
dest.writeLong(this.mExpireDate != null ? this.mExpireDate.getTime() : -1);

// read
long tmpMExpireDate = in.readLong();
this.mExpireDate = tmpMExpireDate == -1 ? new Date() : new Date(tmpMExpireDate);

TYPED ARRAY LIST (ex: libraries is a list of product library):

this.mLibraries = in.createTypedArrayList(ProductLibrary.CREATOR);
4brunu commented 6 years ago

Hey,

Can you please describe the problems you faced with the current implementation? Did you found a case where it doesn't work?

Regarding the DATE FIELD, bellow is an example of the current generated code for a date and an optional<date>.

// write
out.writeLong(this.mDate.getTime());

if (this.mOptionalDate != null) {
    out.writeByte((byte)1);
    out.writeLong(this.mOptionalDate.getTime());
} else {
    out.writeByte((byte)0);
}

// read
this.mDate = new Date(in.readLong());

if (in.readByte() == 0) {
    this.mOptionalDate = null;
} else {
    this.mOptionalDate = new Date(in.readLong());
}

I think we could adopt you suggestion for non optional dates, just for safety reasons, although if the date is not optional, it should always exists. But I would suggest one change, if the date is null, after being serialised it should continue to be null, and the your suggestion it will end up with the current date. So I suggest returning null if the value is -1. What do you think?

// write
dest.writeLong(this.mExpireDate != null ? this.mExpireDate.getTime() : -1);

// read
long tmpMExpireDate = in.readLong();
this.mExpireDate = tmpMExpireDate == -1 ? null : new Date(tmpMExpireDate);

Regarding the TYPED ARRAY LIST, initially the generated code was like your suggestion, but I think it was changed to support all the types, including an array of dictionary. Bellow is an example of the current generated code.

// write
out.writeList(this.mList);

// Read
this.mList = new ArrayList<String>();
in.readList(this.mList, getClass().getClassLoader());
paulocoutinhox commented 6 years ago

Hi,

You are correct. So i suggest:

DATE FIELD:

new Date(0) // Thu Jan 01 00:00:00 UTC 1970

Djinni dont work with null objects.

TYPED ARRAY LIST:

Ok, correct. I test it here. List of hashmap need use writeList and readList.
4brunu commented 6 years ago

Hey,

If you want to have a Date that can be null, you should make it nullable by declaring it optional<date> in the djinni file, that way, djinni works with null objects.

paulocoutinhox commented 6 years ago

Ok. I will close it for now.