Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strange behavior when adding an activity starting by a number #618

Open
brunetton opened this issue Jun 5, 2020 · 12 comments
Open

Strange behavior when adding an activity starting by a number #618

brunetton opened this issue Jun 5, 2020 · 12 comments

Comments

@brunetton
Copy link

brunetton commented Jun 5, 2020

Hi

I'm using happily using Hamster for years, and like many many other users, I'm very confused with new behavior (introduced in v3 if I understand well; not sure about this).

"Long" story: why I need to enter activities starting by a number

Before I was using Hamster for time tracking and I used two projects I developed for syncing my work with issues systems:

This scripts used regex to find the issues ID I'm working for. This was straight and simple: #123 - fix this bug indicated I was working on issue 123.
But since v3 (since using hamster-snap on Ubuntu), I can't use #123 notation on facts description as Hamster interprets it as a hashtag. I never use hashtags but as the new Gnome philosophy seems to be "no configuration, we choose for you what you need" I can't decide to use tags or not. So I can't use #123 notation anymore.
The alternative is to start activities names (facts, in DB) by the issue number; without #. For example: "123 - fix this bug".

Short story, the concrete example

Steps to reproduce:

  • work on an activity named "123 - fix this bug"
  • stop it
  • later (at 17:31), work on it again: start a new activity (ctrl-+), Hamster gently propose your last activity, so cmdline is 17:31 123 - fix this bug
  • validate this choice (enter) and observe that instead of starting an activity named 123 - fix this bug at 17:31, Hamster started an activity named - fix this bug that finishes in the future (at 19:34)

image

@mwilck
Copy link
Contributor

mwilck commented Jun 5, 2020

Thanks for the report.

17:31 123 - fix this bug

The problem is that hamster interprets the 2nd number as a duration. This is by design (if you look at the "Input" page in the hamster help). It was merged by the previous maintainer @ederag as part of #525 two weeks before the beta release. There was no broad discussion about this change. IIRC, it hasn't bothered anybody so far. In general, the parser has changed quite a bit in 3.0 without a formal specification and without broad community discussion.

Note that you wouln't have this problem if your number had 4 or more digits, or if you didn't use a blank after the number. 1234 - fix that bug or 123- fix that bug would work. Please consider if that works for you.

If I'd designed the parser, I think I'd have defined duration as +123, and I ditched the "time in the future" feature instead (which is how hamster currently interprets a number preceded by +).

I can't use #123 notation on facts description as Hamster interprets it as a hashtag

This seems to be a bug, actually. We have commit 03288b1, merged in #490, which should allow # everywhere inside a fact. It must have been broken by a later commit. But there have been quite a few parser changes in reaction to complaints from users with different preferences, and I've lost oversight.

What we'd really need would be a specification for the parser, clearly defining what the syntax would be and how it would be interpreted, ideally discussed and agreed on by maintainers and users. Currently we only have a few comments in the source code (which is more than nothing).

I count you as a user who wants activities starting with # and including blanks, and who does not want plain numbers to be interpreted as duration.

@mwilck
Copy link
Contributor

mwilck commented Jun 5, 2020

as the new Gnome philosophy seems to be "no configuration, we choose for you what you need"

I understand your frustration, and I've cursed GNOME for this myself. But hamster is not part of GNOME any more. Making a complex arser like this configurable is very hard to get right. If we had made this configuratble, most users wouldn't even understand the effect the options would have. And imagine trying to test, or write documentation for, a parser that can be configured to interpret # or 123 this way or another way - that would be a true nightmare. And not to forget, the man power behind hamster is limited.

I'm not saying we made all the right choices. Some of this might be up to discussion again, even though we have to be cautious, as some users may have got used to and like the new parser already, and they won't be pleased if we revert certain features.

The only thing that might be feasible is to offer a choice between "v2" and "v3" parsers.

@brunetton
Copy link
Author

Hi Martin, first very much for your complete answer ! Here are my answers below

Thanks for the report.

17:31 123 - fix this bug

The problem is that hamster interprets the 2nd number as a duration. This is by design (if you look at the "Input" page in the hamster help). It was merged by the previous maintainer @ederag as part of #525 two weeks before the beta release. There was no broad discussion about this change. IIRC, it hasn't bothered anybody so far. In general, the parser has changed quite a bit in 3.0 without a formal specification and without broad community discussion.

