projecthamster / hamster

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

UpdateFact DBus-Method not callable #671

Closed aquaherd closed 3 years ago

aquaherd commented 3 years ago

I am in the process of adding in-place edits to the xfce4-hamster-plugin but I am hitting a block:

If I call UpdateFact like this: 9241, "chores@homeoffice", 1615451700, 1615455000, False I get the following:

g-io-error-quark: GDBus.Error:org.freedesktop.DBus.Python.AttributeError: Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/dbus/service.py", line 707, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "/usr/libexec/hamster/hamster-service", line 257, in UpdateFact
    return self.update_fact(fact_id, fact, start_time, end_time, temporary)
  File "/usr/lib/python3/dist-packages/hamster/storage/storage.py", line 126, in update_fact
    self.check_fact(fact)
  File "/usr/lib/python3/dist-packages/hamster/storage/storage.py", line 58, in check_fact
    if fact.start_time is None:
AttributeError: 'dbus.String' object has no attribute 'start_time'
 (36)

Version 3.0.2. Everybody else seems to use the UpdateFactJson method. Should I stick to it instead or is there some way to make this API work?

ederag commented 3 years ago

If it worked and no longer does, it's a bug. Hold on, I'm a bit rusty (out for one year now), but see where it should be fixed.

ederag commented 3 years ago

Should work (blind guess, untested). Please Kill the hamster daemons, then (in a temporary directory) follow the installation instructions, except replace the download-source section with

git clone --branch=fix-UpdateFact-str https://github.com/ederag/hamster.git hamster-fix-UpdateFact-str
cd hamster-fix-UpdateFact-str

Please do ask for guidance if anything is unclear.

aquaherd commented 3 years ago

thank you. will check.

aquaherd commented 3 years ago

Confirmed: Works for me. Please merge to upstream.

ederag commented 3 years ago

Thanks a bunch for the perfect report and the test !

I gave up trying to merge fixes. But anyone is welcome to cherry-pick b35a3d61 and create a PR.

By the way, isn't #590 fixed as well ? (the branch you checked is based on my other fix)

ederag commented 3 years ago

Everybody else seems to use the UpdateFactJson method.

It depends on your plugin.

If it has only a single cmdline, then users expect it to match the terminal command line, and UpdateFact is right. (if you want to enter a description, please refer to the help or to #482 for the new double comma barrier)

Otherwise, UpdateFactJson gives you a better control over the Fact (fields are passed verbatim) and might be more natural, for instance if you have a separate description field.

aquaherd commented 3 years ago

Working with Json from C is another dependency on top of GVariant-wrapped DBus that I would rather avoid. Reading hamster code just got me the best solution:


            if (!strcmp("fact", type))
               fact = hview_get_fact_by_activity(view, val);
            else if(!strcmp("date", type))
               duration = val;
            if (hview_span_to_times(duration, &start_time, &stop_time))
            {
               // hamster #671 merged upstream?
               if (hamster_call_update_fact_sync(view->hamster, id, fact, start_time, stop_time, FALSE, &id, NULL, NULL))
               {
                  DBG("UpdateFact worked: new id=%d", id);
               }
               else
               {
                  DBG("UpdateFact did not work: remove, then add fact #%d", id);
                  hamster_call_remove_fact_sync(view->hamster, id, NULL, NULL);
                  hamster_call_add_fact_sync(view->hamster, fact, start_time, stop_time, FALSE, &id, NULL, NULL);
                  DBG("UpdateFact worked: %d", id);
               }
            }
            else
            {
               DBG("Duration '%s' did not parse.", duration);
            }

It looks like UpdateFact just does this inside a DB-Transaction:

Did not understand the UpdateFactJSon difference, though.

I rather expected an old/new comparizon and then the usual update fact from facts where id={0} values starttime={1}, ... or whatever SQL magic would be most efficient.

I was not able to confirm #482 with my panel plugin.

image

ederag commented 3 years ago

Working with Json from C is another dependency...

Understandable. I'd rather construct the Json string "by hand", indeed (it should be easy). But it looks like your interface is the "command line" type, so you are all set already.

And your workaround looks good, indeed (clever !).

I was not able to confirm #482

This one I do not understand. Did you mean that #590 is fixed instead ? Otherwise, would you please elaborate ?

aquaherd commented 3 years ago

Yeah sorry I meant #590 since I only deal with today's facts.

Never worked the midnight shift so can't tell really how it behaves when a fact leaks onto the next day.

Btw., I have a hamster-DB that goes all the way back to October, 2013. I have been maintaining my plugin since late 2014.

I stuck to the gtk+2 versions of both until it was no longer possible, when xfce4.16 finally dropped gtk+2 support. Only since then I am slowly making inroads to gtk+3... The port is rather stable and I am trying to get it into Void Linux (together with Hamster) and would like to see both in Alpine Linux, too.

For me, Hamster always delivered as is. The only thing I would ask of it is to drop the 'Gnome' in its name since I am using it under every desktop I use, e.g. dmenu-driven under i3, sway and bspwm; xfce4-driven under vmWare and WSL1 and so on.

I would also like to see a Windows/Mac OS/iOS port one day - none of the platforms I would expect to gnome to run at. And from how I experience it, python actually excels at being cross-platform.

ederag commented 3 years ago

Yeah sorry I meant #590 since I only deal with today's facts.

Thank for the confirmation that at least it does not seem to break anything.

Never worked the midnight shift so can't tell really how it behaves when a fact leaks onto the next day.

The "midnight shift" is not the core issue. It's just where other "fixes" failed. Mine succeeds, but it's secondary. With the new v3 interface, it is possible to safely experiment with this in the "edit activity" window (just do not hit the save button).

The port is rather stable and I am trying to get it into Void Linux (together with Hamster) and would like to see both in Alpine Linux, too.

Great !

I am using it under every desktop I use, e.g. dmenu-driven under i3, sway and bspwm; xfce4-driven under vmWare and WSL1 and so on.

Indeed, and I'm using KDE :slightly_smiling_face:

I would also like to see a Windows/Mac OS/iOS

https://github.com/projecthamster/hamster/issues/89#issuecomment-482957460 But beware, the burden is already high on maintainers. Maintenance of this project is not as simple as it looks, as the new team should have reckoned by now.

https://github.com/projecthamster/hamster/issues/222#issuecomment-601910766 But they did not merge #573 in time (lack of time + totally wrong reasons), and now there have been incompatible changes. The branch you checked has it, and should work fine on MacOS.