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

Fix #84111 #84610

Merged
merged 3 commits into from Nov 15, 2019
Merged

Fix #84111 #84610

merged 3 commits into from Nov 15, 2019

Conversation

@shizengzhou
Copy link
Contributor

shizengzhou commented Nov 12, 2019

This PR fixes #84111

@@ -105,7 +133,8 @@ class Preview extends Disposable {
switch (message.type) {
case 'size':
{
this._imageSize = message.value;
const { size } = fs.statSync(resource.path);

This comment has been minimized.

Copy link
@mjbvz

mjbvz Nov 12, 2019

Contributor

Many of the images we load are not on the file system. They may be remote or data uris, which fs will not understand

Try using VS Code's file system api instead:

stat(uri: Uri): Thenable<FileStat>;

Copy link
Contributor

mjbvz left a comment

I think the binary size should be a new status bar item. That way users can disable the dimensions or binary size independently.

See SizeStatusBarEntry for a good starting point for creating the new status bar item. The BinarySize class can also live in that new file

@@ -105,8 +132,11 @@ class Preview extends Disposable {
switch (message.type) {
case 'size':
{
this._imageSize = message.value;
this.update();
vscode.workspace.fs.stat(resource).then(stat => {

This comment has been minimized.

Copy link
@mjbvz

mjbvz Nov 15, 2019

Contributor

This should not happen inside the message handler. The image's dimensions and binary size should be computed independently

@mjbvz mjbvz added this to the November 2019 milestone Nov 15, 2019
@mjbvz mjbvz merged commit 14e6ad3 into microsoft:master Nov 15, 2019
4 of 5 checks passed
4 of 5 checks passed
linux
Details
windows
Details
darwin
Details
VS Code #20191115.7 failed
Details
license/cla All CLA requirements met.
Details
@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Nov 15, 2019

Thanks! Will be in the next VS Code insiders build and is scheduled to go out with VS Code 1.41

@shizengzhou shizengzhou deleted the shizengzhou:image-size branch Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.