Correctly set check mark after accepting a new permission

[aspiers]

When the user requests the extension to be given access to the site and confirms this in the consequent browser dialog, the context menu item wasn't being immediately changed to checked.

We can fix this by ensuring that the updateItem() function which is responsible for updating the checkmark is called with the correct URL for the tab in question, so that it can detect whether the permission is now granted to the site and update the context menu item accordingly.

Note that the browser will automatically enable the checkmark simply by virtue of the context menu item having been clicked. So if the user denies access in the dialog (despite previously requesting it to be given via a user-initiated "gesture", which is the only way the chrome.permissions API allows access to be given), then chrome.contextMenus.update() needs to be called to reverse that enabling of the checkmark. However by calling updateItem() within the finally block with the correct tab URL, we can ensure the checkmark is always correctly set regardless of what happened.

Fixes #29.

[aspiers]

Oops, fixing CI now.

[fregante]

By the way no need to force push, I'll squash the commits anyway

[aspiers]

By the way no need to force push, I'll squash the commits anyway

It's no extra effort for me; in fact this is the way I normally work. But it's your project, so if the force-pushes cause problems for you, I'll happily stop doing that :-)

[fregante]

if the force-pushes cause problems for you

It's mostly because it becomes more difficult to see what changed. I know squashing is common in other repos so I generally say it to save contributors from unnecessary work

[aspiers]

if the force-pushes cause problems for you

It's mostly because it becomes more difficult to see what changed.

Yes, GitHub is bad in this regard. In fact I documented this in detail a long time ago:

• https://github.com/isaacs/github/issues/997
• https://github.com/isaacs/github/issues/998
• https://github.com/isaacs/github/issues/999

However, it got a bit better over the years. For example, you can click on either of these links to see the difference:

(Although in this PR, you will see a force-push which didn't change the code at all. That doesn't mean this feature is broken; it's just because I realised I had created the commit with the wrong author and committer emails, so I fixed the commit metadata and force-pushed to prevent invalid email addresses from polluting main forever.)

I know squashing is common in other repos so I generally say it to save contributors from unnecessary work

True, I have noticed that a lot of developers really struggle with cleaning git history. I used to contribute to git, so this kind of stuff has been second nature to me for a long time.

BTW in case you haven't seen it, Gerrit does code review WAY better. After years of working with both, GitHub code review still feels like bashing my head against the wall every time. And for Open Source repositories, it's possible to have the best of both worlds for free via https://gerrithub.io/ (although keeping GitHub Actions is a bit more effort). But I digress :laughing:

[fregante]

you can click on either of these links to see the difference

I know, but the difference is that with regular commits you don't have to click. In fact GitHub even has a nice commit dropdown in both the Files tab and the Commits tab. 😃

Also re-pushing the same commit with an issue reference causes noise in the referenced issue: https://github.com/fregante/webext-domain-permission-toggle/issues/29#ref-issue-2092059333

[fregante]

Closing this for now, I'm improving the overall logic in a new PR.

the benefit of separation of concerns

I agree with you here. Fixing it in a new PR

All we need to do is ensure that updateItem() is called at the appropriate points.

Yes. I also opened this:

• https://github.com/fregante/webext-domain-permission-toggle/issues/36

[aspiers]

I know, but the difference is that with regular commits you don't have to click. In fact GitHub even has a nice commit dropdown in both the Files tab and the Commits tab. 😃

Sure. The problem with that is that it's mixing "private" / unclean history (which isn't intended for inclusion in main) with "public" / clean history (which is). This is a key failing of GitHub's code review system, and again something which Gerrit gets perfectly right by cleanly separating the two and making it easy to review whichever diffs you need to see, both history and "meta-history".

So GitHub forces us to pick one of two suboptimal approaches. You prefer one, I prefer the other, it's all good :-)

Also re-pushing the same commit with an issue reference causes noise in the referenced issue

Yes that's an annoying downside. It could be worked around by only referencing the issue in the PR and not in the commits. Also technically speaking, fixups commits if written correctly should also reference the issue they are relating to, so that the reviewer knows which issue they are fixing up. For small PRs and small repos like here, this is less relevant because typically the PR only targets a single issue. I normally work in much larger repos with larger teams, where writing comprehensive messages for every single commit and polishing history prior to merge are much more important, so that's where my preference comes from.

Anyway, back to the main topic ;-)