GNOME 3.34 support

[mwilck]

This is based on my previous GNOME 3.32 PR (#312). It fixes issue #315 (and #307, too). The changes on top of #312 are small.

[jmberg]

The reason was probably that in 3.32 shell, this is actually not possible - it says something like "gobject.registerClass used with invalid base class ..."

With that reverted, I have the hamster extension working on 3.32 again - thanks a lot!

(Upgrading to 3.34 is also WIP)

[jmberg]

Hm, did you test this on 3.34? I can actually see the popup and the data etc., but cannot enter anything to change the task I'm working on. The shell's "lg" doesn't show any errors.

[mwilck]

Hm, did you test this on 3.34?

Sure I did. I am using it under 3.34 every day. Check your "journalctl --user" output.

[jmberg]

Hm, did you test this on 3.34?

Sure I did. I am using it under 3.34 every day. Check your "journalctl --user" output.

OK, just asking :-)

I can stop the current activity, but neither the triangle "play" button on a previous activity, nor the input field actually work. Nothing logged in the journal, nor in the lg. Any other debug hints?

[mwilck]

Hm, strange. Whenever I had problems like that, I also had some errors in the log. What I did in similar situations was add debug output in various places in the extension, and try to spot issues that way.

[jmberg]

I think it's an issue with me having installed a very new version of the hamster service, dbus-monitor showed me an issue:

method call time=1575999007.966957 sender=:1.152 -> destination=:1.48 serial=384 path=/org/gnome/Hamster; interface=org.gnome.Hamster; member=AddFact
   string "asdf"
   int32 0
   int32 0
   boolean false
error time=1575999007.969080 sender=:1.48 -> destination=:1.152 error_name=org.freedesktop.DBus.Python.AssertionError reply_serial=384
   string "Traceback (most recent call last):
  File "/usr/lib64/python3.7/site-packages/dbus/service.py", line 707, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "/usr/lib64/hamster/hamster-service", line 141, in AddFact
    return self.add_fact(fact) or 0
  File "/usr/lib/python3.7/site-packages/hamster/storage/storage.py", line 63, in add_fact
    result = self.__add_fact(fact, temporary)
  File "/usr/lib/python3.7/site-packages/hamster/storage/db.py", line 556, in __add_fact
    self.validate_fact(fact)  # sanity check
  File "/usr/lib/python3.7/site-packages/hamster/storage/storage.py", line 92, in validate_fact
    assert fact.start_time, "missing start_time"
AssertionError: missing start_time
"

[jmberg]

Yes, looking at the code, it wants a start_time as non-0 at least, so I guess we need to pass the current time.

Really the code wants to have this pre-parsed and called with some kind of dict as the first arg

https://github.com/projecthamster/hamster/blob/master/src/hamster/storage/storage.py#L44

[jmberg]

This makes it kinda work

https://p.sipsolutions.net/baba42547992ef59.txt

I guess I need to check further, the rounding is odd. But without it, the overview window (not in the applet, the normal window) shows "NEGATIVE" during the first ~minute...

[mwilck]

So you're using the latest hamster? That makes sense then. I'm still at 2.2.2 here. Feel invited to create a PR against my repo for the GNOME extension.

[jmberg]

Out of necessity in a sense, I upgraded to Fedora 31 which removed the packages due to python2 dependency, so I just pulled master and installed it myself ... that doesn't even have python2 dependency anymore, but evidently also broke/changed other things.

I'm wonder if I shouldn't create a PR against hamster instead to fix the behaviour though.

[mwilck]

I'm wonder if I shouldn't create a PR against hamster instead to fix the behaviour though.

You can call it an API change, for sure. Not sure how much @ederag cares about compatibility with the GNOME extension. I don't think he is using it himself.

[jmberg]

You can call it an API change, for sure. Not sure how much @ederag cares about compatibility with the GNOME extension. I don't think he is using it himself.

It affects every D-BUS client though... I dunno. I guess I have the local hack for now while I continue to ponder ...

[mwilck]

I doubt there are relevant other Dbus clients that are not part of hamster itself.

[jmberg]

Fair enough.

[mwilck]

