Skip to content
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

Let instance view return instance DTSTART and DUE rather than the tas… #620

Merged
merged 5 commits into from Jan 9, 2018

Conversation

dmfs
Copy link
Owner

@dmfs dmfs commented Jan 8, 2018

…k values. Implements #615

This commit improves the instances view by returning the instance start and due times as task DTSTART and DUE. This way the UI will see the correct instance times.

From the UI perspective the instances view is now a list of individual non-recurring instances, which can be read and written on its own.

Note, for non-recurring tasks, this won't have any effect.

…k values. Implements #615

This commit improves the instances view by returning the instance start and due times as task `DTSTART` and `DUE`. This way the UI will see the correct instance times.

From the UI perspective the instances view is now a list of individual non-recurring instances, which can be read and written on its own.

Note, for non-recurring tasks, this won't have any effect.
@dmfs dmfs requested a review from lemonboston January 8, 2018 10:12
* Note: The {@link #DTSTART}, {@link #DUE} values of instances of recurring tasks represent the actual instance values, i.e. they are different for each
* instance ({@link #DURATION} is always {@code null}).
* <p>
* Also, none of the instances are recurring themselves, so {@link #RRULE}, {@link #RDATE} and {@link #EXDATE} are always {@code null}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to consider not using all of the TaskColumns in Instances here?
Since DURATION, RRULE, RDATE, EXDATE are always null.
Having something like a CommonTaskColumns?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the instances table should be compatible with the tasks table, i.e. instances are special case of a task. In particular, a projection that works on the task table should work on the instances table as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

+ Tables.LISTS + "." + Tasks.LIST_COLOR + ", "
+ Tables.LISTS + "." + Tasks.VISIBLE
+ " FROM " + Tables.TASKS
+ " JOIN " + Tables.LISTS + " ON (" + Tables.TASKS + "." + TaskContract.Tasks.LIST_ID + "=" + Tables.LISTS + "." + TaskContract.Tasks._ID + ")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of the first join, shouldn't TaskContract.Tasks._ID be TaskContract.TaskLists._ID instead? I know it's effectively the same, but just for clarity.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll fix that.

@@ -79,7 +80,14 @@ public InstanceTestData(@NonNull Optional<DateTime> instanceStart, @NonNull Opti
.withValue(TaskContract.Instances.INSTANCE_DURATION,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Instances.INSTANCE_DURATION column need at all if it is always DUE which is used?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instances.INSTANCE_DURATION was added for two reasons:

  • provide an easy way to the UI to get the actual absolute duration of an instance
  • provide an easy way to filter instances by their duration (e.g. in an SQL query)

both could certainly achieved with Instances.INSTANCE_START and Instances.INSTANCE_DUE, but considering that these might also be null (which would be returned as 0 by Cursor.getLong(int)), it would require more work work to achieve that without a duration column.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I realized that I mistook the override in the VIEW, I though it was INSTANCE_DURATION that is nulled but that's Tasks.DURATION... Ok, I see it then why this is needed.

@dmfs
Copy link
Owner Author

dmfs commented Jan 8, 2018

@lemonboston I've fixed the _ID column and also mapped the ORIGINAL_INSTANCE_TIME value to the actual instance value. This is a bit more complicated because the instances table contains 0 instead of null for absent values.

@lemonboston
Copy link
Contributor

And what is the reason for Instances table using 0 instead of null for ORIGINAL_INSTANCES_TIME?

// override task due and start times with the instance values
+ Tables.INSTANCES + "." + TaskContract.Instances.INSTANCE_START + " as " + Tasks.DTSTART + ", "
+ Tables.INSTANCES + "." + TaskContract.Instances.INSTANCE_DUE + " as " + Tasks.DUE + ", "
// also replace ORIGINAL_INSTANCE_TIME.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it needed to replace ORIGINAL_INSTANCE_TIME? Doesn't it break the specification for that column on the contract? Its value will be different from what the docs says there, if I am not mistaken.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No not really. ORIGINAL_INSTANCE_TIME is the time at which an instance occurs as per the specification in the master. The instances view represents a flattened view on the tasks, where each task instance is represented as a non-recurring override. Or in other words, it "emulates" an override for each instance of every (recurring or non-recurring) task.
So to me it makes sense to also "emulate" ORIGINAL_INSTANCE_TIME if present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a bit confusing for me. Could you please remind me why is the Instances.INSTANCE_ORIGINAL_TIME column needed?

The docs for the two columns talk about almost the same things but with a bit different wording. Could it be maybe updated to be phrased the same? It could be easier to see the difference.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using Instances.INSTANCE_ORIGINAL_TIME primarily as a sorting key for all instanced of a recurring task. Without it I can't sort regular instances and overrides in the correct order. Correct sorting is very helpful when syncing the instances specified in the master with the instances in the database.
Also, with this column it is easier to handle recurring tasks without a start. In this case we assume start == due and store that as the INSTANCE_ORIGINAL_TIME. Without it, we would have to check at usage time which value we need to use, start or due (because we still store null in the start column).

@dmfs
Copy link
Owner Author

dmfs commented Jan 8, 2018

The instances table uses 0 because that's easier to compare, no need to deal with null in Java code.

@dmfs
Copy link
Owner Author

dmfs commented Jan 8, 2018

Hmm... there seems to be only one place where it actually matters, I guess I can drop this requirement.

@dmfs
Copy link
Owner Author

dmfs commented Jan 8, 2018

@lemonboston I dropped the requirement of using 0 for INSTANCE_ORIGINAL_TIME of non-recurring tasks and simplified that view column.

Simplyfy instance view `ORIGINAL_INSTANCE_TIME`
@dmfs dmfs force-pushed the stories/615-instance-start-due branch from c058759 to 768a7b5 Compare January 9, 2018 09:26
@dmfs
Copy link
Owner Author

dmfs commented Jan 9, 2018

@lemonboston I've also added some JavaDoc to the Instances contract to explain how the operations will affect the tasks. The insert behavior is not fully implemented yet, at least not in this branch. I'll post a separate PR to fix this.

@lemonboston
Copy link
Contributor

Those javadocs for the Instances table look very useful.

I've checked the changes, no more questions from me besides the one about the ORIGINAL_TIMEs.

Copy link
Contributor

@lemonboston lemonboston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing the purpose of ORIGINAL_INSTANCE_TIME column on Slack, it is now clearer for me.
Regarding looking at every row in Instances table as overrides, would it make sense to add this note, too, to the Instances contact javadoc? Perhaps mentioning INSTANCE_ORIGINAL_TIME explicitly?

@dmfs dmfs merged commit 76dbaa7 into master Jan 9, 2018
@dmfs dmfs deleted the stories/615-instance-start-due branch January 9, 2018 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants