Adding activities in retrospect is very hard

[gsobczyk]

Update edit_activity.py to components in the form be aware of selected date

For activity in cmdline: some activity (without date and time)

• checked incrementing and decrementing day
• checked date picker
• checked date picker set to one date and cmdline on another date (cmdline has higher priority)

This fixes #627 Adding activities in retrospect is very hard.

[gsobczyk]

Related to #590 / #597

[matthijskooijman]

I just gave this PR a whirl. At first glance, it seems to work, but it seems to introduce a new problem: If you create a new activitity and add e.g. "-02:00" in the commandline to set the end time. Without this PR, the end date is set to tomorrow, but with this PR, the end date is set to today and saving is prevented. This problem does not occur when you enter the end time in the "end" input box, just in the commandline.

So some part of the "past midnight" handling is broken by this.

I also do not understand the fix in this commit completely yet, the commit message doesn't really help explain the fix here and should be improved before this is merged (but I can do that once I manage to figure out what it does exactly and why). It seems that he essence of the fix is to remove special "delta" handling from both the date setter and on_start_date_changed and apply it once in the change_start_date method, which is largely what was discussed in #590 as well (though that suggested removing the date setter completely, which might still be a good idea.

I also still wonder if, as I suggested in #590 (comment), it wouldn't be better to even remove date entirely, and just derive the start date from the fact.start_time (which I think should always exist, and if not, we can probably ensure it does).

[matthijskooijman]

@gsobczyk, Why is this change needed? It has a bug, since it uses self.fact.date also for the start date, but I also wonder why this is needed at all. Especially the or self.data part seems to set a default, but why not just let (especially) the end date unset when none is set yet?

[matthijskooijman]

If you create a new activitity and add e.g. "-02:00" in the commandline to set the end time.

This seems to be a small typo, see the comment I left inline (though I don't understand why this change is needed in the first place).

I just pushed some extra commits to this PR. Three of them are fixups (separated for easier review) to the commit by @gsobczyk that:

• Rename the new function to move_to_date, emphasizing that the code actually moves the fact (including the end time)
• Move the calls to validate_fields() and update_cmdline() back to on_start_date_changed, since that's more consistent with the other on_*_changed fields. This also means that increment_date no longer calls them, but I don't think it needs them (and if it does, that should be a separate change IMHO).
• Move the definition of the new move_to_date to a more logical place in the file
• Revert the change to update_fields that causes the bug described above (I just reverted this rather than fixing it, since I don't understand why this is needed. If there's a good reason, we can reaplly a fixed version).

I also added three new commits of my own, which are related cleanups and a small fix.

I'm pondering to make some more changes, but I'm out of time now. In particular:

• Maybe _date can be removed, and the date getter can just return self.fact.date instead. This means that the start date of the fact is also always the "default_day", which I think makes sense. This does mean that wherever self._date is currently assigned, self.cmdline.default_day should be assigned (or rather, whenever self.fact.range.start changes, self.cmdline.default_day should change, but there does not seem to be a good way to do an "onchange" handler or so here, so just manually inserting some code wherever this property changes is probably a second-best thing).
• Currently, on on_cmdline_changed, the start date might be changed, but self.cmdline.default_day is not updated (which is a small bug that might cause weirdness in some cornercases).
• Maybe replace the custom delta code that is (now) in move_to_date() with a simple assignment to self.fact.date, which already seems to handle these delta semantics.
• Check the various related bug reports to see if there are more related bugs to be solved.

I think none of these changes really affect the PR as it is, though, so if people test and approve the PR as it is now, maybe we can merge this (after squashing the fixups and probably improving the commit message, didn't do that yet) and do the above changes in a new PR as well.

[matthijskooijman]

Currently, on on_cmdline_changed, the start date might be changed, but self.cmdline.default_day is not updated (which is a small bug that might cause weirdness in some cornercases).

I added a commit to fix this by setting self.date, which updates self.cmdline.default_day and (indirectly) also ensures that the dayline at the top is updated (this was another problem before).

As for the other changes I suggested above, I think they are sensible, but I have the feeling that this entire code might need a more thorough redesign to become a bit less fragile. And I'd rather get these fixed merged (and released) ASAP, rather than letting that wait for a big redesign task.

I also had another look at #627, and this PR fixes only "issue 5" described there (which is #590), the other issues are unfixed. However, I think the other issues are mostly small usability issues, not actually incorrect behaviour like #590.

So, I have nothing more to add to this PR now, I think it's good to merge it as it is now. @gsobczyk, what do you think? Also, could you respond to my inline comment above? @Others, I'd welcome review and testing of these changes.

[rhertzog]

I did test the code and indeed it fixes only issue 5 so the PR should be reworded to not close #627. I did not find any other serious side-effect so I believe it's OK to merge this but we should really have some non-regression tests covering the logic of such changes... the other usability issues are pretty annoying.

[ederag]

Beware: @matthijskooijman and others want/seem to mix civil date and hamster day.

The timeline choice is the hamster day.
The fact will be displayed under this day in the overview,
so it can't be the civil date.
(good old feature of hamster)

The start and end entries are civil dates.
All the entries except cmdline are "verbatim"
(as close as possible to the Fact data).
The Fact data must be close to the database, so it must be civil dates.
So the entries can't be hamster days.

[GeraldJansen]

I have tested the PR, past midnight, and find that some things are still quirky. The Back arrow on the timeline moves the date back by two days and the Forward arrow is unresponsive. Setting the "New day starts at" to 00:00, things seem to behave better. My testing has not been very systematic, but I feel that the date handling is still too fragile.

[ederag]

Indeed, that's a consequence of my previous comment.
Both the original PR and cf3d13a are wrong.
Please see #597 (comment) for a better start.

[GeraldJansen]

@gsobczyk, @matthijskooijman: My previous comment may have sounded negative, but your work on this critical issue is very much appreciated! Please keep at it!

Personally I find the complexity of the code w.r.t. date and time handling too daunting to tackle myself. I would seek simplifications. Perhaps @matthijskooijman's idea of removing the date field from Fact would help.

I think having three ways to adjust the date, i.e. cmdline, timeline arrows and date picker, are just too many. I would consider removing the arrows from the timeline for starters. They were very useful back when there was no date picker, but less so now.

I would also go a step further and remove the "end date" date picker altogether. Normally the duration of activities would not exceed 24 hours and the end date can be determined by the start date and the start and end times. If the end time is greater than the start time the dates are the same, otherwise the end date is the day after the start date. This would eliminate the annoyance of the Save button being disabled for a reason the casual user doesn't understand. (Note that activities longer than 24 hours could still be entered on the cmdline by fully specifying start and end dates and times.)

[matthijskooijman]

The timeline choice is the hamster day.
The fact will be displayed under this day in the overview,
so it can't be the civil date.

Ah, good point, I haven't looked properly at that, and didn't test past-midnight start times (only end times). It seems @GeraldJansen did and it is indeed broken. I'll have a look at this again later this week. Thanks for pointing this out and providing some extra links, let's see if I can make sense of them :-)

@gsobczyk, @matthijskooijman: My previous comment may have sounded negative, but your work on this critical issue is very much appreciated! Please keep at it!

I had not taken it as negative, just as constructive feedback and testing. Thanks :-)