... but that's a good point, the Dbus API shouldn't be changed without good reason.

[ederag]

Not sure how much @ederag cares about compatibility with the GNOME extension. I don't think he is using it himself.

Not using it, but compatibility does matter.
@jmberg Thanks for the early testing and the patch.
Could you please try projecthamster/hamster#490 ?
Let's continue there.

[ederag]

Well, that could be much better actually.
@mwilck, @jmberg could you please comment on projecthamster/hamster#491 ?

[matthijskooijman]

I looked through this PR, the code looks good to me. I also tested it briefly, seems to work as expected on 3.34.

I left one minor comment that is easy to fix. I would say it would be good to merge this, is there anyone other than @elbenfreund that can and wants to do this still? @ederag maybe?

[matthijskooijman]

This change removes the (indirect) dependency from collect to $(BUILDDIR). If you now run make clean-build and then make compile, you get cp: target 'build' is not a directory. Could you maybe make collect depend on $(BUILDDIR) again?

[ederag]

Can't do anything. Got no rights on this repo.
The shell-extension is looking for a maintainer (#315 (comment)).

[mwilck]

OK. Pushed an update. Also, reverted the patch that skipped zip file creation.

[ederag]

Since there is movement, and that will me merged soon,
Please do not forget to add to the merge commit
(remove the @ from @ernestask to avoid notifications from rebases):

It is almost equivalent to #308, and very heavily based on ernestask's work there.

This is from @mwilck's comment #312 (comment) that is linked in the top message,
but was not repeated and thus might easily be forgotten.

[matthijskooijman]

Now that there is some movement in the repository access front, I think this PR would be a good candidate to merge (though I have not really looked at others much).

However, @DirkHoffmann suggests in #315 (comment) that this branch/PR does not work on gnome 3.32, only on 3.34 (but I might be misreading his comment). IMHO it would be good (and I think possible) to support both 3.32 and 3.34 with a single codebase.

Also, before merging this, it might be useful to do a rebase to clean up history a bit (I see there is a merge commit in here, which would probably be better if it was flattened).

[DirkHoffmann]

Thank you, @matthijskooijman,

However, @DirkHoffmann suggests in #315 (comment) that this branch/PR does not work on gnome 3.32, only on 3.34 (but I might be misreading his comment). IMHO it would be good (and I think possible) to support both 3.32 and 3.34 with a single codebase.

Indeed, we will have to solve the problem of incompatibility between (at least) gnome-shells 3.30, 3.32 and 3.34.
I think it is fair enough to say that we do not know how to handle this ambiguity technically (on git/github) in the ideal way: tags, branches, something else? Any input from more experienced people is welcome.

For the sake of it: The hamster-shell-extension is located in the middle of two APIs: hamster (dbus) on one side and gnome-shell on the other. The compatibility matrix is likely to cause us some headaches in the future. But we start with optimism.

[mwilck]

Indeed, we will have to solve the problem of incompatibility between (at least) gnome-shells 3.30, 3.32 and 3.34.
I think it is fair enough to say that we do not know how to handle this ambiguity technically (on git/github) in the ideal way: tags, branches, something else? Any input from more experienced people is welcome.

IMO branches are the way to go. Changes for recent GNOME version should go into the master branch. If that breaks compatibility with previous GNOME versions, a compatibility branch should be created for the respective GNOME version. If any non-GNOME-API-related fixes are added to the master branch (I don't think this has happened during the last 2 years), they can be cherry-picked onto the compatibility branches. As an openSUSE Tumbleweed user, my personal main interest will be focused on the latest releases (but don't and won't test with unstable/odd-numbered GNOME releases).

The hamster-shell-extension is located in the middle of two APIs: hamster (dbus) on one side and gnome-shell on the other.

Fortunately, @ederag has committed himself to keep the hamster side of the API stable for the time being, so there's not so much to worry about. One exception: hamster 3.0 has removed some "GNOME specific" parts of hamster code such as the idle detection, so we may need to figure out how to add this functionality back. I guess it could be done in the extension itself.

[benjaoming]

@mwilck great that you bring this up -- this has been my idea for a while, too, except that it seemed pretty useless to bring up back when no one had rights to the repository anyways :) Created this separate issue: #319

[benjaoming]

Thanks - so I guess anyone with admin rights should consider deleting the CLA comments that have been created in currently open PRs.

[elbenfreund]

I went over all open PRs for hamster and hamster-shell-extension and removed the bot comment.
If I missed one, feel free to ping me.
It seems that the (non mandatory) check entry is not refreshed automatically. I suspect this will happen once a new push happens. I hope this is alright? If you click on "details" you will find that "no CLA is found "...

[mwilck]

Rebased to current develop, necessary because of the recent merge of #310.

[mwilck]

@ernestask hasn't responded to my questions (both on #312 and via email) on how he'd like his authorship to be acknowledged besides what's already in the commit messages.

I'm inclined to merge here, and go forward with #323 afterwards (to be clarified if the 3.36 changes are compatible with 3.34, or if we need to branch).

If anyone disagrees, speak up now.

[mwilck]

Rebased on code base of #312.

[mwilck]

I've tried Fedora 30 / GNOME 3.32 now. This version is not compatible with GNOME shell 3.32.
The breakage is caused by e866154 (which is required by GNOME 3.34). Error message:

gnome-shell[7033]: Extension "[email protected]" had error: TypeError:GObject.registerClass() used with invalid base class (is PopupBaseMenuItem)

(This was the "reason I don't remember" in the commit msg of e866154).

Consequently, #323 is also not compatible with 3.32. I will push corresponding updates when @hedayat has finished his rebase on #299.

[mwilck]

This means that we'll need compatibility branches for 3.30, 3.32, and 3.34, where the latter supports 3.36 too. I guess we should also use the current develop (16b7a0e) as branch for GNOME shell 3.28 and older. PR history suggests that that code base used to work until 3.30, where #299 was needed.

[matthijskooijman]

Would it be possible to make that commit compatible with 3.30? That would save us a branch. E.g. something like:

var FactsBox =
class FactsBox extends PopupMenu.PopupBaseMenuItem {
    constructor(controller, panelWidget) {
        super({reactive: false});
        actual_constructor(controller, panelWidget);
    }
    _init(controller, panelWidget) {
        super._init({reactive: false});
		actual_constructor(controller, panelWidget);
    }
    actual_constructor(controller, panelWidget) {
...
};
if (GNOME < 3.34)
    const FactsBox = GObject.registerClass(FactsBox);

Maybe the GNOME version check could be more specific by checking whether FactsBox is valid for passing to registerClass, or we could just pass it and ignore this specific error if it occurs.

I'm not familiar enough with javascript to actually tell if this would work,
though.

If this does not work, maybe the commit message of that commit (and maybe also e2a5edf) should be updated to say that FactsBox was not converted to GObject because its superclass was not converted to GObject in gnome-shell yet (since that's I think how it is)?

[mwilck]

Would it be possible to make that commit compatible with 3.30? That would save us a branch.

In theory it can be done, but most extensions seem to choose branching over the headache of maintaining this sort of complex code. It doesn't tend to get better over time. I'd recommend this only for very small changes, like one-liners, and definitely not for class definitions.

For this particular case, I don't like the idea to start experimenting again. We've got working code bases for every GNOME shell release since 3.30 at least, and I'd like to get this done and out to e.g.o. ASAP. If someone figures out an elegant, easy-maintainable solution to unify the code and merge two branches again, I'm not going to object.

If this does not work, maybe the commit message of that commit (and maybe also e2a5edf) should be updated to say that FactsBox was not converted to GObject because its superclass was not converted to GObject in gnome-shell yet (since that's I think how it is)?

I'll certainly update the commit message for e866154, and sure, I can also do it for e2a5edf. It looks as if the commit that changed PopupBaseMenuItem was js/status: Use menu items as actors, added in GNOME shell 3.33.2.

[mwilck]

PR update: updated commit message of 13457c0 to reflect this comment (previous: 7e2d9e0). Moreover, merged the 3.32 changes from develop branch (README.rst only), marked GNOME 3.32 as unsupported in metadata.json, and updated GNOME support information in the README.