search

projecthamster / hamster-shell-extension

Shell extension for hamster
http://projecthamster.org
GNU General Public License v3.0
214 stars 91 forks source link

Starting tracker with single-comma delimited description doesn't work (with hamster 3.0.2) #334

Closed rhertzog closed 8 months ago

rhertzog commented 3 years ago

With hamster 3.0.2, if you type Activity@Category, task description in the shell extension, nothing happens. To get it to actually start, you need to type Activity@Category,, task description. This is likely related to the breaking change made in hamster that now requires two commas to separate descriptions: https://github.com/projecthamster/hamster/commit/42d3c053d4330df480091c40e9ebb8280ce25ea8

I think we should revisit that decision as it doesn't seem like a good decision to me. In particular when typing Activity@Category, #tag just works.

But if we don't, then we somehow need to fix the shell extension. Something needs to happen when the user inputs something... either it creates an activity with some unexpected values, or it displays an error. But just dropping the user's input is wrong.

matthijskooijman commented 3 years ago

If we want to fix this in hamster, we should probably discuss it there, I created https://github.com/projecthamster/hamster/issues/657 for that.

ederag commented 3 years ago

@rhertzog This change was not done lightly.

But you are right that the extension should follow, see my recommendations: https://github.com/getting-things-gnome/gtg/pull/465#issuecomment-711026830

rhertzog commented 3 years ago

@ederag I see that you have thought hard to support quite some corner case but I believe it's not the good trade off that you picked.

ederag commented 3 years ago

@rhertzog I'm not surprised that it might seem so at first sight, but no, the main starting point for the parser overhaul was much deeper.

No time to dive into my notes, but it was needed. Main points: readable code, simple rules, no fragile heuristics. The infrastructure is rock solid (not a bug about it surfaced in 9 months), thanks to this approach.

The "corner cases" came as a bonus.

mwilck commented 3 years ago

I can just repeat what I said before: the current rules of the "permissive parser" are overly complex. Too much complexity causes confusion, and the "double comma" ist just one particularly weird example for that.

It has all been done with good intentions, trying to satisfy user requests. Yet I believe it was misguided. Simplicity is key, the syntax rules have to be so simple that they can be understood at a single glance. IMO for the shell command line, they should match what command line users are used to (and that means: no whitespace in activity and category names). But I'm digressing and probably overdoing it.

The dilemma we have now is that in a way, the ship has sailed - 3.x has been out in the field with these parsing rules for almost a year. If we revert this now, we'll make some happy, and others who've come to like the new rules will be upset. We have no metrics to judge how far this adoption has gone yet, and what the final thumbs-up/down relation will be. But we need to choose one. Going for some compromise with yet new rules would confuse people even more.

My personal vote, no surprise here, is to revert this change.

hedayat commented 3 years ago

I'd suggest all discussion about reverting the change happen in projecthamster/hamster#657, so that everything is in the same place.

I'd suggest this issue is either fixed for the current released version, or is suspended until projecthamster/hamster#657 is resolved.

ederag commented 3 years ago

Agreed, the technical discussion should go to projecthamster/hamster#657, but here are a few comments relevant to this thread only (IMO):

  1. > you have thought hard to support quite some corner case Funny how some people like @rhertzog want their own features so bad, and call others "corner cases" :rofl:,
  2. The v3 rules are summarized here. So any reader can see what @mwilck called "complexity" above :rofl:.
    Obviously the change from single to double comma is annoying for some existing users (which does not make me laugh at all), but v3 rules actually removed many other sources of confusion,
  3. The main motivation was not to "satisfy user requests" (@mwilck never believes me, :sigh:),
    but to get an interface as clean, user-friendly, and robust as the infrastructure.
    Unfortunately this effort was stopped; but the parser is solid.

the ship has sailed - 3.x has been out in the field with these parsing rules for almost a year. If we revert this now, we'll make some happy, and others who've come to like the new rules will be upset.

True. For instance GTG migrated a while ago: https://github.com/getting-things-gnome/gtg/pull/465 Note that if you want to support v2 as well, you'll need to know which version you are talking to, so there's a new Version dbus method in hamster v3.

rhertzog commented 3 years ago 
3. The main motivation was not to "satisfy user requests" (@mwilck never believes me, :sigh:),
    but to get an interface as clean, user-friendly, and robust as the infrastructure.

Clearly we don't have the same definition of user-friendly. And if instead of laughing at my code, you'd have taken the time to read it, you would see that my changes actually make the parser more robust while not introducing the needless complexity of the required double-comma.

the ship has sailed - 3.x has been out in the field with these parsing rules for almost a year. If we revert this now, we'll make some happy, and others who've come to like the new rules will be upset.

True.

This is not true. If anyone uses double-comma, it will continue to work as usual with a single-comma as delimiter. We can even add some tests to ensure we properly strip the extra comma.

ederag commented 3 years ago

instead of laughing at my code

I did not.

And I also disagree with the remainder (you're missing the point). But go ahead, you've been warned.

matthijskooijman commented 8 months ago

This was fixed with merging https://github.com/projecthamster/hamster/pull/663, so I think this issue can be closed.

DirkHoffmann commented 8 months ago

This was fixed with merging projecthamster/hamster#663, so I think this issue can be closed.

Go for closing then. If this is an error, we can reopen it on request. (ping @rhertzog)

ederag commented 8 months ago

I'm astonished that you chose to break the v3 parser, which was perfectly robust.