I think having three ways to adjust the date, i.e. cmdline, timeline arrows and date picker, are just too many. I would consider removing the arrows from the timeline for starters. They were very useful back when there was no date picker, but less so now.

I'm not sure I agree here: If they all just work as expected, I think they each have a place. For example, I often use the date arrows for entering an activity yesterday or the day before. I could use the start date picker, but that's just a little less convenient. I'm also hesitant about removing long-standing features, since that's likely to make people unhappy (unless they are really broken or really unnecessary, of course, but I'm not convinced of that here).

I would also go a step further and remove the "end date" date picker altogether. Normally the duration of activities would not exceed 24 hours and the end date can be determined by the start date and the start and end times. If the end time is greater than the start time the dates are the same, otherwise the end date is the day after the start date. This would eliminate the annoyance of the Save button being disabled for a reason the casual user doesn't understand. (Note that activities longer than 24 hours could still be entered on the cmdline by fully specifying start and end dates and times.)

I'm also not sure I like this idea. I agree that > 24h activities are rare, but the idea behind the expanded view we have now is that you can edit everything without using the cmdline, which IMHO makes it easier for people to figure out how the commandline works, or to just not use the commandline if they prefer to click instead of type where possible. I'd prefer not to break this by removing the (admittedly rarely used) end date picker.

Of course, all the UI elements should be doing the right thing and make things easy, not hard for people, so there are still some things to fix here. But I think those are mostly the subject of #627, and I think I want to keep them out of scope for this PR for now.

[matthijskooijman]

I had another look at this bit of code, and I think I now better understand the distinction between "civil date" (the actual date that a start of end time is in) and the "hamster day" (the effective day that a time belongs to, which is one day less than the civil date for past-midnight times) that @ederag has made. And indeed, the current version of this PR insufficiently respects this distinction. In particular, it should be noted that the dayline at the top shows the hamster day, while the date picker shows the civil date (so for a 2021-03-13 01:00 start time, the dayline will show 3-12, while the date picker will show 3-13). I guess this makes sense, with the date picker always showing the exact moment and the dayline showing the day the fact belongs to. However, my code in this PR does not make this distinction and when the date is changed (using the dayline or the date picker), the new date is treated as a civil date in both cases, which is wrong for the dayline and causes the behaviour that @GeraldJansen reported.

I also think it would be good to update the code to make this distinction more clear, e.g. by using hday for variables and methods returning the hamster day and more standard terms (just date, start, etc.) for civil dates. This would apply to variables inside edit_activity.py, but renaming Fact.date (which returns a hamster day) would also help a lot.

I also finally had a good look at @ederag's https://github.com/ederag/hamster/commits/fix-calendar-offset branch, and now I understand better what they are doing there. In fact, that branch already updates some uses of the hamsterday to use day (I was thinking about hday, but I guess day is already distinct enough from date, so that would be fine), so it seems we're in agreement about that part. I'm still having trying to puzzle out the reasons for some of those changes (lacking detailed commit messages), but I think the approach of renaming some things for clarity and then making some fixes make sense.

So, I'm going to try to make a new PR for this issue, based on @ederag's branch, adding more detailed commit messages, restructuring some of the commits and maybe doing a few things different (especially in the later commits).

[ederag]

Good that you eventually reconsidered #597. You got it.

But I explicitly ask you not to tamper with my commits.
That's not OK. Their dates and styles are important. That's what future devs will see.
Cherry-pick or rebase as is,
then you may append your own commits, with your own style.

[matthijskooijman]

I guess #597 would be a better place for this discussion, so I've copied the above two comments there and I'll also reply there (I should probably have posted part of my previous comment there in the first place).

[matthijskooijman]

PR #674 has been merged, which fixes this particular issue. However, I'll leave this PR open since it still contains some useful cleanups and refactorings, which should probably be extracted and serve as a basis for a more thorough cleanup of edit_activity later.