[vscode] Support encoding/decoding from workspace and while opening text documents #15873
Conversation
| } else { | ||
| encoding = options?.encoding ?? UTF8; | ||
| } | ||
| // see https://github.com/microsoft/vscode/blob/118f9ecd71a8f101b71ae19e3bf44802aa173209/src/vs/workbench/services/textfile/browser/textFileService.ts#L806 |
There was a problem hiding this comment.
Does this file contain enough copied code to warrant adding a copyright by MS notice?
There was a problem hiding this comment.
I added a copyright + comment that indicates where some parts of the code comes from.
| return { encoding, hasBOM }; | ||
| } | ||
|
|
||
| async $validateDetectedEncoding(uri: UriComponents | undefined, detectedEncoding: string | undefined, options: { encoding: string } | undefined): Promise<string> { |
There was a problem hiding this comment.
When I read validate..., I expect a method that returns an error if the parameters are not valid. Why not $getValidEncoding?
| @@ -74,6 +76,10 @@ export class DocumentDataExt { | |||
| ok(!this.disposed); | |||
| this.languageId = langId; | |||
| } | |||
| acceptEncoding(encoding: string): void { | |||
There was a problem hiding this comment.
This is never called. Do we need it?
There was a problem hiding this comment.
It is now called when encoding is changed. Thanks for catching this piece of missing code!
| const cached = this.editorsAndDocuments.getDocument(uri.toString()); | ||
| if (cached) { | ||
| if (cached && !options?.encoding) { |
There was a problem hiding this comment.
Why does passing an encoding invalidate the cache? Maybe we read the content with the same encoding before?
There was a problem hiding this comment.
Checked for the current encoding of the cached one. This may ease the reuse of cached version.
| @@ -80,6 +84,7 @@ export class WorkspaceExtImpl implements WorkspaceExt { | |||
| initialize(): void { | |||
| this.proxy = this.rpc.getProxy(Ext.WORKSPACE_MAIN); | |||
| this.logger = new PluginLogger(this.rpc, 'workspace'); | |||
| this.encodingService = new EncodingService(); | |||
There was a problem hiding this comment.
I added some bindings for the EncodingService, and used the injection rather than creating a new one.
github-project-automation
bot
moved this from Waiting on reviewers
to Needs merge
in PR Backlog
|
@rschnekenbu This seems to work O.K. for me, but could you try to get the tests to pass, please? |
|
@tsmaeder , I updated the PR according for your comments, thank for the review! |
|
@rschnekenbu could you just quickly rebase on master and push? I'd like to get some data on whether the electron update improved the failing tests. |
…ext documents. fix #15559 Contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
rschnekenbu
force-pushed
the
issues/15559
branch
from
2a214bc to
8a51ffe
Compare
|
Rebased on top of master |
What it does
Support the workspace openTextDocument, encode and decode methods with additional encoding options.
fix #15559
How to test
The following extension can be used:
This extension provides:
Here is also a sample workspace with a set of files with different encodings. This can be used as a test for the Encoding Sample: open with encoding action or reopen editor with encoding actions
workspace-encoding-sample.zip
15559.sample.encoding.mp4
Follow-ups
None known
Breaking changes
Attribution
Contributed on behalf of STMicroelectronics
Review checklist
Reminder for reviewers