Ok, I suspected it but I wasn't sure. I said to myself "no, it can't be a wanted behavior" as I personally would have used +XXXm or something like that; ie something quite characteristic.

Note that you wouln't have this problem if your number had 4 or more digits, or if you didn't use a blank after the number. 1234 - fix that bug or 123- fix that bug would work. Please consider if that works for you.

If I'd designed the parser, I think I'd have defined duration as +123, and I ditched the "time in the future" feature instead (which is how hamster currently interprets a number preceded by +).

In fact, I wonder why can't I define my own patterns, defining a regexp expression. But maybe this is not as simple as I thought.

I can't use #123 notation on facts description as Hamster interprets it as a hashtag

This seems to be a bug, actually. We have commit 03288b1, merged in #490, which should allow # everywhere inside a fact. It must have been broken by a later commit. But there have been quite a few parser changes in reaction to complaints from users with different preferences, and I've lost oversight.

Again, I don't think a solution that makes everybody happy could exist. Why not let users define their own regex / parsing class that could be included as plugins ? This would solve all of those questions / complains / problems and let everybody happy. For non technical users, we could easily imagine a main repo for those syntaxes where users can pick their prefered one and activate it by pasting a Python file onto their ~/.local/share/hamster/ directory.

What we'd really need would be a specification for the parser, clearly defining what the syntax would be and how it would be interpreted, ideally discussed and agreed on by maintainers and users. Currently we only have a few comments in the source code (which is more than nothing).

I count you as a user who wants activities starting with # and including blanks, and who does not want plain numbers to be interpreted as duration.

Indeed, I never used plain numbers as duration, as I'm using Hamster "in real time"; ie: start / stop. Sometimes I forget to start Hamster (when a client calls typically, it's an activity switching but not initiated by me) but when the call finishes I look at current time and I add an activity knowing the end time. Anyway, I never use that functionality.

So in summary I'd say that I'm not sure that an international parsing format could exist, as every user have their needs, that are all. And I see some solutions to make everybody happy (separated by inclusive OR):

  • add the possibility to customize parsing regex / or surcharge parsing Class/function with a plugin system
  • add a config option that let user choose between multiple tastes/needs identified in issues

Maybe I should open an issue for this idea being discussed.
Thanks !

@brunetton
Copy link
Author

as the new Gnome philosophy seems to be "no configuration, we choose for you what you need"

I understand your frustration, and I've cursed GNOME for this myself. But hamster is not part of GNOME any more.

I thought ! As the project description in Github is "GNOME time tracker". Maybe should it be "A time tracker for Gnome ?"

Making a complex arser like this configurable is very hard to get right. If we had made this configuratble, most users wouldn't even understand the effect the options would have. And imagine trying to test, or write documentation for, a parser that can be configured to interpret # or 123 this way or another way - that would be a true nightmare. And not to forget, the man power behind hamster is limited.

Of course I fully understand that a few devs are working on this project, mainly in their spare time.

I'm not saying we made all the right choices. Some of this might be up to discussion again, even though we have to be cautious, as some users may have got used to and like the new parser already, and they won't be pleased if we revert certain features.

The only thing that might be feasible is to offer a choice between "v2" and "v3" parsers.

Maybe all of this could be cancelled with a plugin system letting users code their parser. I'd love that !

Thanks again

@GeraldJansen
Copy link
Contributor

You can include any #tag in the activity name, including numeric values like #999, but you have to end the activity[@category] by ,,, i.e. #123 fix this bug,,. (The double ,,, which has to precede actual tags, was introduced to satisfy another user who wanted to be able to include , in the activity.)

@mwilck The wiki page Entering activities was my attempt at getting the parser behaviour formally specified outside of the code. Unfortunately the idea was not fully adopted and the page is out of date w.r.t. the actual changes implemented.

@mwilck
Copy link
Contributor

mwilck commented Jun 6, 2020

You can include any #tag in the activity name, including numeric values like #999, but you have to end the activity[@category] by ,,, i.e. #123 fix this bug,,. (The double ,,, which has to precede actual tags, was introduced to satisfy another user who wanted to be able to include , in the activity.)

