projecthamster / hamster-shell-extension

Shell extension for hamster
http://projecthamster.org
GNU General Public License v3.0
214 stars 91 forks source link

Gnome 3.36 support #323

Closed mwilck closed 4 years ago

mwilck commented 4 years ago

This is an attempt to fix hamster shell extension under GNOME 3.36 (#321). It is based on my PR for GNOME 3.34 (#316).

CAVEAT: The extension needs to be compiled with a non-standard UUID to work:

make dist UUID="some_uuid@some.domain"

Consequently, it must also be installed under $HOME/.local/share/gnome-shell/extensions/some_uuid@some.domain, and it will be necessary to enable it using gnome-tweaks or the new GNOME "Extensions" tool. Any older versions of the hamster shell extension should be purged before trying this one.

See https://github.com/projecthamster/hamster-shell-extension/issues/321#issuecomment-59789027; it seems as if the extension with UUID contact@projecthamster.org is currently marked as "blacklisted" on extensions.gnome.org.

mwilck commented 4 years ago

@projecthamster/gnome-shell-extension reviewers, the new commits start at 6970284.

mwilck commented 4 years ago

This isn't perfect yet, the today's facts list is too wide. But at least it works now.

gnome-3 36

mwilck commented 4 years ago

This isn't perfect yet, the today's facts list is too wide.

it's looking OK today, no idea why.

gnomeshell-3 36

mwilck commented 4 years ago

Got the reason for the "wide popup window" now - it's the summary line at the bottom (above "Show Overview") which causes the window to widen. We probably should truncate that output to the biggest 3 contributors. It has nothing to do with GNOME 3.36.

mwilck commented 4 years ago

@matthijskooijman, for some reason I can invite Dirk and Ben for review, but not you. So please, feel invited, too :smile:

matthijskooijman commented 4 years ago

@matthijskooijman, for some reason I can invite Dirk and Ben for review, but not you. So please, feel invited, too smile

I think you can by just typing my full username, but I'm probably not suggested because I don't have write access to this repo. Regardless, I should really be doing other things right now :-)

DirkHoffmann commented 4 years ago

@mwilck wrote:

We probably should truncate that output to the biggest 3 contributors.

Or line-break?

DirkHoffmann commented 4 years ago

@matthijskooijman, for some reason I can invite Dirk and Ben for review, but not you. So please, feel invited, too :smile:

No way to add @matthijskooijman to the reviewers (even typing his UID explicitely) although he can be made assignee (which is not what we need in this case). A question to @elbenfreund once more? Can you check the configuration, please? Independently from the fact that we may or may not want to add Matthijs as maintainer, can he become "only" a reviewer by a correct parametrisation of github? BTW Matthijs is reviewer of #316, which should have the same constraints as this PR, shouldn't it?

PS: Asked github help. Faithfully.

matthijskooijman commented 4 years ago

Github docs suggest that anyone with "read access" to a repo can be asked for review: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/requesting-a-pull-request-review They do not specify what that means for public repos, though. In another public repo I have admin access to, I can request a review from any organization member, it seems, but not anyone else. So that would suggest I should end up in this list too. I also looked through repo and organization settings on that other org, which do not show any obvious things to configure about the review requests.

BTW Matthijs is reviewer of #316, which should have the same constraints as this PR, shouldn't it?

There, I just did a review unrequested (which anyone can do on a public repo, AFAIK).

So, I would suggest to just leave this for now. If you want to request review from someone, you can always tag them in a comment :-)

mwilck commented 4 years ago

FTR, @xMordax provided positive feedback in https://github.com/projecthamster/hamster-shell-extension/issues/321#issuecomment-598630028.

