Skip to content

Added extensibility points for searchView/context#109049

Merged
roblourens merged 8 commits intomicrosoft:masterfrom
digeff:searchView/context/extensionPoints
Nov 10, 2020
Merged

Added extensibility points for searchView/context#109049
roblourens merged 8 commits intomicrosoft:masterfrom
digeff:searchView/context/extensionPoints

Conversation

@digeff
Copy link

@digeff digeff commented Oct 21, 2020

I tested this by writing a mini extension that contributes a menu for the search results:

	"contributes": {
		"commands": [
			{
				"command": "set-breakpoints-on-search-results.helloWorld",
				"title": "Hello World"
			}
		],
		"menus": {
			"search/context": [
				{
					"command": "set-breakpoints-on-search-results.helloWorld"
				}
			]
		}
	},

I got the menu to appear, and I got the expected parameters when I clicked on it:
image

I tested the IMatchContext and IFileMatchContext cases. I haven't been able to figure out how to make the editor call me with a IFolderMatchContext yet.

This PR fixes part of #107996

cc @roblourens

@digeff digeff force-pushed the searchView/context/extensionPoints branch from 69454f4 to eae3fcc Compare October 21, 2020 00:39
roblourens
roblourens requested changes Oct 21, 2020
View reviewed changes
@roblourens
Copy link
Member

roblourens commented Oct 21, 2020

Also remember to fix the existing context menu actions that rely on the context.

@roblourens roblourens mentioned this pull request Oct 21, 2020
Open
@digeff digeff force-pushed the searchView/context/extensionPoints branch from e1d5472 to bce86a1 Compare November 2, 2020 18:58
roblourens
roblourens requested changes Nov 10, 2020
View reviewed changes
roblourens
roblourens approved these changes Nov 10, 2020
View reviewed changes
Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Thanks!

@roblourens roblourens merged commit a1bb5ac into microsoft:master Nov 10, 2020
@digeff digeff deleted the searchView/context/extensionPoints branch November 10, 2020 22:55
alexdima added a commit that referenced this pull request Nov 11, 2020
@alexdima
Revert "Merge pull request #109049 from digeff/searchView/context/ext…
14140d2 
…ensionPoints"

This reverts commit a1bb5ac, reversing
changes made to 9a08a10.
@alexdima
Copy link
Member

alexdima commented Nov 11, 2020

@roblourens I'm sorry, I had to revert this PR via 14140d2 because it was causing browser integration tests to fail in our nightly build

jrieken
jrieken reviewed Nov 11, 2020
View reviewed changes
src/vs/workbench/contrib/search/common/searchModel.ts
}

toJSON(): object {
return {}; // We send an IRenderableMatchContext to the extensions
Copy link
Member

@jrieken jrieken Nov 11, 2020

Choose a reason for hiding this comment

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

Such changes aren't good and data that's transfers between processes should have explicit DTOs (data transfer objects)

jrieken
jrieken reviewed Nov 11, 2020
View reviewed changes
src/vs/workbench/api/common/menusExtensionPoint.ts
key: 'search/context',
id: MenuId.SearchContext,
description: localize('menus.searchContext', "The search context menu")
},
Copy link
Member

@jrieken jrieken Nov 11, 2020

Choose a reason for hiding this comment

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

@roblourens This smell like "under the radar" API work. Esp. interesting is what arguments those commands are invoked with. We have no API representation for search results and shouldn't introduce one with these changes unless properly reviewed and designed

Copy link
Member

@roblourens roblourens Nov 11, 2020

Choose a reason for hiding this comment

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

I thought the whole space of arguments for commands is sort of under the radar API. I looked at some other context menu contexts and they are invoked with different arbitrary things.

Copy link
Member

@jrieken jrieken Nov 11, 2020

Choose a reason for hiding this comment

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

This is API since changing arguments breaks extensions. I would be surprised if context menus use arbitrary objects as arguments, the rule should be use URI whenever the source itself has no representation in the API, eg. the explorer uses URI, so does the editor title and editor context, but SCM uses SCM things, custom tree use TreeItem etc.

For search results a URI isn't enough and search API (which I assume would talk about search matches) has never been tackled for finalization

Copy link
Member

@roblourens roblourens Nov 11, 2020

Choose a reason for hiding this comment

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

I was looking at variables and callstacks, which as far as I can tell are arbitrary objects with no api representation @isidorn.

Copy link
Collaborator

@isidorn isidorn Nov 11, 2020

Choose a reason for hiding this comment

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

@roblourens correct, they have no official vscode api representation.
Here's a expert from the documentation for more details:

"When a registered context menu command is executed, both the underlying variable and its container are passed as Debug Adapter Protocol (DAP) objects. Please note that VS Code's extension API uses opaque stand-in types instead of the real DAP types. In order to access their properties, they can be easily coerced into the corresponding DAP types."

Copy link
Member

@roblourens roblourens Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

On the callstack, I saw this
image which I think isn't associated with any single DAP type, but I guess it's all coming directly from stuff in the DAP.

Let's reopen the issue for discussion. I'm looking at the proposed API that we have for search results and trying to decide whether it makes sense to use the same objects here. You could make a case either way. If we can make a case to design this api without necessarily tying it to search provider API, then we can do this without making it part of the whole search provider api problem.

One difference between the UI results and the provider results is that providers return one Match per line, possibly with multiple ranges on the line. The UI object always has one object per actual match. Or you would want some additional info like the replacement value.

Copy link
Collaborator

@isidorn isidorn Nov 12, 2020

Choose a reason for hiding this comment

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

Yeah unlike the variables view the callstack is in a bad shape and I have this issue to tackle it #85696
We can continue the discussion there

@roblourens roblourens added this to the November 2020 milestone Nov 11, 2020
@digeff digeff restored the searchView/context/extensionPoints branch November 11, 2020 17:49
@roblourens roblourens mentioned this pull request Nov 12, 2020
Open
@digeff digeff mentioned this pull request Nov 16, 2020
Closed
meganrogge pushed a commit that referenced this pull request Nov 18, 2020
@alexdima @meganrogge
Revert "Merge pull request #109049 from digeff/searchView/context/ext…
8dce03b 
…ensionPoints"

This reverts commit a1bb5ac, reversing
changes made to 9a08a10.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@roblourens roblourens roblourens approved these changes

@jrieken jrieken jrieken left review comments

@isidorn isidorn isidorn left review comments

Assignees

@roblourens roblourens

Labels

None yet

Projects

None yet

Milestone

November 2020

Development

Successfully merging this pull request may close these issues.

6 participants

@digeff @roblourens @alexdima @jrieken @isidorn @JacksonKearl