search

projecthamster / hamster-shell-extension

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

Gnome 3.32 support - alternative pull request #312

Closed mwilck closed 4 years ago

mwilck commented 5 years ago

This PR aims to fix https://github.com/projecthamster/hamster-shell-extension/issues/307. It is almost equivalent to https://github.com/projecthamster/hamster-shell-extension/pull/308, and very heavily based on @ernestask's work there.

Differences are:

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 commented 5 years ago

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

ernestask commented 5 years ago

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

StykMartin commented 5 years ago

@ernestask Why? He makes it more clear

ernestask commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 4 years ago

Closing in favor of #316.

ederag commented 4 years ago

@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 commented 4 years ago

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

ederag commented 4 years ago

Someone privately said that my post "looks really unnecessary". It was triggered by a discussion involving access rights, and mainly motivated to regain @ernestask confidence that this was not considered normal, at least in the current main repo: https://github.com/projecthamster/hamster

Nothing personal at all. At first sight it might seem otherwise, sincerely sorry about that.

@mwilck Here is my point of view: Given the time it took to create my PR, I would gladly have taken time to discuss and to modify it. Or you might just have pulled mine and pushed your alterations right after, as you see fit. But your explanation is plausible. It's OK.

mwilck commented 4 years ago

@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 commented 4 years ago

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

mwilck commented 4 years ago

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 commented 4 years ago

Rebased on current develop branch.

mwilck commented 4 years ago

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

DirkHoffmann commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

benjaoming commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mwilck commented 4 years ago

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 commented 4 years ago

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

benjaoming commented 4 years ago

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

DirkHoffmann commented 4 years ago

@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 commented 4 years ago

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.