New constraints and defaults (forbid DUE=START)

[korelstar]

• CustomizedDefaultFieldAdapter: Introduce customizable default values (new interface Default) in order to provide different defaults for DUE (should be after START: new class DefaultAfter) and START (should be before DUE: new class DefaultBefore).
• New constraint After: requires that a value is after another value (e.g. DUE must be after START); if constraint is violated, set value to the default (DefaultAfter); similar to the old NotBefore.
• New constraint BeforeOrShiftTime: requires that a value is before another value (e.g. START must be before DUE); if constraint is violated, then the reference value (e.g. DUE) is shifted with respect to the duration of the task.

With the two new constraints, we prohibit DUE=START and therefore fix #262. The constraint BeforeOrShiftTime is a tradeoff between shifting always (cp. constraint ShiftTime) and shifting never (cp. constraint NotAfter); maybe this is sufficient in order to close #197.

[korelstar]

@dmfs Please review.

It looks like you have currently no time for this project. But particularly in this case, it would be cool to merge all open pull requests and then publish a new version of the app. 😃

[simonspa]

I agree, some of them solve long-standing issues...

[dmfs]

@lemonboston please help to review this PR.

[lemonboston]

I started to check this out, but as I am not very much familiar with the details involved here, all my comments should be considered more just as questions and inputs rather than recommendations.

FieldAdapter has now getDefault() and getCustomizableDefault() - it's not clear for me why is the customization version needed. I don't know the contract and use-cases of getDefault() in detail, but if getCustomizableDefault() is only added to support more dynamic, strategy-based value, then I suppose getDefault() could accommodate both under the hood.

AbstractDefault could simply be an interface - I know there are types like this in the app, but I checked with @dmfs , he said we can abandon that abstract class style.
The idea to represent a default value calculation looks nice to me. I haven't checked the actual requirements and implementation logic for these relative Times, but from the first look the genericDefault parameter is not clear for me. For example in After, the value of DTSTART as referenceAdapter is both passed in as constructor param to DefaultAfter and as method param, too.
Couldn't any reference value / adapter that's needed to calculate the default be passed in as constructor parameter? So could this method param be removed?
I am thinking what would be needed in general to calculate any default.. wouldn't this suffice?:

interface Default<T>
{
   T getDefault(ContentSet currentValue, FieldAdapter fieldAdapter);
}

fieldAdapter is the current field, so implementation can check current value. Isn't it only the current field value and other field values that are needed to calculate a default? Anything else could go into constructor, to reference other fields for example.
Instead of FieldAdapter the actual value could go in as well, but I am also wondering if Default could apply the Constraints of a type if it wants to. (a getter would be needed)

[korelstar]

Thanks for your feedback!

FieldAdapter has now getDefault() and getCustomizableDefault() - it's not clear for me why is the customization version needed. I don't know the contract and use-cases of getDefault() in detail, but if getCustomizableDefault() is only added to support more dynamic, strategy-based value, then I suppose getDefault() could accommodate both under the hood.

I can do that, if favored. However, this would require to extend TimeFieldAdapter (e.g.) with StartTimeFieldAdapter and DueTimeFieldAdapter, each implementing the respective default-strategy. We would then have two field adapters which depend on the usage (StartTimeFieldAdapter can be used for a time field, which is used as start field). Since all other existing field adapters only depend on a specific type, I thought it would be better to keep this structure. But I'm fine with realizing the other variant (= introduce two subclasses from TimeFieldAdapter and remove the default package). That's a strategic decision. So please give an advise.

AbstractDefault could simply be an interface - I know there are types like this in the app, but I checked with @dmfs , he said we can abandon that abstract class style.

Okay, I will change this (only relevant if the general approach is kept, see "strategic decission" above).

[...] but from the first look the genericDefault parameter is not clear for me. For example in After, the value of DTSTART as referenceAdapter is both passed in as constructor param to DefaultAfter and as method param, too.
Couldn't any reference value / adapter that's needed to calculate the default be passed in as constructor parameter? So could this method param be removed?
I am thinking what would be needed in general to calculate any default.. wouldn't this suffice?:
[...] T getDefault(ContentSet currentValue, FieldAdapter fieldAdapter);
fieldAdapter is the current field, so implementation can check current value. Isn't it only the current field value and other field values that are needed to calculate a default? Anything else could go into constructor, to reference other fields for example.

