Gnome 3.32 support - alternative pull request

[mwilck]

This PR aims to fix #307. It is almost equivalent to #308, and very heavily based on @ernestask's work there.

Differences are:

• The branch is based on @hedayat's work for 3.30. As such, the first 5 commits up to 3e75499 are actually aimed at 3.30, and are partially reverted for 3.32 later in the series.
• It's split into several commits for easier review
• It does not introduce "Arrow" style function syntax (yet, this could be easily added later).
• It adds a few comments for jshint to avoid problems in make test-style
• It marks only GNOME 3.32 as supported in this version (IMO necessary; other extensions do the same).

If we want to support older GNOME versions in the future, we'll need to fork.

Note that the only parts that are necessary to make hamster-shell-extension work with GNOME 3.32 are 78bc2ff (plus the fix 76e961c) and 0621854.

[mwilck]

Removed a broken URL in the docs to make the CI pass.

[ernestask]

I would rather not be associated with this, thank you. :)

[StykMartin]

@ernestask Why? He makes it more clear

[ernestask]

If Martin here wants to get more internet points so badly that the usual review process seems too slow, so be it. I’m past caring by now.

[mwilck]

@ernestask, I assure you I was not after "internet points". I thought I'd given you the credits you deserve. I'm sorry if you feel otherwise. I certainly did not want to annoy you, or steal any "points" from anyone.

The reason I created this PR is this: I was looking at #308, and found if very hard to review because of the big amount of changes in a single patch. I could have reviewed #308, asking you to split your patch yourself. But you had already indicated that you were not interested in investing more time into the issue. Therefore I did it myself. Maybe I should have submitted a PR to your branch rather than to the main repo, but I couldn't figure out how to do that without reverting your patches first.

In any case, you did the main work, and if anyone is to be credited, it should be you, not me.

[mwilck]

@elbenfreund, for clarification: if you intend to merge changes for 3.32, I have no objections if you merge #308 rather than this PR. As already indicated, the two approaches are technically equiv
alent,and I basically re-did ernestask's changes in smaller steps. See this as a review help if you wish.

[mwilck]

Closing in favor of #316.

[ederag]

@ernestask Indeed, it would have been better to discuss the splitting beforehand,
and to add a Co-Authored-By: line to each commit.
It might be just that @mwilck did not know, but it definitely is not an isolated case:

My PR: https://build.opensuse.org/request/show/653434
The request that made it: https://build.opensuse.org/request/show/672876
Edit: it's about packaging work in this case (as the answer confirms).
v2.1.1 coding work was perfectly cited.

So you are not alone. Hope to see you contributing on the main hamster !

[mwilck]

@ernestask Indeed, it would have been better to discuss the splitting beforehand,
and to add a Co-Authored-By: line to each commit.
It might be just that @mwilck did not know, but it definitely is not an isolated case:

