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

Allow bypass (0) and override to None (-1) start/end in DBus AddFact #491

Closed
ederag opened this issue Dec 11, 2019 · 9 comments · Fixed by #492
Closed

Allow bypass (0) and override to None (-1) start/end in DBus AddFact #491

ederag opened this issue Dec 11, 2019 · 9 comments · Fixed by #492
Milestone

Comments

@ederag
Copy link
Collaborator

ederag commented Dec 11, 2019

#490 seemed to fix the shell-extension (projecthamster/hamster-shell-extension#316 (comment)),
but that overrides the parsed fact start_time with hamster_now as a default.

Here is the 2012 state:

hamster/src/hamster-service

Lines 117 to 121 in 80110af

@dbus.service.method("org.gnome.Hamster", in_signature='siib', out_signature='i')
def AddFact(self, fact, start_time, end_time, temporary = False):
start_time = dt.datetime.utcfromtimestamp(start_time) if start_time else None
end_time = dt.datetime.utcfromtimestamp(end_time) if end_time else None
return self.add_fact(fact, start_time = start_time, end_time = end_time) or 0

self.start_time = start_time or self.start_time or None
self.end_time = end_time or self.end_time or None

so start and end times were taken from the parse string, and overridden only
if the separately given start_time and end_time were non-zero.

This prevents overriding the end_time with None for instance.

Proposition

Use 0 to mean "do not mess around with the parsed fact"
(just ignore this argument)
Use -1 as a special value to mean "replace with None"

This way the full power of the new parser (including datetimes and relative times parsing)
can be exploited in the shell-extension as well.

For instance one might try this branch:
https://github.com/ederag/hamster/tree/AddFact-parse-times

@jmberg
Copy link

jmberg commented Dec 11, 2019

Hmm. So the gnome shell extension doesn't include any times in the string at all, as far as I can tell. Would that still use "hamster_now()" then

Of course the user could enter "10:00 something" but that's not really suggested in the shell extension UI - other places pre-fill the current time there and put the caret after it to indicate that you can change this around, but the shell extension just has a single entry field and even labels this "What are you doing?", and auto-completes only the fact name, not any times.

(Technically, you could even argue that one should be able to enter a fact like "10:00 meeting" where the "10:00" is part of the fact name, not a time. But neither does that really matter, nor do I have any idea how that would've behaved previously. I assume it would've parsed it already for a long time.)

AddFact-parse-times branch looks like it should work, but I can only test it later.

@mwilck
Copy link
Contributor

mwilck commented Dec 11, 2019

Agreed with @jmberg, the typical usage of the extension is to enter only an activity name or activity@category. With the current extension and hamster 2.2.2, it's actually possible to enter a start time via the extension's text input field, but not an end time (it isn't parsed correctly, at least). Also, as @jmberg noted, there's no autocompletion if a start time is used in the input field. I have never used this, I always enter just the current activity, and if I need to fix the start time, I click the "edit" button to start the hamster UI and apply changes there. Admittedly not the perfect workflow, but I've got used to it.

@jmberg
Copy link

jmberg commented Dec 11, 2019

Ther'es a button for "Add earlier activity", so arguably that's how you should enter something with start/end time :-)

I think the AddFact-parse-times branch will actually make that work though, if I'm reading it correctly, I just don't think it's really important in any way.

@mwilck
Copy link
Contributor

mwilck commented Dec 11, 2019

Ther'es a button for "Add earlier activity", so arguably that's how you should enter something with start/end time :-)

And that button simply opens the hamster "add activity" UI, does it not?

@jmberg
Copy link

jmberg commented Dec 11, 2019

Ther'es a button for "Add earlier activity", so arguably that's how you should enter something with start/end time :-)

And that button simply opens the hamster "add activity" UI, does it not?

Yes.

@ederag
Copy link
Collaborator Author

ederag commented Dec 11, 2019

Thanks for the explanations.
The experimental branch has just been updated to 7ab1d3c.
It should be backward compatible with existing workflows,
while allowing to enter start and end times as well, for people interested.

Looking at the shell-extension code,
the completion is indeed based only on the beginning of the string,
so better choose the activity first, then go to the beginning (home key ?),
and add time.

Alternatively, it would be possible to accept time at the end ("tail" position),
like on the command line, with the new parser.
But that would break this (the old parser looks for time at the "head" position):

it's actually possible to enter a start time via the extension's text input field

We shall see how to make a transition if there is an interest someday.

@jmberg
Copy link

jmberg commented Dec 11, 2019

I tested it now, it mostly works, you only forgot

from hamster.lib.stuff import hamster_now

in src/hamster-service.

@ederag
Copy link
Collaborator Author

ederag commented Dec 11, 2019

Thanks ! I need to rest... Forced-push to the same branch (3c4c268)

@ederag
Copy link
Collaborator Author

ederag commented Dec 14, 2019

@jmberg Thanks, should be fixed in #492 ?

@ederag ederag changed the title DBus AddFact Allow bypass (0) and override to None (-1) start/end in DBus AddFact Dec 14, 2019
@ederag ederag added this to the v3.0 milestone Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants