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

contribShareMenu proposed menus #176316

Open
joyceerhl opened this issue Mar 6, 2023 · 17 comments
Open

contribShareMenu proposed menus #176316

joyceerhl opened this issue Mar 6, 2023 · 17 comments
Assignees
Labels
api api-proposal feature-request Request for new features or functionality on-testplan
Milestone

Comments

@joyceerhl
Copy link
Contributor

joyceerhl commented Mar 6, 2023

I'm filing this issue to track the finalization of the contribShareMenu API proposal since I was not able to find a tracking issue, cc @alexr00.

Current state

The contribShareMenu API proposal tracks the following menu points:

  • file/share
  • editor/context/share

This proposal has been in place for several months without changes, and was previously discussed in #146309 and #157722, but has not yet been finalized. This means it can't yet be used by most extensions, limiting its utility. An example of an extension that IMO would be a great fit for the Share submenus is Live Share (cc @jramsay):

image

Additional share submenus

As part of #175676 I'm looking at integrating link sharing more deeply into our product surface area, with the goal of making collaboration via links as seamless as possible. Therefore I'm proposing to expand this API proposal to also include

  • editor/title/context/share
  • explorer/context/share

(Note, there is also editor/lineNumber/context via #175945, but I have not added a share submenu for that context menu yet because we are introducing that context menu for the first time all up and it would be odd to only have one item in the menu which is itself a submenu.)

@joyceerhl
Copy link
Contributor Author

joyceerhl commented Mar 7, 2023

Feedback from API sync:

  • Could be some opportunity for consolidation across all share menus (and then just inline share actions into all menus)
    • At the same point in time, don't want to show all the same share actions everywhere, and the context object is different
    • e.g. file/share includes export/import profiles which wouldn't be relevant in editor/context/share
  • Unsure whether to include active editor selection if coming via editor/title/context/share or explorer/context/share, waiting for more feedback

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 14, 2023

@joyceerhl One thing I just thought of: how do we support sharing documents from other views, such as the open editors view, or potentially even sharing from custom tree views, such as the Find All References view?

One option would be to create contribution points for each case. That would let extensions control where their menus show up, but would also require knowing about and adopting a lot of contribution points

Another option though would be to have a generic shareFile contribution point. This would let VS Code show these contributions in any places where we think a share menu makes sense. It would also let VS Code surface the share commands in other ways, such as a Share... command in the command palette

I believe we may have considered something similar last week. Were there any concerns about this approach?

@joyceerhl
Copy link
Contributor Author

joyceerhl commented Mar 14, 2023

The main issue that we discussed last week is that we don't want to show the same share actions everywhere, e.g. file/share has Export/Import Profile... which doesn't make sense in any of the other share submenus.

how do we support sharing documents from other views, such as the open editors view, or potentially even sharing from custom tree views, such as the Find All References view?

This is a really good point though. Having a single contribution point would allow us to avoid a continuous adoption cost across extensions anytime we introduce a new area in the product where sharing makes sense.

It would also let VS Code surface the share commands in other ways, such as a Share... command in the command palette

@bamurtaugh had a proposal to add a Share button next to the command center if that ever becomes default, and a single share contribution point would also support that scenario.

If we go with this approach, here is the work that I think should happen:

  • Introduce a shareResource contribution point (I think file might be a little too limiting, e.g. I have wanted to copy links to a specific folder or workspace as well). The contribution point would be responsible for creating share submenus in the following places and ensuring that the right context (both command args and when clauses via overlays) is passed to contributed actions:
    • editor/context
    • editor/title/context
    • editor/lineNumber/context
    • explorer/context
    • file/share
    • editor line number context in diff editors
    • open editors view
    • find all references view
    • SCM view resource state context
  • Keep file/share as a separate menu identifier for now, since at the moment that is the only menu where we want to show actions that wouldn't make sense in all the other cases that we are discussing)
  • Standardize what context gets passed to actions, should be one of:
    1. { uri: vscode.Uri }: indicates generating a link for just the resource but not including range/selection
    2. { uri: vscode.Uri, selections: vscode.Selection }: indicates generating a link for the resource and a specific selection
      -if the start line and end line are the same or if the start and end line are off by 1 and the end column is 0, extensions should generate links for just one line
      • @rebornix we had a related conversation around supporting links for notebooks, I think that maybe we want to support passing NotebookCell ranges as well?
    3. undefined: command palette context
  • Adopt the share contribution in:
    • GitHub Repositories
    • GitHub Pull Requests
    • GitHub
  • Remove previous duplicated link code

@mjbvz let me know if I missed anything or if you have other suggestions.

@joyceerhl
Copy link
Contributor Author

joyceerhl commented Mar 16, 2023

Two more places that would be nice to have share menus in:

  • references view
  • search view

It should be possible to deeplink to a line in a file from the share actions for those views.

@joyceerhl
Copy link
Contributor Author

joyceerhl commented Mar 16, 2023

@jrieken and I discussed this and there are a few complications:

  • we need to perform the appropriate renderer <-> ext host type conversions for the arguments that we pass, e.g. an internal ISelection object needs to be properly transformed into a vscode.Selection object if it's being passed to the ext host
  • some of the views where we want to introduce share links in the context menu are actually controlled by extensions rather than core, in which case the context that gets passed is always the data model object of the corresponding extension and is not influenced by core:
    • references view
    • call hierarchy
    • scm view
    • timeline
  • the command/arg approach suffers from the problem that the extension never really knows what args it receives and there is no contract or explicit typing

In order to be able to provide a consistent context from both core and extension-controlled contexts--without layer breaking--to extensions which want to generate a shareable link, we may be better off with an explicit data provider api. Here is a sketch of how I think this would work:

  • extensions like builtin GitHub, GitHub Pull Requests, Remote Repositories, and Live Share would declare a share contribution in package.json
  • core would inline the contributed command into various menus where it makes sense
  • core and extensions would register SharableDataProviders for various sharable contexts
  • when an extension's share contribution is selected, core would first call a registered ShareableDataProvider capable of providing info about the sharable context
  • the context then gets passed to the extension to generate a link, start a collaboration session, etc

Currently we're most interested in sharing editor and view context, so here's a sketch of what that api could look like:

declare module 'vscode' {

	export namespace window {
		/**
		 *
		 * @param viewId The id of the view that the provider can share data about.
		 * @param provider A provider that can generate {@link ShareableData} for the specified view.
		 */
		export function registerShareableTreeDataProvider(viewId: string, provider: ShareableTreeDataProvider): Disposable;

		/**
		 *
		 * @param document A selector for editors that the provider can share data about.
		 * @param provider A provider that can generate {@link ShareableData} for the specified document.
		 */
		export function registerShareableDocumentDataProvider(document: DocumentSelector, provider: ShareableDocumentDataProvider): Disposable;
	}

	interface ShareableTreeDataProvider {
		provideShareableData(context: TreeItem, token: CancellationToken): ProviderResult<ShareableData>;
	}

	interface ShareableDocumentDataProvider {
		provideShareableData(context: DocumentSelector, token: CancellationToken): ProviderResult<ShareableData>;
	}

	interface ShareableData {
		readonly uri?: Uri;
		readonly ranges?: Range[];
	}
}

Another option is to expand the existing TreeDataProvider to include a provideShareableData method:

	/**
	 * A data provider that provides tree data
	 */
	export interface TreeDataProvider<T> {
		
		provideShareableData?(item: TreeItem, token: CancellationToken): ProviderResult<ShareableData>;
	}

@joyceerhl
Copy link
Contributor Author

Some additional limitations of the current implementation, which a central API would be able to address:

  • Each extension is responsible for writing share links to the clipboard, so they step on each other which prevents nice multiselect behavior
  • We have had to introduce multiple share submenus to each context menu that we want to surface Share actions in (to date: explorer/context, editor/context, editor/title/context, editor/lineNumber/context, file). We also want to add Share to more places, and since some of these places are extension-owned tree views and all places have different data object shapes, each extension which contributes a sharing link must deal with parsing different source data types, so the current approach doesn't scale:
    • References view
    • Search view
    • SCM view
    • Open editors view
  • We cannot support a top-level Share action since the actions are surfaced piecemeal by extensions to specific Share submenus

Basically what we need is API to support the ability to

  1. Get a standard context object from extension-contributed tree views, whose tree item shapes are opaque to core (all other data objects can be handled by core)
  2. Pass that context object to extensions which provide share actions

I spent some time looking at prior art (copy/paste and drag/drop controllers, which support performing actions relating to data transfer on both tree items and editor context). I learned that we use stringified URIs with line numbers in the fragment, which works well enough for text editors. Unfortunately we already support copying deeplinks to notebook cells, so this approach doesn't suffice for my scenario:

vscode/src/vscode-dts/vscode.d.ts

Lines 10567 to 10569 in 06fc826

* To add a data transfer item that can be dragged into the editor, use the application specific mime type "text/uri-list".
* The data for "text/uri-list" should be a string with `toString()`ed Uris separated by newlines. To specify a cursor position in the file,
* set the Uri's fragment to `L3,5`, where 3 is the line number and 5 is the column number.

Here's my latest API proposal sketch:

/**
 * 
 * An {@link TreeShareableResourceDataProvider} enables sharing tree items
 * which represent resources from extension-contributed tree views.
 */
export interface TreeShareableResourceDataProvider<T> {
    /**
     * 
     * @param source A tree item to provide shareable resource data for
     * @returns The underlying shareable resource data represented by the tree item
     */
    provideShareableResourceData(source: T, token: CancellationToken): ProviderResult<ShareableResourceData>;
}

export interface TreeViewOptions<T> {
    /**
     * 
     * {@link TreeView}s can optionally specify a {@link TreeShareableResourceDataProvider}
     * to ensure that `Share` actions show up in the context menu for the {@link TreeView}. 
     */
    shareableResourceDataProvider?: TreeShareableResourceDataProvider<T>;
}

/**
 * 
 * To be implemented by:
 * 1. GHPRI
 * 2. RemoteHub
 * 3. Builtin GitHub extension
 * 4. MakeCode
 * 5. Live Share
 */
export interface ShareProvider {
    readonly id: string;
    readonly label: string;

    /**
     * @param resourceData Data representing a shareable resource
     * @returns If applicable, a URI representing the shared resource, to be written to the clipboard
     */
    provideShareResource(resourceData: ShareableResourceData, token: CancellationToken): ProviderResult<Uri>;
}

/**
 * 
 * Modeled after TabInputText__, TabInputNotebook__, etc.
 */
export type ShareableResourceData = TextDocumentShareableData | NotebookDocumentShareableData;

export interface TextDocumentShareableData {
    uri: Uri;
    selection: Range;
}

export interface NotebookDocumentShareableData {
    uri: Uri;
    selection: NotebookRange;
}

@joyceerhl
Copy link
Contributor Author

Feedback from API sync:

  • Instead of introducing a TreeView provider for extracting shareable data, expand the TreeItem interface to include a new, optional resource + location property
    • There is already resourceUri but this isn't always indicative of a shareable resource, e.g. GHPRI sets dummy URIs in order to be able to provide file decorations
    • We could have shareableResource with shape { uri: Uri, location: Range | NotebookRange }
  • Wherever we can find a URI + location, we treat that as shareable and inline the Share menu
    • We won't yet support selectively inlining actions in some menus but not others
  • Share actions are async, and VS Code/Electron do not currently support asynchronously updating context menus: Support for updatable/async menus and submenus #149323 We have gotten around this in the past with custom widgets, e.g. for Refactor. May need to consider a hybrid provider + static contribution solution in this scenario

@eamodio
Copy link
Contributor

eamodio commented May 16, 2023

I'm not sure I really understand the goal of a unified API/experience here.

Each extension is responsible for writing share links to the clipboard, so they step on each other which prevents nice multiselect behavior

I definitely see this as a challenge, but it's also solvable with multi-select aware commands. I'm not sure I understand why "share" should be treated specially.

This seems to assume that an individual item/line/editor/etc has 1 shareable thing, but each thing could have many shareable aspects -- how would that be handled in a unified api? { uri: Uri, location: Range | NotebookRange } isn't enough to differentiate. For example, I might want to share a GitHub link, or a vscode.dev link, or a GitLens link, or, or... And each of these would need to be provided by a particular extension to resolve the behavior.

Sorry if I'm just completely missing something on this.

@joyceerhl
Copy link
Contributor Author

joyceerhl commented May 16, 2023

And each of these would need to be provided by a particular extension to resolve the behavior.

Maybe I am misunderstanding your question--the proposal is that each extension command which is currently implemented as a menu contribution would instead have a corresponding share provider, and we expect multiple providers to exist simultaneously. So for example there would be a GitHub permalink provider, a vscode.dev link provider, a GitLens remote file link provider and so on. A similar API exists for the rich Paste As... action, where there are multiple providers for pasting data from the clipboard as text, Markdown links, images, and so on.

The benefits of this approach are:

  • your extension's provided share actions will be surfaced for any element in the VS Code workbench that is deemed 'shareable', including extension tree views like the reference view or GHPRI view once they adopt the proposed shareableResource property, rather than needing to contribute to a proliferation of share submenus
  • your extension's provider always receives a predictable shape (a resource and optionally a location), rather than having to accept/marshall arguments of unknown shape
  • we'd be able to move towards offering a richer top-level Share action mediated by an overlay widget, similar to Share experiences in other applications like Office (Word, Powerpoint) and the share tray in iOS apps

Are you perhaps saying that we might need more information about a shareable location than just the Uri and the Range?

...and, TIL that in Electron, the Share menu is already special-cased for macOS, and a special SharingItem gets set for the Share menu:

sharingItem SharingItem (optional) macOS - The item to share when the role is shareMenu.

@eamodio
Copy link
Contributor

eamodio commented May 17, 2023

I still don't understand how this would work. For example, in GitLens you can right-click on a branch and can copy the following URLs:

  • Branch on GitHub
  • Commit the Branch is at on GitHub
  • Repository the Branch belongs to on GitHub

How would the user choose which one they want? In this case the "resource" is the same.

@joyceerhl
Copy link
Contributor Author

@eamodio GitLens would register 3 link providers, one for each link type.

@eamodio
Copy link
Contributor

eamodio commented May 17, 2023

I see -- so the individual share provider is the "named" entry in the "Share" menu (I missed the label prop on that).

Still seems like a lot of overhead vs a command, and then doesn't provide the granular control based on context. Currently, we can control based on the particular context what actions are most relevant. With this VS Code would have to show all options in all contexts.

@joyceerhl
Copy link
Contributor Author

we can control based on the particular context what actions are most relevant

Do you have examples of share actions in GitLens that are suppressed based on context?

@eamodio
Copy link
Contributor

eamodio commented May 17, 2023

Today each command placement in GitLens is controllable via settings -- so if someone wanted to turn off a set of menus from showing up in the Share menu of the line/gutter, but not in the editor context menu they can. That wouldn't be possible with this.

Also, context for the same "resource" feels important, what you might expect on the Share menu on an editor tab vs editor context vs SCM, etc could be different, if not in the raw options themselves, but ordering and grouping.

@eamodio
Copy link
Contributor

eamodio commented May 17, 2023

Why is "Share" special? We don't have this for "Copy" or for other actions. And it feels like a unified system is harder and less flexible than a targetable menu (like Copy As).

I get the challenge of multi-select (though is multiselect sharing common?), but a top-level action is solvable by another targetable menu. It will be harder to handle because of long standing challenges with command callbacks (#25716), but also solvable.

@alexr00
Copy link
Member

alexr00 commented Jun 27, 2023

Moving to July.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

No branches or pull requests

5 participants