Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce share links in more places via a contrib #177311

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

joyceerhl
Copy link
Collaborator

@joyceerhl joyceerhl commented Mar 16, 2023

Re: #176316, #175676

I received feedback in #176316 (comment) that introducing multiple share menus, each with their own expected behavior (copying a range, copying a path without a range, and copying neither a path nor a range) is onerous for extensions.

This PR reduces the burden on extensions by passing a more predictable context object to extension-contributed share commands and allowing extensions to register commands in all relevant share submenus with one contrib declaration in package.json. In brief:

  • no args: use active editor (this is the behavior for command palette)
  • uri in args: link to file or folder without range (this is the behavior for SCM, editor title context, explorer context, open editors context)
  • uri and range in args: link to file with range (this is the behavior for editor context share and editor line number context)

It's adopted here for the builtin GitHub extension but also requires adoption in GHPRI and GitHub Repositories.

@joyceerhl joyceerhl enabled auto-merge (squash) March 16, 2023 02:04
@joyceerhl joyceerhl self-assigned this Mar 16, 2023
@vscodenpa vscodenpa added this to the March 2023 milestone Mar 16, 2023
@joyceerhl joyceerhl force-pushed the dev/joyceerhl/surviving-swordfish branch from 601569f to 72c7353 Compare March 16, 2023 02:06
@joyceerhl joyceerhl requested a review from alexr00 March 16, 2023 02:06
@joyceerhl joyceerhl requested a review from jrieken March 16, 2023 06:00
@joyceerhl joyceerhl merged commit e9ff97a into main Mar 16, 2023
@joyceerhl joyceerhl deleted the dev/joyceerhl/surviving-swordfish branch March 16, 2023 08:15
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has a layer breaker and I would also argue that it has a conceptual flaw. You might be able to fix the layer breaking (tho not easy) but generally commands should never just rely on the arguments they are being called with. Any extension can call any command at any time and therefore it is best that commands "discover" their arguments themselves.

character: selection.getEndPosition().column
}
}))]
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not OK because it is a conceptual layer breaker. It introduces extension host types to the renderer, e.g this new argument is wrong for all "internal" context menu actions and wrong for monaco-editor use-cases.

if (this._options?.arg) {
if (this._options?.arg && Array.isArray(this._options.arg)) {
runArgs = [...runArgs, ...this._options.arg];
} else if (this._options?.arg) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have unfortunately limited capacities for a deeper look but this seems breaking for those cases where use have been using an array as argument, right?

jrieken added a commit that referenced this pull request Mar 16, 2023
aeschli pushed a commit that referenced this pull request Mar 16, 2023
Revert "Introduce share links in more places via a contrib (#177311)"

This reverts commit e9ff97a.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants