-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update FieldAdapter with Nullable/NonNull annotations. Fix code in im… #649
base: master
Are you sure you want to change the base?
Conversation
…plementations where it was needed. #644
{ | ||
values.put(mFieldName, value ? 1 : 0); | ||
values.put(mFieldName, new BooleanBinaryLong(value).value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I started to use Long
instead of String
for 0/1 booleans is that I saw that in SQLite these booleans are actually stored as number. I just looked up again:
https://www.sqlite.org/datatype3.html#boolean_datatype
I see integer can be 8 bytes there, so java long would cover it, but for this 0/1, java integer could fit as well, I suppose.
This also relates to the type I've introduce in the other pull request:
opentasks/opentaskspal/src/main/java/org/dmfs/opentaskspal/datetimefields/DateTimeFields.java
Lines 31 to 54 in e35047c
public interface DateTimeFields | |
{ | |
/** | |
* The timestamp. ({@code null} if it's empty) | |
*/ | |
@Nullable | |
Long timestamp(); | |
/** | |
* Time zone id. ({@code null} if it's empty) | |
*/ | |
@Nullable | |
String timeZoneId(); | |
/** | |
* All-day flag of the date-time. 1 for true, 0 for false. | |
* <p> | |
* ({@code null} if it's empty, which is always interpreted as 0-false) | |
*/ | |
@Nullable | |
Long isAllDay(); | |
} |
So the all-day flag is represented as
Long
here as well, as its raw value.
So in order to keep this consistent, I chose Long
here, too. Although, I remember that some container types only store everything in String
(Which type was that? I forgot. ) which may mean a double conversion. So it's not that obvious what's best.
But the idea again was to use the raw type and keep it consistent for simplicity.
What do you think?
We could certainly change to Integer
at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could even use Short
. (I've never used that type before..:) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short
is not supported by all containers, so that's not good. Integer
is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to use Integer
.
…plementations where it was needed. #644