FactController: Fix start date picker modifying date twice

[matthijskooijman]

This is another fix for the calendar picker issue #590, that intends to be a minimal fix for just that issue (so it's easy to review), without introducing any side effects.

I've tested this in various circumstances (new/existing facts, start/end times before and after midnight, changing the date using the dayline and the date pickers) and it seems to hold up well enough. I did not some usability issues (you cannot set an end date if there is no end time, if you remove the start time, the dayline buttons stop working), but all of those already existed before this change, so I'll leave those for a later PR.

The commit message is below and should explain how this fix works (and is longer than the diff :-p):

Previously, setting the date property would (in addition to setting
the property itself and the default_day property of the dayline) also
update the fact's start and end, moving those by the same delta as the
date property was modified.

However, this caused a problem when changing the start date using the
date picker, since on_start_date_changed() would already update the
fact's start and end and then also update the date property, which
would then update the fact again, effectively applying the delta
twice.

This commit fixes this problem by changing the date setter to no
longer update the fact. This means that the date property now only
tracks the currently selected day of the dayline (and the UI as a
whole), making the setter and getter a bit better matched.

This also means that any changes to the fact's start and end must be
done separately from modifying the date property. There are two places
that currently update the date property:

1. The on_start_date_changed() method, which already updates the fact.
2. The increment_date(), which is changed by this commit to update
   the fact.

Note that increment_date() now uses the Fact.date property setter to
modify the fact, which mostly implements the same logic as the code that
it replaces, except there is a small change in behavior when
fact.start is None. However, that is a corner case that probably
almost never occurs and has other problems, so this is left for a later
refactor.

This fixes #590.

[GeraldJansen]

Close, but still no cigar ;-) With this PR, if I create a normal completed activity today, then edit it and pick a start date two days ago with the start date picker, the cmdline shows a start date 4 days ago.

I further noted that creating a new activity straddling my hamster start of day setting (01:00) in the cmdline, say 00:30 02:00 xxx@yyy, the start date is correctly set to tomorrow, but the end date remains today and the save button is disabled.

[matthijskooijman]

Thanks for giving this a swing!

Close, but still no cigar ;-) With this PR, if I create a normal completed activity today, then edit it and pick a start date two days ago with the start date picker, the cmdline shows a start date 4 days ago.

Hm, are you sure that you are using the right version? I just tried again what you tried and it works as expected for me with this PR applied (but I do see the behavior you describe with an unpatched master).

I further noted that creating a new activity straddling my hamster start of day setting (01:00) in the cmdline, say 00:30 02:00 xxx@yyy, the start date is correctly set to tomorrow, but the end date remains today and the save button is disabled.

I can reproduce this behavior, with and without this PR applied. However, this is really a different problem, so I think it should be out of scope for this PR (but probably be fixed separately). I've created #675 for further discussion of this issue.

[GeraldJansen]

Hm, you're right that I wasn't using the right version. My bad. So you do get the cigar after all. Yay and congratulations! LGTM.

[rhertzog]

LGTM.

But the use of getter/setter might no longer be needed at this point. It would be cleaner to just always update self.cmdline.current_day in the update_cmdline method so that it's in sync when you compute a new command line representation of the fact.

[matthijskooijman]

But the use of getter/setter might no longer be needed at this point. It would be cleaner to just always update self.cmdline.current_day in the update_cmdline method so that it's in sync when you compute a new command line representation of the fact.

Yeah, I'm pondering an even more thorough cleanup of this class, also removing this getter/setter probably. But while pondering that, I thought it would be better to do a small-as-possible fix first, and then do the cleanup later.

I believe this PR has now seen sufficient testing and approval, so I'm going to merge it :-)

[m-sundman]

It's been 2 years since this issue was fixed (presumably). It would be ever so nice to get a new release containing the fix, so this application can actually be used.