OpenTaskPal module (ContentPal extension for TaskProvider). #421 #422

lemonboston · 2017-09-14T17:57:15Z

Note that these are only the classes and constructors that were needed for the test cases, so this is not a complete ContentPal support for TaskProvider at the moment. I think we can extend it later, for example when test coverage is improved, since test and opentaskpal will be in same project so both can be extended in the same commits.

Also note some TODOs are pending on ContentPal PRs.

lemonboston · 2017-09-14T18:27:07Z

Missed to create NoTimeData that clears start, due, duration, added it now.

dmfs

first pass done.

dmfs · 2017-09-14T20:10:50Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/DueData.java

+{
+    public DueData(@NonNull Long dueTimeStamp)
+    {
+        super(new SimpleRowData<TaskContract.Tasks>(TaskContract.Tasks.DUE, dueTimeStamp));


This should also set START and DURATION to null.

I understand the need to set DURATION to null, but wouldn't this class be used also to just update DUE, keeping START? Or for that with need to use TimeData and retrieve START first?

What if this changes the all-day flag? Start would become invalid in that case. I'd say that this can not be used to update only due.

(addressed)

dmfs · 2017-09-14T20:12:07Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/NoTimeData.java

+                .withValue(TaskContract.Tasks.DTSTART, null)
+                .withValue(TaskContract.Tasks.DUE, null)
+                .withValue(TaskContract.Tasks.DURATION, null);
+    }


A task without times is currently not allowed to recur, so RDATE, RRULE and EXDATE should also be set to null.

Also wipe out all-day and time zone.

dmfs · 2017-09-14T20:14:29Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/DueData.java

+ */
+public final class DueData extends DelegatingRowData<TaskContract.Tasks>
+{
+    public DueData(@NonNull Long dueTimeStamp)


Date-time values should be typed DateTime.

(addressed)

dmfs · 2017-09-14T20:15:47Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TimeZoneData.java

+ *
+ * @author Gabor Keszthelyi
+ */
+public final class TimeZoneData implements RowData<TaskContract.Tasks>


Setting time zone alone doesn't make sense. Since it's part of DateTime it should be set by the other date & time RowData implementations.

Ideally OpenTasksPal provides high-level access to the content provider. Check out SingleEventData in CalendarPal it takes DateTimes and automatically sets timestamps, time zones and all-day flags. Loading or setting a time zone or an all-day flag without loading or setting the timestamp too doesn't make much sense and is considered error prone.

There is only one timezone and all-day flag for tasks, I assume start and due with different timezone should be rejected, right? What about the all-day flag? Take it only from start, or validate if its the same for start and due?
Does it make sense to set all-day flag in DueTime from due?

No different time zones should not be rejected. Actually we should update the provider to support this case. For now just shift start to the due time zone.
But start and due must have the same all-day flag.

(addressed)

lemonboston · 2017-09-15T13:16:22Z

Updated the time RowDatas, please check if they make sense. I would write unit test afterwards.
Also, there is a possibility to use that Equalator interface for the time zone comparison, is it okay to add it here now? (It could go to jems later.)

… accordingly.

lemonboston · 2017-09-15T14:06:20Z

Updated to latest contentpal master and removed all related files from here.

dmfs · 2017-09-15T21:34:30Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TimeData.java

+        }
+
+        DateTime start = mStart;
+        if (mDue != null && !TimeZoneEqualator.INSTANCE.areEqual(mStart, mDue))


There is no need for this Equalator. DateTime is smart enough to return the same instance if the timezone is considered to be the same.

You mean that there is no need to compare the getId() String value but use reference equality on the getTimeZone() TimeZones? Like if (timeZone1 == timeZone2)?
Don't we depend on some undocumented internals of DateTime this way?
I've updated the code, removed the Equalator and added a comment about this behavior, but I think it would be clearer to check the getId() values. TimeZone doesn't provide equals(), so we are really depending on reference equality here which doesn't seem to be safe.
I can do that in private method if you are not consent with that Equalator.
(Although it could be easier to test that code in isolation and ideally just mock it out in (upcoming) TimeZoneTest if it could be injected there, but it couldn't.. so it may not be important.)

No that's not what I meant. I've added a new comment to the new code. The comment above was a bit misleading.

(addressed)

dmfs · 2017-09-18T08:06:23Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/DueData.java

+    public ContentProviderOperation.Builder updatedBuilder(@NonNull TransactionContext transactionContext, @NonNull ContentProviderOperation.Builder builder)
+    {
+        return builder
+                .withValue(TaskContract.Tasks.DUE, mDue.getTimestamp())


Also set allday flag and time zone.

dmfs · 2017-09-18T08:10:23Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/DueData.java

+                .withValue(TaskContract.Tasks.DTSTART, null)
+                .withValue(TaskContract.Tasks.DURATION, null)
+
+                // A task without times is currently not allowed to recur:


but this one has a time!? The point is that we can't update due without updating recurrence because updating due usually also affects recurrence.
For now it's ok to null the recurrence columns because recurrence is not really supported yet. But the reason stated in the comment doesn't apply here.

Yeah, that was copied from Slack, but it probably meant 'recurrence times' originally, should have added that... anyway, I've removed the comment.

dmfs · 2017-09-18T08:12:33Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/OriginalInstanceSyncIdData.java

+ */
+public final class OriginalInstanceSyncIdData extends DelegatingRowData<TaskContract.Tasks>
+{
+    // TODO Should this be @Nullable?


No because you should not be allowed to clear this once it has been created.

Thanks, removed the TODO.

dmfs · 2017-09-18T08:12:51Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/SyncIdData.java

+ */
+public final class SyncIdData extends DelegatingRowData<TaskContract.Tasks>
+{
+    // TODO Should this be @Nullable?


Thanks, removed the TODO.

dmfs · 2017-09-18T08:14:34Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TimeData.java

+
+                .withValue(TaskContract.Tasks.DURATION, duration)
+
+                // A task without times is currently not allowed to recur:


this task has times.

Removed the comment.

dmfs · 2017-09-18T08:15:42Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TitleData.java

+ */
+public final class TitleData extends DelegatingRowData<TaskContract.Tasks>
+{
+    public TitleData(@Nullable CharSequence title)


The title of a task should not be allowed to be null. You may set it to an empty string, but null doesn't make sense.

Ok, changed to @NonNull.

lemonboston · 2017-09-18T13:10:58Z

I see, thanks, I've updated the code. Although, I think we still depend a bit on the knowledge of DateTime internals, for example that as I see it ignores null input, which is good in this case.
But the upcoming unit test of TimeData will check these variations, so I guess it's okay.

…ntial Matcher support.

lemonboston · 2017-09-18T14:32:44Z

Waiting for the ContentProviderOperation.Builder/ RowData Matcher support now from contentpal to write the unit test(s), otherwise ready for review again (all comments addressed).

dmfs · 2017-09-20T09:38:00Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/predicates/TaskOnList.java

+                https://github.com/dmfs/ContentPal/pull/51
+                https://github.com/dmfs/ContentPal/pull/52
+                */
+                new EqArg(TaskContract.Tasks.LIST_ID, taskListRow.values().charData(BaseColumns._ID).value("-1"))));


We'll wait for dmfs/ContentPal#75 to implement this. The current approach is not reliable enough.

#75 has been merged.

Yes, I've replaced this with the ReferringTo(Predicate).

dmfs · 2017-09-20T09:47:47Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tables/TaskListScoped.java

+ * A view onto the {@link TaskContract.Tasks} table which contains only tasks from a specific task list.
+ * Tasks created with {@link #insertOperation(UriParams)} will automatically be added to this task list.
+ * <p>
+ * Note, if you create a {@link View} (using {@link #view(ContentProviderClient, String...)}) with a virtual {@link RowSnapshot}, the {@link View} will always


FYI: I've added dmfs/ContentPal#81 to address this.

dmfs · 2017-09-20T09:50:17Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasklists/NameData.java

+    @Override
+    public ContentProviderOperation.Builder updatedBuilder(@NonNull TransactionContext transactionContext, @NonNull ContentProviderOperation.Builder builder)
+    {
+        return builder.withValue(TaskContract.TaskLists.LIST_NAME, mName.toString());


Why not delegate to a CharSequenceRowData here?

CharSequenceRowData allows null, so NameData wouldn't fail with that. Maybe we need another class for that. ?

Hmm.. I am not sure of the correct approach here actually. We don't validate for null just use the @Nullable @NonNull annotations. Should that be enough in these cases? I mean if there wasn't a .toString() call here, it wouldn't fail either without a validation.
Similar question come up with OriginalInstanceSyncIdData below.

apparently we need another delegate which doesn't allow null. Or make sure CharSequenceRowData doesn't allow null and find another, better solution to "erase" values in the database.

This comes back to the topic of whether to delete or leave a value alone when the incoming update doesn't have it (which may be represented as null). Because that's what makes using Optional instead of the nulls ambiguous as you mentioned before.
I don't know the actual usage in syncadapters.. can the operations maybe be distinguished based on whether they are in 'override' mode or 'fill in the missing data' mode?
If so, could this maybe be a flag? So we could either have an extended Optional model with that flag or pass it in the constructor as separate parameter.

Updated the class with a TODO about this nullability.

dmfs · 2017-09-20T09:52:00Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/OriginalInstanceSyncIdData.java

+ */
+public final class OriginalInstanceSyncIdData extends DelegatingRowData<TaskContract.Tasks>
+{
+    public OriginalInstanceSyncIdData(@NonNull String originalInstanceSyncId)


OriginalInstanceSyncId could also be just a CharSequence. So you probably should delegate to CharSequenceRowData instead.

Updated the class with a TODO about this nullability.

dmfs · 2017-09-20T09:52:33Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/SyncIdData.java

+{
+    public SyncIdData(@NonNull String syncId)
+    {
+        super(new RawRowData<TaskContract.Tasks>(TaskContract.Tasks._SYNC_ID, syncId));


Same here, could be a CharSequence

Updated the class with a TODO about this nullability.

dmfs · 2017-09-20T09:54:37Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TimeData.java

+    }
+
+
+    public TimeData(@NonNull DateTime start, @NonNull String duration)


Duration should be passed as a Duration object not as a String.

dmfs · 2017-09-20T09:55:09Z

opentaskspal/src/main/java/org/dmfs/opentaskspal/tasks/TimeData.java

+    @NonNull
+    private final DateTime mStart;
+    @Nullable
+    private final DateTime mDue;


Maybe make them (mDue and mDuration) Optional instead.

Right, I've changed it. We still use @Nullable at the ctor params? That's what I am not sure of, I don't see the end usage - is it going to be part of an 'automatic' syncronization mapping where it is good if 'null' can be passed on, or is the 'normal' usage, like how I would use the class from UI code is what we should aim for? In this latter case, I would put Optional in the constructor as well.

No, we should not use null at all here.

it should look like this:

private TimeData(@NonNull DateTime start, @NonNull Optional<DateTime> due, @NonNull Optional<Duration> duration) { mStart = start; mDue = due; mDuration = duration; } public TimeData(@NonNull DateTime start, @NonNull DateTime due) { this(start, new Present<>(due), absent()); } …

Ok, updated.

dmfs

Apart from a new nitpicks it looks ok.

lemonboston · 2017-09-25T15:46:04Z

@dmfs Could you please help a little with this?:
Can there be a nullpointer here?

withValue(TaskContract.Tasks.TZ, start.isAllDay() ? "UTC" : start.getTimeZone().getID())

I am not sure of the relationship between all day and the timezones and the ways DateTime can be constructed... can getTimeZone() return null here?

lemonboston · 2017-09-25T17:24:15Z

About the DateTime timezone<->all-day flag question, I figured they are mutually exclusive, right?

I've update to latest contentpal and added unit tests. Let me know if I should squash.
The only outstanding TODOs are about the nullability, otherwise ready for review.

dmfs · 2017-09-27T07:24:10Z

Technically you could have a non-allday task without time zone. That's called floating time. But OpenTasks doesn't really support that so it's ok for now. Maybe create an issue to remind us about this.

Same goes for the nullable. We'll keep it like this and revisit later on when we may have a better idea of how to handle it.

lemonboston · 2017-09-28T11:52:15Z

Ok, I've created the tickets: #431, #432

dmfs

Let's go with this and see where it takes us. We still can adjust things if it turns out they don't work well.

OpenTaskPal module (ContentPal extension for TaskProvider). #421

f73d3c5

lemonboston assigned dmfs Sep 14, 2017

lemonboston requested a review from dmfs September 14, 2017 17:57

Add NoTimeData.

1e1dcd5

dmfs requested changes Sep 14, 2017

View reviewed changes

Update datetime RowData-s.

ce7903a

Update to latest master version of contentpal, remove files from here…

6a43656

… accordingly.

Gabor Keszthelyi added 4 commits September 15, 2017 17:16

Fix DateTime usage in TimeData.

0a9476d

Remove TimeZoneData.

face62e

Fix all-day flag value in TimeData.

0447ed2

Extract time zone comparision to Equalator implementation.

d878351

lemonboston mentioned this pull request Sep 15, 2017

Tests for TaskProvider #410 #418

Merged

dmfs reviewed Sep 15, 2017

View reviewed changes

dmfs reviewed Sep 18, 2017

View reviewed changes

Gabor Keszthelyi added 3 commits September 18, 2017 14:45

Update time zone comparison in TimeData, remove the Equalator.

1a9dc5d

Update RowData-s based on the code review.

47594ff

Update time zone matching code in TimeData.

c721751

Gabor Keszthelyi added 2 commits September 18, 2017 15:59

Created unit test class for TimeData, put on hold as waiting for pote…

717c356

…ntial Matcher support.

Clear all time related values in NoTimeData.

8ebbc01

dmfs reviewed Sep 20, 2017

View reviewed changes

dmfs requested changes Sep 20, 2017

View reviewed changes

Gabor Keszthelyi added 5 commits September 20, 2017 19:23

Update TimeData to use Duration instead of String.

13d773b

Use Optional in TimeData instead of Nullable fields.

6cf1e95

Update TimeData to not use @nullable ctor parameters.

87031e8

Update RowData classes to use CharSequenceRowData.

c35782c

Update to latest contentpal.

a915f7f

Gabor Keszthelyi added 5 commits September 25, 2017 17:48

Unit test for TimeData.

0fd21e5

Unit test for DueData.

9367d02

Add more RowData unit tests.

3c46f7b

Use new ReferringTo Predicate in TaskOnList.

30f792a

Javadocs and NameDataTest.

182946c

This was referenced Sep 28, 2017

Revisit floating DateTime handling in OpenTaskPal #431

Open

Revisit @NonNull RowData input parameters handling in OpenTaskPal #432

Open

dmfs mentioned this pull request Oct 4, 2017

[feature] Possibility of handling repeating tasks? #434

Closed

dmfs approved these changes Oct 5, 2017

View reviewed changes

dmfs merged commit c6f10f2 into master Oct 5, 2017

dmfs deleted the issues/421-opentaskpal branch October 6, 2017 13:30


		.withValue(TaskContract.Tasks.DURATION, duration)

		// A task without times is currently not allowed to recur:

		}


		public TimeData(@NonNull DateTime start, @NonNull String duration)

OpenTaskPal module (ContentPal extension for TaskProvider). #421 #422

OpenTaskPal module (ContentPal extension for TaskProvider). #421 #422

Conversation

lemonboston commented Sep 14, 2017

lemonboston commented Sep 14, 2017

dmfs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonboston commented Sep 15, 2017

lemonboston commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonboston commented Sep 18, 2017

lemonboston commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfs left a comment

Choose a reason for hiding this comment

lemonboston commented Sep 25, 2017 • edited Loading

lemonboston commented Sep 25, 2017

dmfs commented Sep 27, 2017

lemonboston commented Sep 28, 2017 • edited Loading

dmfs left a comment

Choose a reason for hiding this comment

lemonboston commented Sep 25, 2017 •

edited

Loading

lemonboston commented Sep 28, 2017 •

edited

Loading