Make sure non-recurring tasks get an INSTANCE_ORIGINAL_TIME of 0. Imp… #581
Conversation
…lements #577 This resulted in a minor SRP-ish refactoring to separate steps of instance value creation.
|
|
||
|
|
||
| /** | ||
| * Experimental {@link Single} which applies a {@link BiFunction} based on the presence of an {@link Optional}. |
There was a problem hiding this comment.
I wonder whether it's good to create general classes for these kind of special compositions. Not sure if we can give it any name and 'recallable concept', so that 1. we remember that we have something for this 2. it will be clear at the usage point what it does without stepping into it.
What do you think? Isn't it be better to just 'inline' the content of this ctor in Dated? Especially since that is a declared class as well, so we don't 'pollute' higher level code with details.
There was a problem hiding this comment.
I see (at least) two big advantages for putting this into a separate class:
- it's easier to read and understand if it has a proper name
- it's testable in isolation
We can never be sure if we remember already having special classes for the things we implement. That's one of the reason we do peer reviews, It doubles the chance of someone spotting duplicate implementations. If we don't have a special class we'll always have to implement it again. We we have a special class, chances are that we remember having it.
I mean this is something very generic that could be useful in other scenarios as well and so it could go into jems.
There was a problem hiding this comment.
Ok, if you see this as something that can come commonly useful, then of course it makes sense to have this, in jems.
I am not sure about the name, but don't we use Zipped for things that create Pairs?
Maybe using 'fold' or 'reduce'?
There was a problem hiding this comment.
For the time being I keep the name Zipped. We can think of a better one when we move it to jems.
| { | ||
| // add timestamp and sorting | ||
| values.put(timeStampColumn, dateTime1.getTimestamp()); | ||
| values.put(sortingColumn, dateTime1.isAllDay() ? dateTime1.getInstance() : dateTime1.shiftTimeZone(TimeZone.getDefault()).getInstance()); |
There was a problem hiding this comment.
This part seems to be appearing at multiple places already:
dateTime1.isAllDay() ? dateTime1.getInstance() : dateTime1.shiftTimeZone(TimeZone.getDefault()).getInstance()Maybe creating a Single for this in a separate ticket?
| * | ||
| * @author Marten Gajda | ||
| */ | ||
| public final class Enduring implements Single<ContentValues> |
There was a problem hiding this comment.
The word enduring I think has a more particular everyday meaning, but I don't really have a better idea either for an adjective. I suppose you are trying to avoid WithDuration here... because then every class could have that With* name.
I looked up 'lasting' since that sounds a bit better for me, but dictionaries say that refers to something long-lasting as well.
There was a problem hiding this comment.
Initially I was considering Durable but that has a different meaning as well. I would be ok with the current name because it's really just an internal thing.
| * | ||
| * @author Marten Gajda | ||
| */ | ||
| public final class Overridden implements Single<ContentValues> |
There was a problem hiding this comment.
Just that I understand it correctly, this class maybe used when creating instances for an original task - in which case START or DUE is used, or creating more instances of a recurring task - in which case ORIGINAL_TIME is copied. Right?
There was a problem hiding this comment.
I that isRecurring() is checked before using this...
I don't have a clear picture of how recurring tasks will be originated. For example if we changed an existing non-recurring task to recurring, wouldn't this code result in 0 ORIGINAL_TIME for the recurring instance rows?
There was a problem hiding this comment.
ORIGINAL_TIME always refers to the "start" (or "due" in the absence of start) of the original instance as determined by the recurrence rule. For simplicity it can always be 0 for non-recurring tasks. If a task is converted to recurring the 0 value will be overridden with proper timestamps.
The ORIGINAL_TIME identifies the instance that's overridden by an override (there is no other "key" you can use for that, because the override might change the actual time).
So ORIGINAL_TIME is different for each instance of a recurring task. It's not the start of the first instance of a recurring task.
|
|
||
| Single<ContentValues> baseData = new Enduring(new DueDated(effectiveDue, new StartDated(start, new VanillaInstanceData()))); |
There was a problem hiding this comment.
Interesting to see that Singles can be used this way as well, to have this kind of composable factory. Looks nice.
There was a problem hiding this comment.
It's a bit of a stretch because ContentValues is mutable and using Frozen would break this pattern.
…lements #577
This resulted in a minor SRP-ish refactoring to separate steps of instance value creation.