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

UpdateFact DBus-Method not callable #671

Closed
aquaherd opened this issue Mar 11, 2021 · 10 comments · Fixed by #672
Closed

UpdateFact DBus-Method not callable #671

aquaherd opened this issue Mar 11, 2021 · 10 comments · Fixed by #672

Comments

@aquaherd
Copy link
Member

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?

@aquaherd aquaherd changed the title UpdateFact DBuss-Method not callable UpdateFact DBus-Method not callable Mar 11, 2021
@ederag
Copy link
Collaborator

ederag commented Mar 11, 2021

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
Copy link
Collaborator

ederag commented Mar 11, 2021

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
Copy link
Member Author

thank you. will check.

@aquaherd
Copy link
Member Author

Confirmed: Works for me. Please merge to upstream.

@ederag
Copy link
Collaborator

ederag commented Mar 12, 2021

Thanks a bunch for the perfect report and the test !

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

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

@ederag
Copy link
Collaborator

ederag commented Mar 12, 2021

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
Copy link
Member Author

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:

  • Remove fact
  • add new fact
  • return changed ID.

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
Copy link
Collaborator

ederag commented Mar 12, 2021

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
Copy link
Member Author

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
Copy link
Collaborator

ederag commented Mar 12, 2021

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 🙂

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

#89 (comment)
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.

#222 (comment)
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.

matthijskooijman pushed a commit to aquaherd/hamster that referenced this issue Mar 13, 2021
This method was written to handle either a Fact object or a string
description of a fact. However, the string-to-Fact conversion happened
too late in the method, after `check_fact()` was called, which requires
a Fact object, leading to an error like:

    AttributeError: 'dbus.String' object has no attribute 'start_time'

This happened in particular when using the DBus UpdateFact method, which
always passes a string to `update_fact()`.

This commit ensures that the string-to-Fact conversion is done
immediately, so the rest of the method can just assume there is a Fact
method.

This fixes projecthamster#671.
matthijskooijman pushed a commit to aquaherd/hamster that referenced this issue Mar 13, 2021
This method was written to handle either a Fact object or a string
description of a fact. However, the string-to-Fact conversion happened
too late in the method, after `check_fact()` was called, which requires
a Fact object, leading to an error like:

    AttributeError: 'dbus.String' object has no attribute 'start_time'

This happened in particular when using the DBus UpdateFact method, which
always passes a string to `update_fact()`.

This commit ensures that the string-to-Fact conversion is done
immediately, so the rest of the method can just assume there is a Fact
method.

This fixes projecthamster#671.

Thanks to ederag for this commit, and to Hakan Erduman for submitting it
in a PR.
aquaherd added a commit that referenced this issue Mar 13, 2021
#671: convert str to Fact before check
This was referenced Mar 13, 2021
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.

2 participants