I think it should be possible to replace the param genericDefault by fieldAdapter (and then call fieldAdapter.getDefault(currentValues) to get the default). But what do we win? If we want to eliminate that parameter and move it into the constructor, then the instantiation in TaskFieldAdapters would be much more complex.

but I am also wondering if Default could apply the Constraints of a type if it wants to. (a getter would be needed)

I don't understand what do you mean with this. What do you want to achieve with this?

[lemonboston]

I think keeping one TimeFieldAdapter is right and applying default value calculation to it generally. We shortly discussed it with @dmfs , one way might be to use a FieldAdapter decorator that sets up the Default strategy. Something like this:

FieldAdapter fieldAdapter2 = new CustomizedDefault(fieldAdapter1, customDefault)

concretely (may not be correct, just for indication):

TimeFieldAdapter DUE = new CustomizedDefault(
new TimeFieldAdapter(Tasks.DUE, Tasks.TZ, Tasks.IS_ALLDAY).addConstraint(new After(_DTSTART)),
new DefaultAfter(_DTSTART));

Would it make sense? So no getCustomizedDefault() and setDefault(), just the decorator which delegates everything else, except getDefault().

It's still a question for me though whether DUE or DTSTART need different way of calculating the default value at different places of the app or not.

[korelstar]

Would it make sense?

I think it's just a matter of programming style. I will adapt the style of this PR to whatever style you want ;-)

It's still a question for me though whether DUE or DTSTART need different way of calculating the default value at different places of the app or not.

As far as I know, the default value calculation of TimeFieldAdapter is only used by the TimeFieldEditor. So I think no: they do not need different ways of calculating the default value at different places of the app.

[lemonboston]

As far as I know, the default value calculation of TimeFieldAdapter is only used by the TimeFieldEditor. So I think no: they do not need different ways of calculating the default value at different places of the app.

I've also double-checked it now and I didn't see any other usage either, so it's good news, I think it would be better then to use getDefault() for the customized version as well, setting that 'strategy' with that decorator. Since FieldAdapter is kind of central type, I think it's worth keeping it simple if we can.
Could you please try to update it in that way?
dmfs will probably join as well later to check out the actual logic around these date-time defaults.

[korelstar]

I began the refactoring, but I have the problem, that the new CustomizedDefaultFieldAdapter<Type> should be generic (extends FieldAdapter<Type>) but when be used for DUE and DTSTART, the adapter should extend also TimeFieldAdapter which is not possible in Java. So I have at least three options:

1. Don't implement a generic CustomizedDefaultFieldAdapter<Type> but instead only a CustomizedDefaultTimeFieldAdapter which extends TimeFieldAdapter, or
2. change at several places in the code TimeFieldAdapter to FieldAdapter<Time> (I don't even know if this is always possible, since TimeFieldAdapter implements new methods), or
3. extract an interface from TimeFieldAdapter and let a new AbstractTimeFieldAdapter as well as a new CustomizedDefaultTimeFieldAdapter (which extends the generic CustomizedDefaultFieldAdapter<Type>) implement that interface.

I think the third one is too complicated. But what would you think is the best solution?

I've made a new commit which should show the problem (currently, it has compile errors due to the problem).

[dmfs]

You're right TimeFieldAdapter shouldn't implement two new methods that don't event implement an interface. We should get rid of these two methods and use option 2.

TimeFieldAdapter.hasTimeZoneField() was a bad idea. No other code should depend on that.
It's only used once in TimeFieldView.onContentChanged(ContentSet):

if (!taskTimeZone.equals(mDefaultTimeZone) && mAdapter.hasTimeZoneField() && mTimeZoneText != null)

I think we should just check taskTimeZone for UTC in that case. If the time zone of a task or field is UTC it should just be shown in local time. UTC is not a real time zone and no country in the world uses UTC as its time zone.
So I guess the check above can be replaced by

if (!taskTimeZone.equals(mDefaultTimeZone) && !Time.UTC.equals(newValue.timezone) && mTimeZoneText != null)

TimeZoneAdapter.isAllDay() is a bit harder to replace, because that field might be of interest even the get(...) methods return null. A better solution would be an AllDayFieldAdapter which extends FieldAdapter<Boolean>. So all calls to TimeZoneAdapter.isAllDay(values) could be replaced by AllDayFieldAdapter.get(values).

FYI: we're trying to get away from inheritance and move towards composition, hence the change request. In our recent projects we don't use inheritance at all, which worked out pretty well.

[korelstar]

Okay. I'm fine with that. Should we integrate that in this Pull Request or make a new one? (It's also possible to outsource the spell changes (`contraint` to `constraint`) into a separate PR in order to keep this one simple.) However, I can only do anything in a week, at least. Maybe you want to realize your proposed changes in the meanwhile ...

