Skip to content

Fix #73021#73154

Merged
mjbvz merged 2 commits intomicrosoft:masterfrom
hunghw:Fix-#73021
May 10, 2019
Merged

Fix #73021#73154
mjbvz merged 2 commits intomicrosoft:masterfrom
hunghw:Fix-#73021

Conversation

@hunghw
Copy link
Copy Markdown
Contributor

@hunghw hunghw commented May 2, 2019

This PR fix #73021 by add etag into cacheKey. If the image is modified, the editor will not remember the previous scale.

};

const cacheKey = descriptor.resource.toString();
const cacheKey = descriptor.resource.toString() + descriptor.etag;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

${descriptor.resource.toString()}${descriptor.etag}

Isn't this better ? Es6 String Literals

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@amarpathak what are you smoking?

Copy link
Copy Markdown
Contributor Author

@hunghw hunghw May 3, 2019

Choose a reason for hiding this comment

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

@amadare42 That’s right. I think we can add a symbol between these two string to separate them.

${descriptor.resource.toString()}:${descriptor.etag}

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.

@hwhung0111 well, I think you linked wrong person, but I want to mention that es6 string templating will call toString automatically so your line can be shortened:
${descriptor.resource}:${descriptor.etag}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is is not the best solution. ES6 literals are not suited for this purpose but injecting dynamic data into strings.

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.

@amadare42 I am sorry about that. And thanks for telling me about es6 string.
@KristofferTolboll2 If I add a colon in the string, is it better to use ES6 literals.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@hwhung0111 ES6 template literals are most used, when there are more static content, in this case, there is only the : which is static, everything else is dynamic, therefore it will decrease readability

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.

@KristofferTolboll2 I get your point. But why I still found ${message}: [${err.toString()} and ${this.scope}.${key} in source code? These are similar to this issue.

Copy link
Copy Markdown

@KristofferTolboll2 KristofferTolboll2 left a comment

Choose a reason for hiding this comment

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

This is not good practise, it makes the code less readible

Copy link
Copy Markdown

@KristofferTolboll2 KristofferTolboll2 left a comment

Choose a reason for hiding this comment

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

Don't change it Amar, it was better before

@mjbvz mjbvz added this to the May 2019 milestone May 3, 2019
@mjbvz
Copy link
Copy Markdown
Collaborator

mjbvz commented May 3, 2019

Thanks for the PR. It looks good to me.

We're currently stabilizing for the VS Code April release so I cannot merge this at this time but have marked it to be included in the may release

@KristofferTolboll2
Copy link
Copy Markdown

Thanks for the PR. It looks good to me.

We're currently stabilizing for the VS Code April release so I cannot merge this at this time but have marked it to be included in the may release

Great thanks @mjbvz

@mjbvz mjbvz merged commit 2bd51e5 into microsoft:master May 10, 2019
@mjbvz
Copy link
Copy Markdown
Collaborator

mjbvz commented May 10, 2019

This fix should be in the first VS Code 1.35 insiders build and is scheduled for the May VS code release

@hunghw hunghw deleted the Fix-#73021 branch May 28, 2019 14:37
@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.

Image View Remembers Zoom

6 participants