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]

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 https://github.com/projecthamster/hamster/pull/490 ? Let's continue there.

[ederag]

Well, that could be much better actually. @mwilck, @jmberg could you please comment on https://github.com/projecthamster/hamster/issues/491 ?

[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 https://github.com/projecthamster/hamster-shell-extension/pull/312#issue-277571538 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 https://github.com/projecthamster/hamster-shell-extension/issues/315#issuecomment-586685378 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: https://github.com/projecthamster/hamster-shell-extension/issues/319

[mwilck]

Wrt GNOME shell compatibility see also this comment.

[ederag]

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.

Confirmed, glad to read that this is clear.

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.

The recommended way is outlined in https://github.com/projecthamster/hamster/issues/493 This would reuse existing code, and would work on all desktops, not just gnome. That would be already functional if things were going more peacefully (https://github.com/projecthamster/hamster/issues/574 for the latest one).

[mwilck]

The recommended way is outlined in projecthamster/hamster#493 This would reuse existing code, and would work on all desktops, not just gnome.

I'm not sure. I believe that some of the functionality can be generalized across (some) desktops, and some can't. Imagine how many desktops are in use in the Linux ecosystem :-/

IIUC some these features have worked only on GNOME in the past (after all, hamster once used to be a dedicated GNOME tool). The idle detection was using the GNOME-specific DBus signal org.gnome.ScreenSaver.ActiveChanged(), and this is what GNOME shell 3.34 still seems to be emitting on my system. There's also org.freedesktop.ScreenSaver, but I don't observe it on my system, and actually it seems to be only a collection of stubs.

We might as well consider hamster "plugins" for different desktops with some code duplication initially, and start using common code once we see what actually qualifies. The problem that I see here is packaging / shipment. @ederag, you didn't want this code in hamster core any more, so we'd need to package it separately somehow, which is more difficult (packaging and shipment-wise) than just adding code to an existing package.

That's why I said we could add this functionality to the GNOME extension. Adding a handler for the screensaver signal and calling the hamster method to stop the current activity from there would be pretty straightforward, even for a Javascript noob like myself.

[ederag]

Your call; our views diverge. But was just waiting for that kind of proposition to happen, to make sure the information was given once. If you have questions, mention me (I can't follow any longer, too much distracting activity). That being said, it's good to see work on the extension resuming, go ahead.

[DirkHoffmann]

This merge request has actually been reviewed by @matthijskooijman already above, @mwilck, @benjaoming. To my understanding it could be confirmed very quickly now. His remark about convenience.js has been implemented, IIUC. I would be in favour of doing that, closing this PR and implementing improvements in a next step/issue. What do you think?

[mwilck]

@DirkHoffmann, I'd like to figure out the issue with 3.32 vs. 3.34. I still have hopes that we can cover both releases with a single version. Also, if anyone is on Ubuntu 20.04 already, I'd like to find out about 3.36. Not that it would be a blocker if that didn't work, but it'd be good to know.

[benjaoming]

@mwilck an alternative: Merging your PR (into develop) and opening issues and milestones about adding 3.32 and 3.36 support? This way, people can benefit from the current changes and other things can be merged/added/discussed on top of this work?

[mwilck]

@benjaoming, yes, that makes sense. In the "worst case" we could branch 3.32 from 0fc670a.

However, I still see "You’re not authorized to merge this pull request", so I'll not be able to do it.

[benjaoming]

Yes, review approval doesn't change anything.

[mwilck]

I changed the merge base from "master" to "develop", but still I lack the rights to merge.

[DirkHoffmann]

I changed the merge base from "master" to "develop", but still I lack the rights to merge.

Same for me. :confused:

[mwilck]

"The base branch requires all commits to be signed."

@hedayat, this branch contains commits by you and me, and yours are first. In order to be able to merge this, you would first need to re-push your branch (hedayat/master, d748dcc) with signed commits, so that I could rebase onto that and sign my own commits. See e.g. https://superuser.com/questions/397149/can-you-gpg-sign-old-commits.

Unless @elbenfreund would grant us an exception, that is.

[mwilck]

I've re-pushed this branch with signed commits (by myself, @hedayat's yet missing). The new head 0ff430d is identical to the previous 12c3b11.

[mwilck]

Well, if I'm rebasing anyway, I could as well skip the reverted 83d2dd8 / ec07cff, objections anyone?

[elbenfreund]

Just to reiterate and avoid frustrations: If you want me to relax the restriction, I am happy to. just let me know...

[mwilck]

Just to reiterate and avoid frustrations: If you want me to relax the restriction, I am happy to. just let me know...

Thanks, let's wait a bit for @hedayat. There's also the authorship issue (see #312), I'll make an attempt to resolve it.

[DirkHoffmann]

I admire your relentless attempts to solve this pull request within the given formal constraints of github, @mwilck! :+1:

[matthijskooijman]

To be honest, I would consider removing the requirement for signed commits. It is a good idea, but I'm afraid that the average (or at least a casual) contributor to this repo might be put off by such a requirement (and with many gnome-shell versions to support, we might very much need casual contributors...). Of course, maintainers could maybe thoroughly review and sign commits submitted by others (which would maybe defeat the traceability feature of signed commits, but retain the security I guess)?

[mwilck]

The kernel uses signed tags. IMHO that would be good enough for hamster, too. Or maybe, require a signature on the topmost commit in a PR. As the commits are chained through their hashes, that would be quite a good verification for the other commits in the PR, too.

[DirkHoffmann]

I agree that these formalities put the bar quite high for the moment. In a couple of days we will know, if it is too big a challenge for casual contributors.

[elbenfreund]

I created #322 as a place to discuss those policies. For now, all restrictions are removed (except preventing force pushes and deletions). While I have opinions on the validity of those restrictions, its not like we are loosing anything by disabling right now as they have not been in place so far (iirc) anyway. Hope that helps with moving on and getting work done.

[hedayat]

I"m probably late. BTW, I'll see if I can push signed commits very soon; although it seems to not be required now. But don't wait for me! I can't predict myself if I'll do it till tomorrow. :P

[benjaoming]

@elbenfreund do you know what this CLA stuff is about? I thought we would stop taking initiatives without consulting the community.

[elbenfreund]

@elbenfreund do you know what this CLA stuff is about? I thought we would stop taking initiatives without consulting the community.

I didn't. As explained in another issue, this was a leftover from days long gone, that got triggered by my recent work on hamster-*. I have removed the link to the hamster organization. There should be no further intrusion. Please let me know if it pops up again.

[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.