hamster/client: edit_activity: don't touch self.date on date changes

[lynxis]

The fact is recalculated already in on_start_date_change(). By calling the setter of
self.date the fact would be change a second time. The start_date was then wrong by
the delta. E.g. edit a complete fact (start and end must be set). Click on the start date and choose 2 days into the future, the command line shows now 4 days into the future.

Fixes: #667

[ederag]

Better fix, been pending for 9 months and a half: #597 (comment)

[benjaoming]

Thanks for the change and the fix - LGTM

[matthijskooijman]

I haven't looked closely at this PR yet, but it seems very simple and I recall (from earlier discussion and fix attempt in #630) that this code is quite complex with a lot of complex interactions. So I'm afraid that this simple fix might have unintended side effects.

@benjaoming Did you also test this PR, or just reviewed the code?

[benjaoming]

No, maybe @lynxis can reflect on the extend of how this is tested?

Could letting a small change like this in to the master branch also provides a way forward for verification before a release?

[GeraldJansen]

I have tested this simple change. Tests in tests/test_stuff.py all pass. The coordination of cmdline and date pickers seems to work perfectly with this change. The left and right arrows also decrement and increment the dates properly in the cmdline and date pickers. Activities straddling the hamster start of day also seem okay. The only glitch I've found is that the day displayed on the timeline (hamsterday) doesn't get updated if the date is changed in the cmdline or start date picker, so the timeline display gets out of sync. This in turn affects whether or not the start date is displayed on the command line. Anyway, complements for the dramatic improvement in ability to edit facts on prior dates with such a minimal change.

[lynxis]

I've tested only a couple things in the edit_activity and see the update works correct.

• click on the start date change it to a different date as in the main window (hamsterdate?).
• click on the end date and see it's matching the cmdline
• change the start_date back to the original date. the start_date in the cmdline got removed (no idea what's intended)

[GeraldJansen]

• change the start_date back to the original date. the start_date in the cmdline got removed (no idea what's intended)

The start_date is not displayed in the cmdline if it is the same as hamster day.

[ederag]

The start_date is not displayed in the cmdline if it is the same as hamster day.

One needs to be more precise, see #630 (comment):
The timeline shows a hamster day (the default day)
while the start date is a civil date.
Saying they are "the same" makes little sense.

What makes sense is whether the start {civil date+time} is in a hamster day or not.
start or end are instants, while a hamster day describes a span of time.

The rule:

hamster/src/hamster/lib/datetime.py

Lines 439 to 441 in 0ba8407

	Start date is shown only if start does not belong to default_day.
	End date is shown only if end does not belong to
	the same hamster day as start (or to default_day if start is None).

[lynxis]

Anything missing for a merge?

[benjaoming]

Can someone say with confidence that it's an overall improvement? In that case, I think we should merge it.

It certainly sounds like it's an overall improvement :)

[ederag]

Had the fix been that simple, it would have been proposed earlier.
The timeline display getting out of sync issue mentioned by @GeraldJansen is important.
Moreover, this change will make understanding the code intention harder.
Besides, merging this PR would prevent easy merging of my fix,
which to the very least is a much better start
(it both fixes the core issue, and clarifies code for future maintenance),
see #597 (comment).

[matthijskooijman]

I had another look at this issue. I considered (and even started somewhat) working on a more thorough refactor/cleanup that would fix this issue, but then realized that that would probably take quite some time to make and then also to get reviewed and merged, so that a small fix like in this PR would probably be better now, delaying the more thorough cleanup to later.

Looking at this PR again, I realized that the problem is that the fact date is updated twice (once by on_start_date_changed() and one by the date setter) and this PR solves this by removing the call to the date setter from on_start_date_changed(). However, this has the side effect of no longer setting the date property and no longer updating the dayline, as @GeraldJansen found in testing as well. An obvious alternative fix would then be to modify the date setter to no longer update the fact date, which would also prevent that from happening twice. I considered and tried this alternative, and it seems to work very well, without any additional side effects.

The result is in a new PR, #674. @lynxis, @GeraldJansen, @benjaoming, would you care to have a look at that PR and let me know what you think?

[matthijskooijman]

I've just merged #674, which fixes this issue in a slightly different manner, so I'm closing this one. @lynxis Thanks again for your effort, which provided the stepping stone for that fix :-)