Sigh, this is so non-intuitive...

@mwilck The wiki page Entering activities was my attempt at getting the parser behaviour formally specified outside of the code. Unfortunately the idea was not fully adopted and the page is out of date w.r.t. the actual changes implemented.

Thanks a lot for starting this endeavor. I think we should take it up again.

@ederag
Copy link
Collaborator

ederag commented Jun 6, 2020

TL;DR: Hold on. Fix is simple (IIUC the issue).
I had some time and was curious. Still gone from this project.


General discussion started about 4 months before the 3.0 release: #465.


@brunetton Thanks a lot for the clear report.
As Gerald said, activity may start with a hash # and moreover,
the description may contain #123:
Screenshot_20200606_155842
Did you use the command line ?
Please see #482 (comment) or the documentation (Help > input).
Screenshot_20200606_162624

So your "long story" use case is already covered.


The "short story" is an actual bug, but not in the parser itself, IMO.
The serialized string is not adequate in this case.
You may cherry-pick f302a57 that seems to fix it.
Note: it's not a fork, just a courtesy.


Maybe all of this could be cancelled with a plugin system letting users code their parser.

It's tempting, but will end up with even more frustration for the user,
as designing a good parser is no small feat.
Our predecessors did a great job, and still there were corner cases.
I tried to follow, with two strong points:

  1. Few clear rules, no fragile heuristics.
  2. The instantaneous update of separate fields and cmdline, in the gui, as a way to quickly experiment,
    or discover the correct syntax.

About the parser specification.
Beware of theory; it will be hard to avoid mismatches between the specification and the actual parser.
It is much more robust to define use cases, add tests for them, and document (which is mostly done).
This is no basic parser. The many use cases that accumulated along the years prevented that.


Finally, I recently read the first chapter of "The Legacy Code Programmer's Toolbox", and that's exactly how I approach contributing to old codebases. Couldn't resist to recommend it.

@ederag
Copy link
Collaborator

ederag commented Jun 8, 2020

@brunetton About using for the duration

something quite characteristic

And @mwilck

If I'd designed the parser, I think I'd have defined duration as +123, and I ditched the "time in the future" feature instead (which is how hamster currently interprets a number preceded by +).

As many statements about the parser,
it makes sense, and actually was my initial temptation as well.
But deeper thoughts, partly on the coding side and mainly on the users side, decided otherwise.

Rationale here:

  • +123 would lead to friction: is it "the duration" or "the opposite of -123" ?
    Standard mathematical sense was chosen. Easy to remember.
  • 123m could make sense, but then why -123 and not -123m ?
  • Maybe use 123d ? But think of 3d development, not so uncommon...
  • OK, d123 ? Perhaps more rare. But then friction: is the d before or after ?
  • Finally just 123 is clear, simple,
    and, icing of the cake, still allows you to type the range without leaving numpad.
    Adjudicated.

@ederag
Copy link
Collaborator

ederag commented Jun 8, 2020

IMO, what made it so not intuitive is the bug quickly fixed with f302a57, not the parser itself.

To make all this more intuitive, as I said a few months ago,

The workflow is explained in the documentation (see help).
But it would be really good to have a screencast
displaying a typical workflow with the new interface,
when v3.0 is out.

@brunetton
Copy link
Author

You can include any #tag in the activity name, including numeric values like #999, but you have to end the activity[@category] by ,,, i.e. #123 fix this bug,,. (The double ,,, which has to precede actual tags, was introduced to satisfy another user who wanted to be able to include , in the activity.)

@mwilck The wiki page Entering activities was my attempt at getting the parser behaviour formally specified outside of the code. Unfortunately the idea was not fully adopted and the page is out of date w.r.t. the actual changes implemented.

Hi @GeraldJansen. I do not use tags, ever. So I do not want to add tags. I "just" (I know this is relative) want to include a # in the task description.

@brunetton
Copy link
Author

brunetton commented Jun 8, 2020

Hi @ederag thanks for your answer !

@brunetton Thanks a lot for the clear report.
As Gerald said, activity may start with a hash # and moreover,
the description may contain #123:
Screenshot_20200606_155842
Did you use the command line ?

I'm using UI, but in UI in add activity window, the field that I use is named cmdline so I'm not sure how to answer your question.

