-
Notifications
You must be signed in to change notification settings - Fork 29.5k
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
Add 'open image' context in markdown preview #234649
base: main
Are you sure you want to change the base?
Conversation
eb2624a
to
0b24327
Compare
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look. Looks good overall, mainly just want to keep most of the code in the command itself
@@ -679,6 +686,12 @@ export class DynamicMarkdownPreview extends Disposable implements IManagedMarkdo | |||
}); | |||
} | |||
|
|||
openImage(imagePath: string): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this code into the command. The second argument to vscode.open
is a ViewColumn
if you need to open in a specific editor group
|
||
public execute(args: { resource: string, imageSource: string }) { | ||
const source = vscode.Uri.parse(args.resource); | ||
const { fsPath } = vscode.Uri.parse(args.imageSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this as a uri. Otherwise it will only work for file:
images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the user can only open local images so It should have fsPath
. What is other possible options?
|
||
public execute(args: { resource: string; imageSource: string }) { | ||
const source = vscode.Uri.parse(args.resource); | ||
const imageSourceUri = vscode.Uri.file(vscode.Uri.parse(args.imageSource).path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only need a single call to vscode.Uri.parse
. Using vscode.Uri.file
again means it will only work if the workspace is on the local file system
If parse isn't enough, try seeing if you can pass the fill string uri to it. It should be something like: file:///path/to/workspace/image.png
public execute(args: { resource: string; imageSource: string }) { | ||
const source = vscode.Uri.parse(args.resource); | ||
const imageSourceUri = vscode.Uri.file(vscode.Uri.parse(args.imageSource).path); | ||
vscode.commands.executeCommand('vscode.open', imageSourceUri, this._webviewManager.findPreview(source)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findPreview
returns a preview which wont be understood by vscode.open
. Instead you want to get the view column where the preview is showing. This isn't exposed today but can be accessed from the WebviewPanel
stored on a MarkdownPreview
.
resolve #184553
This pull request aims to check whether an
img.src
is local or not, given that a localsrc
attribute is automatically changed to a URI internally.I've utilized the fact that any non-local
src
attribute will not be altered, providing a basis for differentiation.I'm open to any suggestions for better ways to achieve this.