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

Allow Code Lenses to only Provide a Title and no Backing Command #24469

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Apr 10, 2017

Fixes #24209

Bug
Currently, for the js/ts references code lens, even if there are zero references you can click on the lens. This display an empty peek view. The root cause is that we always have to provide a command for these lenses, even if no action can really be taken

Fix
Allow code lenses to only register a title for the lens with no actual backing command. This lets a code lens display info to the user while not potentially incorrectly suggesting that more information is available by clicking

apr-10-2017 15-03-27

@mention-bot
Copy link

@mjbvz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jrieken and @egamma to be potential reviewers.

@mjbvz mjbvz changed the title Allow Code Lenses to only Provide only Allow Code Lenses to only Provide a Title and no Backing Command Apr 10, 2017
@mjbvz mjbvz self-assigned this Apr 10, 2017
@@ -1617,7 +1617,7 @@ declare module 'vscode' {
/**
* The command this code lens represents.
*/
command?: Command;
command?: Command | string;
Copy link
Collaborator Author

@mjbvz mjbvz Apr 10, 2017

Choose a reason for hiding this comment

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

@jrieken I'm not a huge fan of this API design. Let me know if you have any suggestions on a better way to express this

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.

A command is not a string, the Command | string signature is a no-go

@jrieken
Copy link
Member

jrieken commented Apr 11, 2017

Instead of these changes, a simple fix for #24209 could be to use a no-op command that does nothing

@jrieken
Copy link
Member

jrieken commented Apr 11, 2017

The code-lens code is ready for this which makes this somewhat a problem with strict null and our command signature

if (command.id) {
	part = format('<a id={0}>{1}</a>', i, title);
	this._commands[i] = command;
} else {
	part = format('<span>{0}</span>', title);
}

@mjbvz
Copy link
Collaborator Author

mjbvz commented Apr 11, 2017

@jrieken The problem with using a noop command is that it still renders the lens as a clickable element.

I was also hesitant to update the Command interface itself to make the command element nullable since that interface is used in so many other places. Perhaps we could instead take an empty string to mean that there is no command in this case? Other options:

  • Change the signature to sometime like: command?: Command | {title: string}
  • Add a new title field on the CodeLens

None of these seem great

@jrieken
Copy link
Member

jrieken commented Apr 12, 2017

I'll check but #24469 (comment) tells me that we have been using undefined as the actual command already before. This became only visible with the strictNull-move. The empty string tricks sounds also like a valid workaround but in general receivers of Command must always validate it

Fixes microsoft#24209

**Bug**
Currently, for the js/ts references code lens, even if there are zero references you can click on the lens. This display an empty peek view

**Fix**
Allow code lenses to only register a title for the lens with no actual backing command
@mjbvz mjbvz force-pushed the allow-title-only-code-lens branch from 149d89c to a0cb432 Compare April 12, 2017 22:48
@mjbvz
Copy link
Collaborator Author

mjbvz commented Apr 12, 2017

Ok, I've switched to using an empty string for the case when there is no command. This works with the existing api and minimizes the number of changes we need to make

@jrieken jrieken merged commit 2efc376 into microsoft:master Apr 13, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

Confusing References code lens
4 participants