Please see #482 (comment) or the documentation (Help > input).
Screenshot_20200606_162624

I think I got your point, and I'll detail it for any user that comes in this thread:

  • if I enter 16:00 ,,#123 - fix thig bug, I get:
    • start time: 16:00
    • end time: None
    • activity: None
    • hash tag: #123 - fix thig bug
      => I'm not happy with that as I don't want any tag
      => IMHO in this case (activity is None), the starting # shouldn't be interpreted as a tag. The UI prevents from adding a fact without any activity anyway
  • if I enter 16:00 #123 - fix thig bug,,, (note the ,, at the end of the string) I get:
    • start time: 16:00
    • end time: None
    • hash tag: None
      => I'm happy with that 👍 BUT I feel it very weird and counter-intuitive to add ,, at the end of each entered fact

So your "long story" use case is already covered.

Thanks for pointing this out, I would probably never found this by myself !

The "short story" is an actual bug, but not in the parser itself, IMO.
The serialized string is not adequate in this case.
You may cherry-pick f302a57 that seems to fix it.
Note: it's not a fork, just a courtesy.

Thanks. But I'm confused here: I can't find this commit in branches history. Have this commit been deleted from master and is kind of dangling commit ?

Maybe all of this could be cancelled with a plugin system letting users code their parser.

It's tempting, but will end up with even more frustration for the user,
as designing a good parser is no small feat.
Our predecessors did a great job, and still there were corner cases.
I tried to follow, with two strong points:

  1. Few clear rules, no fragile heuristics.
  2. The instantaneous update of separate fields and cmdline, in the gui, as a way to quickly experiment,
    or discover the correct syntax.

I totally agree on this two points 👍, but letting users write their own parser (of course this do not prevent from using the default one, shipped with project) won't break those 2 rules IMHO

About the parser specification.
Beware of theory; it will be hard to avoid mismatches between the specification and the actual parser.

IMHO trying to re-implement a parser is reinventing the wheel, as tools that do the job great out of the box are available, even in Python stdlib.
Why not using this tool ? The only thing to do then is defining a grammar (that is clear and strict, and can be automatically transformed to documentation) ?

It is much more robust to define use cases, add tests for them, and document (which is mostly done).
This is no basic parser. The many use cases that accumulated along the years prevented that.

I understand this. But I doubt that many users need all use cases accumulated along the years. Do we ? Personally I need a very basic parser that I could write in 5mn with a regex (as I don't use tags and I'm fine with adding the category at the end of the "cmdline")

Summary

  • I'm still thinking it's impossible to find a syntax that makes everybody happy (come wants 123 at the beginning mean a duration, other don't, some want # means a tag, other don't ... how to reconcile all of those needs without creating frustrations ?)
  • starting from this principle, I think that we (lol I'm talking like if I have ever written one line of code of this project, sorry for that ;)) should let users redefine their own parser that feeds to their needs
  • why not using an existing parsing library available, especially made for that, robust, and clear ? (like the standard Python AST)
  • this would makes the code
    • more readeable
    • more robust
    • more maintenable
  • adapting the parser to everyone's needs would "only" resume to edit the grammar definition, witch is a lot simpler than modifying a custom parser code

Okay, I'm aware that I'm the guy that comes from nowhere, after years of project life, saying "just do this". But this is just an idea, presented with all my respect and humility !

@ederag
Copy link
Collaborator

ederag commented Jun 9, 2020

@brunetton
🤣 #465 (comment)...

Your post reminds me why I'm glad to be out.
You'll be in perfect company with the new maintainers.


Q: Hey my custom parser looks good, but the result is strange, could it be a bug in hamster ?
A: [after half-an-hour wrapping ones head into the custom parser] No, you forgot [this and that].

it very weird and counter-intuitive to add ,, at the end of each entered fact

It's not weird if you understand that ,, is a barrier: you tell to stop looking for tags (from the end).

Also, you may hit TAB to enter the description in the box below cmdline.

But you want a "no-tags mode", that's clear.
Maybe one day (it's possible), but there will be consequences.

Most people do not even state the version in their reports...
And @mwilck comment about configuration in gnome was an eye-opener.
(even if hamster is no longer gnome, there was wisdom in this statement)

Best wishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants