Offset between calendar selection and cmdline

[prk3]

I found that Hamster is adding some offset between dates selected via the calendar and those that land in "cmdline". For example, if I edit a task started and finished on 15th and change the date to 18th with the calendar, the command will start with "2020-03-21". This is very confusing.

[ederag]

@pkr3 Thanks for the report.
I have now moved off the project before being able to finish this chapter.
[the new team is full of energy; the project is fine].

To reproduce, the times have to be edited as well (a simple day change works fine here).

Workaround: use only the gui cmdline to change times when working on past days.

Edit: In edit_activity.py,
self.date should become self.day,
and should be stable when the TimeInput is emptied. That's the core of this issue.

[natedennis]

its ridiculous this garbage got pushed out to the world before it was any more stable that this. Ive been using this for eyars and im super frustrated with this project at the moment.

[ederag]

@natedennis Have you read the release note ?

Yet it would require more polish, and some functionality is in transfer (#493).
So users happy with their earlier version should wait for the next release.
It has been one year since last release, and there's the ubuntu LTS train to catch (still looking for a packager...).

Then do you really think that kind of comment help people
devote more of their free time to develop this project ?

There has been a fix on a local branch, with the intent to test and push this week-end,
but now it is not sure i'll find the energy.

[mwilck]

@ederag, you know the current situation is difficult. The author of almost all changes since 2.2.2, you, has left the project. We others aren't familiar enough with your code to come up with bug fixes quickly. I appreciate that you're still looking into issues, but you've stated very clearly that we can't count on you continuing to do so.

From the outside, it's non-obvious that the 3.0.x release isn't fully stable yet. Distro maintainers like myself are asked by users to update to "latest upstream" (I've pushed it for openSUSE recently, despite my personal doubts). Hardly anyone reads release notes, let alone on github. Some people may read the changelogs of their distro packages, but those aren't likely to quote the github text. Moreover, your note in for the 3.0 release suggests to "wait for the next release", which users might (wrongly) interpret to mean release 3.0.1.

I wonder whether the 3.0 and 3.0.1 releases should be re-labelled as "beta2" release rather than the official new version, intended mainly for users who want to try / play with the latest features and not "for the masses", as our marketing boldly says. However that's not so easy any more. I've no idea how to do it on github, and distro policiy usually prevents package downgrades. On openSUSE, if I wanted to revert to 2.2.2, I'd have to rename the package.

[ederag]

@mwilck please relax, no one did anything wrong here.

Number-wise, v2.2.2 looks more stable than 3.0.1, right ?

On openSUSE, if I wanted to revert to 2.2.2, I'd have to rename the package.

Then you also thought 3.0 was not as good as v1, but better then 2.2.2.
Having followed #433, and given your concerns, you knew the state,
but correctly considered that a lot bugs were fixed since v2.2.2, see the milestone report.

Finally, there had been three weeks of testing between the announcement and the final release.

[ederag]

@natedennis #597 was just pending, but testing was to be done this week-end.
Don't go thinking your message helped this project, though. Quite the opposite.
You just gained two days, and drained energy from two actual contributors.
That's a good way to come up empty handed in the long run.
I'm out of this project for the week-end, at least.
Will you do the testing yourself ? Suspense !

[mwilck]

The gist of this problem is the convoluted way of setting the self.date using that @date.setter annotation. The innocent-looking

 self.date = self.fact.date or dt.hday.today()

calls the setter behind the scenes and thus has side effects. It actually tries to apply some deltas to various fields and together with the code in on_start_date_changed(), ends up applying the time delta twice, leading to the effect observed.

My personal naïve approach at fixing this would be simply

def on_start_date_changed(self, widget):
    if not self.master_is_cmdline:
        self.date = self.start_date.date
        self.validate_fields()
        self.update_cmdline()

This keeps using the weird setter method (which is used in other places too), but it works because it applies the time delta only once. DISCLAIMER: I'm almost certainly overlooking something because I don't understand all the complexity in this dialog. I noticed that the end_date calendar widget isn't updated, for example (AFAICS not because of this change).

IMO, if we have things like setters with side effects, this is how it should be possible to use it - without any additional special-case code calculating deltas.

[ederag]

Have you missed my comment ?

self.date should be stable when the TimeInput is emptied. That's the core of this issue.

[mwilck]

Have you missed my comment ?

self.date should be stable when the TimeInput is emptied. That's the core of this issue.

Yes, I missed it. Did you expect me, or anyone, to understand this comment? I understand it now, after having digged into your code for about 2h yesterday. I still don't understand how your patch series in #597 addresses this, you didn't bother to explain it, and at least for my poor brain it's non-obvious.

Let me elaborate a bit more on the "setter" issue. I generally prefer explicit programming. Reading this:

self.set_date(x)

suggests to the reader of the code that she should check the set_date() function. (The name could be even more expressive to tell the reader that the function has side effects). OTOH this:

self.date = x

looks like a trivial assignment; the reader has no clue that it is not. Therefore I don't think using a setter here was a good idea. For similar reasons, I also don't like python's @property feature much. Using these techniques just to hide function calls, save some typing, and make the code look superficially more elegant is almost always wrong, IMO. (I don't claim you did it for these reasons).

I believe that both techniques have a certain value, but only if the apparent attributes being fetched or set really, really behave like simple attributes. IOW, the class objects behave in such a way that users of the code never need to bother about side effects. That's not the case here, and therefore I find the usage of the setter confusing at best.

This tells us what we already knew - our ideas about what constitutes good code are quite different. I'm probably not the right person to review your PRs. I will bring this up in the maintainers team.

[ederag]

I do agree with you in general: explicit setter when assignment has strong side effects or take time.

In this case, IIRC, the rationale for a @property setter was mainly (besides backward compatibility)
to avoid bugs because there were already several assignment to self.date,
and any direct assignment to self.date would have led to subtle bugs.

My notes (end of december) say

using increment_date is a bit fragile, there is no way to be sure
that no-one will change date without changing also the CmdLine default_day.
...
Nevermind, stick to increment date, but make date private => self._date ?
No, really property is cleaner,
but only update_fields in a function that is in control.
(e.g. in increment_date)

Now it could make sense to make self.day read-only (no @day.setter) and add an explicit setter.
That would just lose backward compatibility and the ability to write

hamster/src/hamster/edit_activity.py

Line 163 in fadaf45

self.day += delta

Might worth being explicit.

Edit: So our general divergence is not on these technical details,
but rather on a much broader, deeper level.

[ederag]

I understand it now, after having digged into your code for about 2h yesterday

Normal, this is not an easy code, and this area in particular was WIP.

Note that it looks like you complain about taking two hours reading,
but expect me to take hours from my time to explain why I did things.
I'm still expecting tests.
(am I addressing the correct issue ? No one bothered to confirm yet).

[matthijskooijman]

I've also looked around the code a bit, let me share some thoughts here. I'm partly just thinking out loud and keeping some notes for myself, so it's a bit train-of-thought-style.

Now it could make sense to make self.day read-only (no @day.setter) and add an explicit setter. That would just lose backward compatibility and the ability to write

This is also what I would be inclined to do. As for compatibility: There are no users of this code other than the code in this repository, right? It seems all dbus-communication happens directly against the storage, so if we just update all references to the day/date setter, this should be good?

using increment_date is a bit fragile, there is no way to be sure
that no-one will change date without changing also the CmdLine default_day.

I looked at how the default date stuff works, and AFAIU the "default date" is the date selected in the top line, and the date that is omitted from the cmdline view. If that is changed, the cmdline must be told, so it can still show/hide the date properly.

However, it seems that this "default date" is is usually initialized to the start date of the fact (or today for new facts), and the code usually keeps them equal (e.g. when you change the date, both the fact start date and the default date are updated).

I wondered if we could not just remove this default date, and instead make the fact start date always the implicit start date (which would remove the need to keep this default date in sync and potentially significantly simplify the code). However, it seems that the handling of times after midnight might end up with a "default date" of today, and e.g. when entering a start time of 02:00 end up with a start date at tomorrow.

This might still work (as deriving the "default date" from the fact start datetime is deterministic). I guess this means that cmdline never shows a start date, and only shows an end date when fact.start.hday() does not equal fact.end.hday().

This does need that all facts have a starte date, but when adding a new date that can just be the current date (or the date of the overview window), and there is no way in the GUI to remove the start date from a fact AFAICS, so facts should always just have start date?

One complication is adding facts through the commandline on different dates. Always hiding the start date would then mean that if you would enter a different start date, the dayline at the top might update as soon as a date was successfully parsed. If that also means that the date you typed is removed, that might make it tricky to fix typos. OTOH, it seems that the cmdline is only updated when the fact is updated through the separate fields, so this should work out just fine.

One other complication, looking more closely, is that the cmdline does not current have access to the current fact (only the original fact when editing started, but that seems unused). It probably does not actually need this access, since it just produces text and completions and the CustomFactsController handles parsing the text eventually and updating the fact, so it only needs to know the "default date" (so maybe start date). Giving cmdline a reference to the current fact could work, but then on_cmdline_changed actually replaces the current fact (rather than updating it), so that won't work. Maybe we could just pass a getter function for the "default date" to the CmdLine constructor (which is just a lambda that returns the current start date, so the other simplifications can still happen)?

In conclusion, it seems that this code could indeed be simplified somewhat by removing the explicitly set "default date" and instead just use the start date as the "default date" for the cmdline and making changes of start and end dates (e.g. shifting both equally) a bit more explicit. I expect this simplification will also fix this bug as well.

I'll need to look a bit closer at how to approach this refactoring, though, and probably just play around with the code a bit. I'll followup later, maybe with a PR directly.