So basically all we need to do (except finishing up #316) is to clarify the auto-disable issue with the contact@projecthamster.org UUID.

mwilck commented 4 years ago

@DirkHoffmann ,

Or line-break?

I'd rather not, and defintiely not in the framework of the PR. How many lines would you want to allow? This is for quick overview only, all details can be gathered from the hamster overview window. If I was to change this line, I'd add a "total" for the day.

DirkHoffmann commented 4 years ago

If I was to change this line, I'd add a "total" for the day.

You are right.

If we ever were to make it better, probably a second table (ehem grid) section with the summaries by category would be more appropriate. (But not for this issue. Sorry, I am hijacking.)

mwilck commented 4 years ago

Btw, for the time being I changed the GNOME shell compatibility list in metadata.json to just 3.35 and 3.36. I did so because of the trouble I experienced with contact@projecthamster.org; it was part of my attempts to fix that weird behavior of the shell. But if you look at the code changes, that restriction is most probably not necessary. I believe this version would function with 3.34 just fine. I'd be grateful for feedback on this. If it does, we could spare a branch.

DirkHoffmann commented 4 years ago

Github docs suggest that anyone with "read access" to a repo can be asked for review

I got a (quick!) answer from github help (behind a quickly expiring link though, so I cite here):

I believe this happens due to repository permissions. The current permissions don't stop matthijskooijman from leaving a review, but does stop requesting one from him.

Do you confirm, @matthijskooijman? You can submit a review, but we cannot request you to do so through the mechanism in the menu bar at the upper right?

matthijskooijman commented 4 years ago

Do you confirm, @matthijskooijman? You can submit a review,

This is correct. As I mentioned, I think anyone can actually review PRs in a public repo.

Also, as I mentioned, I don't have much time for a review right now. But I did previously review the 3.34 PR, so maybe that should be merged separately first, which should also remove most of the commits from this PR, which makes reviewing easier?

mwilck commented 4 years ago

We have figured out the probably reason for the "blacklist" status in #324. It's fixed with the last commit I pushed (027b558). By setting the version in metadata.json to a valid integer > 2, the problem is avoided.

mwilck commented 4 years ago

Removed "caution, special instructions", because this isn't necessary any more after figuring out the "version" issue.

ederag commented 4 years ago

@mwilck Your https://github.com/projecthamster/hamster-shell-extension/pull/312#issue-277571538 acknowledging of @ernestask is now 2 jumps way from this PR. I take your comment as search for advise to make this as right as possible.

The #323 top post could be edited, saying that this is "very heavily based on @ernestask's work (#308)". Then the merge commit message could contain the same information. (just without the @, otherwise when github goes crazy that would trigger a lot of notifications)

Proceeding like this would emphasize that you meant no harm, and just wanted to speed up review and acceptance in master. That would also give credits while acknowledging @ernestask will.

mwilck commented 4 years ago

@ederag, thanks for your advice. I guess I'd rather merge #312, #316, and this one separately, and give @ernestask the credit where it belongs (on #312). Doing that would have the advantage to separate the adaptations for GNOME 3.32, 3.34, and 3.36 cleanly. To do that, I'd need to rebase all 3 PRs. #312 needs to be rebased on #310, and the other two on top of the previous one, respectively.

The #323 top post could be edited, saying that this is "very heavily based on @ernestask's work (#308)". Then the merge commit message could contain the same information.

AFAIK, by default, the merge commit message would be the head line of the PR only. I believe I can edit the merge commit message and add credits or other contents there. Like @benjaoming added the full message of the commit in his last merge. Or am I getting something wrong?

mwilck commented 4 years ago

Confirmed that this code works also under GNOME 3.34. Test under 3.32 would be appreciated; I can't do this easily.

mwilck commented 4 years ago

Rebased on top of #316 code base. Simplified commit history: Squashed the two GNOME-shell-support commits, and the two commits for the UUID section in README.rst.

mwilck commented 4 years ago

The only open question is the 3.32 support. For safety reasons, I've removed it from the list of supported versions for now. Before merging this, it would be highly desirable to get feedback if this code works on 3.32.

mordax7 commented 4 years ago

Any news on this PR?

hedayat commented 4 years ago

Hi all! Is the blacklist issue solved? I just installed this branch under gnome-shell 3.36.1 without a custom UUID, and it seems to work fine.

mwilck commented 4 years ago

Hi all! Is the blacklist issue solved? I just installed this branch under gnome-shell 3.36.1 without a custom UUID, and it seems to work fine.

The "solution" was removing the "version" field. An extension without version is treated differently by the the shell.

hedayat commented 4 years ago

Oh great. So the branch seems to work fine :)

amboar commented 4 years ago

I gave this pull-req a spin today having upgraded to gnome 3.36 (via Ubuntu 20.04) - thanks for the patches @mwilck. An issue I've found with the PR is that backspace can't completely erase the "What are you doing?" description. Is this related to the changes here? Prior to upgrading to 3.36 I was running gnome 3.34 and the tip of the develop branch. I don't recall having the issue in that configuration.

mwilck commented 4 years ago

An issue I've found with the PR is that backspace can't completely erase the "What are you doing?" description

@amboar, thanks for spotting that. I can confirm that it happens here, too.

mwilck commented 4 years ago

The backspace issue is some other, minor GNOME 3.36 glitch apparently. The code from this PR supports GNOME 3.34, too, and there the backspace key works as usual.

mwilck commented 4 years ago

See GNOME shell d3d1652.

mwilck commented 4 years ago

@amboar, with d217d6e the backspace issue should be fixed. Please confirm.

amboar commented 4 years ago

@mwilck I'm running with d217d6e and the issue appears to be fixed. Thanks!

mwilck commented 4 years ago

@benjaoming, several users besides myself have indicated that they're using this successfully already. if you approve (or indicate you don't care) we could get this finished.

benjaoming commented 4 years ago

Thanks for all the work @mwilck ! I won't be able to review or test this.

benjaoming commented 4 years ago

we could get this finished.

Meaning to merge the PR, right? :)