Skip to content

Fix inlay hint location links problem#167886

Merged
jrieken merged 3 commits into
microsoft:mainfrom
hkalbasi:inlaylink
Dec 15, 2022
Merged

Fix inlay hint location links problem#167886
jrieken merged 3 commits into
microsoft:mainfrom
hkalbasi:inlaylink

Conversation

@hkalbasi

@hkalbasi hkalbasi commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

fix #167564

The changes in goToDefinitionAtPosition.ts disable decoration of the nearest token on hover with Ctrl, and the other changes fix the target when clicking. I didn't find out how to add a test for it but I can do that with some mentoring if needed.

@jrieken jrieken added this to the December 2022 milestone Dec 5, 2022

@jrieken jrieken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks so far. Left a suggestion to avoid a dependency between inlay and goto contributions

Comment thread src/vs/editor/contrib/gotoSymbol/browser/link/goToDefinitionAtPosition.ts Outdated
Comment thread src/vs/editor/contrib/gotoSymbol/browser/goToCommands.ts Outdated
Comment thread src/vs/editor/contrib/gotoSymbol/browser/goToCommands.ts Outdated
}

override runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, arg?: SymbolNavigationAnchor | unknown, range?: Range): Promise<void> {
override runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: [ICodeEditor, SymbolNavigationAnchor | unknown, Range | undefined]): Promise<void> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this change is needed and I do believe that it will break existing callers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There was a mismatch between callers and this before my PR, and it was the reason it wasn't working correctly. So either the caller in editorExtensions.ts:455 or here needs to be changed. And the caller is more generic, so I think changing here is correct. And other inherited classes of EditorAction2 all have an args argument as third argument, and no more arguments:

src/vs/editor/contrib/gotoSymbol/browser/goToCommands.ts
106:    override runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: [ICodeEditor, SymbolNavigationAnchor | unknown, Range | undefined]): Promise<void> {

src/vs/editor/contrib/folding/browser/folding.ts
529:    public override runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: T): void | Promise<void> {
src/vs/editor/browser/coreCommands.ts
36:     public runEditorCommand(accessor: ServicesAccessor | null, editor: ICodeEditor, args?: Partial<T> | null): void {
1885:           public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: unknown): void {
1961:           public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: unknown): void {
2095:           public runEditorCommand(accessor: ServicesAccessor | null, editor: ICodeEditor, args: unknown): void | Promise<void> {
2110:           public runEditorCommand(accessor: ServicesAccessor | null, editor: ICodeEditor, args: unknown): void | Promise<void> {

src/vs/editor/browser/editorExtensions.ts
276:                    public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: any): void {
377:    public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: any): void | Promise<void> {

src/vs/editor/contrib/wordOperations/browser/wordOperations.ts
44:     public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: any): void {
333:    public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, args: any): void {

src/vs/editor/contrib/codeAction/browser/codeActionCommands.ts
210:    public runEditorCommand(_accessor: ServicesAccessor, editor: ICodeEditor, userArgs: any) {

src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts
1675:   public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor) {
1705:   public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor) {
1723:   public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor) {
1750:   public runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are right. Tho, I would fix this at the few special callers that we have. Usually these commands are invoked via the "normal command execution" but some are special: namely inlay hint and definition links. This and this invocation should be changed so that the editor isn't passed anymore

@jrieken jrieken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I pushed some changes as described and I am going ahead to merge this PR. Thanks!

@jrieken jrieken enabled auto-merge December 15, 2022 08:17
@jrieken jrieken merged commit f49ecc8 into microsoft:main Dec 15, 2022
@hkalbasi

Copy link
Copy Markdown
Contributor Author

@jrieken the problem appears again after your changes.

@github-actions github-actions Bot locked and limited conversation to collaborators Jan 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.

Inlay hint location links goto definition goes to the nearest symbol

3 participants