[dmfs]

I've just opened PR #335 which removes the stale methods. Would you do the review?
Regarding the typo in constraint, yes please fix that in a separate PR. That makes it easier to review.

[dmfs]

I've just merged #335 into staging. You can rebase now.

[korelstar]

I've outsourced the typo fix for constraint into #338. Since this PR is also working on constraints, I'm waiting for #338 to be merged in order to reduce merge-conflicts.

[korelstar]

I've rebased this PR and it's ready for a detailed review (again)!

[korelstar]

@dmfs @lemonboston Any news on this? Do my changes comply with your requirements, now?

[dmfs]

Doesn't seem to work for me:

06-08 00:15:41.560  3100  3100 E AndroidRuntime: Process: org.dmfs.tasks, PID: 3100
06-08 00:15:41.560  3100  3100 E AndroidRuntime: java.lang.ClassCastException: org.dmfs.tasks.model.adapters.CustomizedDefaultFieldAdapter cannot be cast to org.dmfs.tasks.model.adapters.TimeFieldAdapter
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.widget.TimeFieldView.setFieldDescription(TimeFieldView.java:136)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.model.FieldDescriptor.getDetailView(FieldDescriptor.java:348)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.widget.TaskView.setModel(TaskView.java:85)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.ViewTaskFragment.updateView(ViewTaskFragment.java:433)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.ViewTaskFragment.onModelLoaded(ViewTaskFragment.java:484)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.model.Sources.loadModelAsync(Sources.java:111)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.ViewTaskFragment.onContentLoaded(ViewTaskFragment.java:691)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.model.ContentSet.notifyLoadedListeners(ContentSet.java:615)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.model.ContentSet.onContentLoaded(ContentSet.java:185)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.utils.AsyncContentLoader.onPostExecute(AsyncContentLoader.java:113)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at org.dmfs.tasks.utils.AsyncContentLoader.onPostExecute(AsyncContentLoader.java:35)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.os.AsyncTask.finish(AsyncTask.java:651)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.os.AsyncTask.-wrap1(AsyncTask.java)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:668)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:102)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.os.Looper.loop(Looper.java:148)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at android.app.ActivityThread.main(ActivityThread.java:5417)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at java.lang.reflect.Method.invoke(Native Method)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
06-08 00:15:41.560  3100  3100 E AndroidRuntime: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
06-08 00:15:41.569  1636  1727 W ActivityManager:   Force finishing activity org.dmfs.tasks/.ViewTaskActivity
06-08 00:15:41.571  1636  1727 W ActivityManager:   Force finishing activity org.dmfs.tasks/.TaskListActivity

I got this when I tried to open the details of a task. Looks like there are still some places where TimeFieldAdapter is used rather than FieldAdapter<Time>

[dmfs]

From a code style and design perspective I have no objections.

[korelstar]

Strange, that those were still there. I've fixed this, now.

Please test again!

[korelstar]

This is also ready for testing 😉
Let's get this finished, soon -- it has been started more than seven months ago, too.

[dmfs]

Looks fine, thx.