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.