projecthamster / hamster

GNOME time tracker
http://projecthamster.org
GNU General Public License v3.0
1.08k stars 249 forks source link

Cmdline parsing when end time is before start time #675

Open matthijskooijman opened 3 years ago

matthijskooijman commented 3 years ago

In https://github.com/projecthamster/hamster/pull/674#issuecomment-798632521 @GeraldJansen wrote:

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 issue. However, in essence what this describes is sortof expected behavior with how the parser currently works: The parser interprets any end time without date as a time within the same hamster day as the start time. This problem is pretty much exactly the same as when you enter e.g. 18:00 - 17:00 foo@bar, i.e. as far as Hamster is concerned, your end time is before your start time in both cases.

This would probably be easy enough to fix by interpreting the second time as a timestamp in the 24h following the start time (instead of in the same hamster day), but I haven't thought that through yet. Changing this should probably also affect serialization of facts for the cmdline.

I think there should be no significant compatibility issue talking to applications that input fact strings into Hamster, since this would only change interpretation of strings that would not previously be valid facts. For applications that take fact strings from Hamster and parse them again, this could be problematic, but I'm not sure if those exist in practice. Also, once such applications are updated to apply the same parsing rule, they would be able to work with fact strings serialized using both the old and new rules.

GeraldJansen commented 3 years ago

I've been working with Hamster for a long time and still get tripped up by this behavior. For me it would be more natural to treat an end time with no date specified as belonging to the same real-actual-civil date as the start time as long as the end time is greater than the start time. If the specified end time is less than the start time assume that the end date is the day after the start date. This will be less confusing to casual users than the current behaviour.

Anyway, as ancillary info, here's what currently happens from the CLI:

$ hamster track 'xxx@yyy' 00:30-02:00
hamster-service up
Traceback (most recent call last):
  File "/d/gj/hamster/src/hamster-cli.py", line 495, in <module>
    id_ = hamster_client.start(*args.action_args)
  File "/d/gj/hamster/src/hamster-cli.py", line 259, in start
    self.storage.check_fact(fact, default_day=dt.hday.today())
  File "/d/gj/hamster/src/hamster/client.py", line 208, in check_fact
    raise FactError(message)
hamster.lib.fact.FactError: Duration would be negative.
Working late ?
This happens when the activity crosses the
hamster day start time (01:00 from tracking settings).

Suggestion: move the end to the next day; the range would become:
00:30 - 2021-03-14 02:00
(in civil local time)
matthijskooijman commented 3 years ago

I've been working with Hamster for a long time and still get tripped up by this behavior. For me it would be more natural to treat an end time with no date specified as belonging to the same real-actual-civil date as the start time as long as the end time is greater than the start time. If the specified end time is less than the start time assume that the end date is the day after the start date. This will be less confusing to casual users than the current behaviour.

I believe that what you propose is equivalent to what I called "interpret the end time within the 24h following the start time", so I think we agree here.

Good to know what the CLI says, looks like there is a specific message for this case that should be adapted or removed if this behavior is changed.