Indeed, I wasn't aware of this practice. But I've clearly given @ernestask credit both in the commit messages and in the head of this PR. I have absolutely no issue with naming @ernestask in the Author: tag (if he wants - my impression was rather that he didn't want to be associated with this PR in any way). It is true that I did not author most of the code in this PR, and I don't think I ever claimed I did. The only reason I created this PR was that I hoped that for the maintainers, it would be easier to review and merge than the original #308.

My PR: https://build.opensuse.org/request/show/653434
The request that made it: https://build.opensuse.org/request/show/672876

Now this is a completely different issue. openSUSE requests are not about authorship, they are simply part of the work flow of integrating updates or bug fixes in the distribution. My name in the changelog does not mean that I claim authorship or credits for the changes in the request. It means that I'm responsible for pushing the changes to openSUSE. In the case you mention, I was trying to speed up things. I could have rejected your request, and left it to you to figure out how to fix it. That would most likely have taken several hops, wasting both your time and mine. Instead I fixed it myself. I can assure you that this happens all the time in the openSUSE submission work flow. This is the first time I've seen a complaint about it.

[mwilck]

@ernestask,

I hope your grudge against myself has diminished over time. Let me reaffirm that my intention has never been to claim credits for your work. #316, which is based on this PR and thus on the work you did, is going to be merged soon, and before that happens, I'd like to ask you whether you'd like to be given credit in the form of Co-Authored-By: or Authored-By: tags in the commit messages, or in some other way. I'd be happy to do so and put an end to our controversy, but I wouldn't do it without your consent.

[mwilck]

@ernestask, gentle reminder. You don't have to respond of course, but please understand that we will merge #316 very soon either way.

[mwilck]

Rebased on top of current develop, changed commit messages of ba2f868 and
20167d3 to mention @ernestask's authorship more prominently, and signed top commit.

[mwilck]

Rebased on current develop branch.

[benjaoming]

To my eyes, this looks good, but I haven't been able to test it / make use of it, as I'm on an older Shell. But it follows the semver release pattern that we had been planning, and I much appreciate the order that changes and deprecation happens in 👍

[mwilck]

@DirkHoffmann, @hedayat, gentle reminder. If you don't care, please indicate that at least.

[DirkHoffmann]

I can make a test over the week-end, because I am still working daily with a 3.32 shell (unless @hedayat wants to do it, but I think he is already on a newer system).

[hedayat]

Sorry to be late. Looks OK.

[hedayat]

I can make a test over the week-end, because I am still working daily with a 3.32 shell (unless @hedayat wants to do it, but I think he is already on a newer system).

Yep, I'm already on 3.36. I usually jump to next Fedora when its Beta comes.

[mwilck]

@DirkHoffmann, have you been able to do the test you talked about?

[DirkHoffmann]

Just a small remark: The Makefile should be completed with some explanations, namely what clean and clean-docs do exactly. I think it is "do all cleans at once" and "clean only doc directory", and I can submit such a PR of course. I am not sure on which branch I should commit it though, because it should go to all branches for the various shell versions.

I did

git fetch origin refs/pull/312/head
git checkout -b mwilck/GNOME-3.32 FETCH_HEAD

and after installation, following the now-up-to-date instructions (Thanks!), I found my shell extension working in production exactly as before. So this PR is approved by me, too.

Finally, I gave gh (as proposed for local checkout/review) a try, installed it and (in the same directory):

[hoffmann hamster-shell-extension]$ gh pr checkout 312
Switched to branch 'GNOME-3.32'
Your branch and 'remotes/mwilck/GNOME-3.32' have diverged,
and have 12 and 19 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)
fatal: Not possible to fast-forward, aborting.
exit status 128

So I am puzzled here, although it has nothing to do with the PR itself, of course.

[benjaoming]

So I am puzzled here, although it has nothing to do with the PR itself, of course.

There was a force-push 8 days ago. I'm not familiar with the tool that you are using, but it sounds like you have a local branch 'GNOME-3.32' that has a history from before that force-push.

It's not to say that force-pushing is bad, but you have to anticipate them sometimes.. if you're not working in the branch anyways, it's harmless to delete it with git branch -d GNOME-3.32

[mwilck]

The force-push had become necessary because of the previous merges (#310, #299, #325), and the requirement for signed commits (which has been relaxed meanwhile, but I tend to think it was a good idea anyway).

[benjaoming]

Definitely @mwilck -- I did mean as reviewers of a PR, we have to anticipate that there is a force-push once in a while 👍

[mwilck]

To avoid another force-push after merging #303, I've merged the develop branch locally and pushed a README change about the GNOME shell version compatibility.

[mwilck]

So, given that all reviewers have approved, I'll move forward here.

[benjaoming]

@mwilck can this be released to extensions.gnome.org after #312 ?

[DirkHoffmann]

@mwilck can this be released to extensions.gnome.org after #312 ?

I think the only person who can do it right now is @elbenfreund.

[DirkHoffmann]

I had not answered this. Maybe for our general culture?

There was a force-push 8 days ago. I'm not familiar with the tool that you are using, but it sounds like you have a local branch 'GNOME-3.32' that has a history from before that force-push.

Actually this is what github suggests (gh). Never heard of it before. I was just curious, if its teething problems of gh or something more fundamental that I was able to understand. Yes, there seem to be two approaches to checkout the GNOME-3.32 branch, which have the same effect, but are interfering if mixed.

It's not to say that force-pushing is bad, but you have to anticipate them sometimes.. if you're not working in the branch anyways, it's harmless to delete it with git branch -d GNOME-3.32

Yes, understood. Thank you. Never mind.