Skip to content

FactController: Fix start date picker modifying date twice

Dirk HOFFMANN requested to merge github/fork/matthijskooijman/fix-590 into master

Created by: matthijskooijman

This is another fix for the calendar picker issue #590 (closed), 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 (closed).

Merge request reports