Add API function that changes document language by mechatroner · Pull Request #55107 · microsoft/vscode

mechatroner · 2018-07-26T04:46:36Z

Hello!
This is my attempt to implement language changing API function #1800
As I've described in the ticket I need for my "Rainbow CSV" extension, so it can use content-based CSV autodetection to automatically change document language to "CSV", "TSV" etc.

So, I've added setLanguageById() function which does exactly this.
I've also created experimental version of my extension which uses this new API function, it is located in vsc_api branch of "Rainbow CSV" repo: https://github.com/mechatroner/vscode_rainbow_csv/tree/vsc_api

So to test setLanguageById() you can install my extension from vsc_api branch and create a csv file with the following content and save it as "movies_csv.txt" or "movies_csv.unknown_extension":

Avatar,United States,English,2009,James Cameron,Color,Action|Adventure|Fantasy|Sci-Fi,178
Pirates of the Caribbean: At World's End,United States,English,2007,Gore Verbinski,Color,Action|Adventure|Fantasy,169
The Dark Knight Rises,United States,English,2012,Christopher Nolan,Color,Action|Thriller,164
John Carter,United States,English,2012,Andrew Stanton,Color,Action|Adventure|Sci-Fi,132
Spider-Man 3,United States,English,2007,Sam Raimi,Color,Action|Adventure|Romance,156
Tangled,United States,English,2010,Nathan Greno,Color,Adventure|Animation|Comedy|Family|Fantasy|Musical|Romance,100
Avengers: Age of Ultron,United States,English,2015,Joss Whedon,Color,Action|Adventure|Sci-Fi,141
Batman v Superman: Dawn of Justice,United States,English,2016,Zack Snyder,Color,Action|Adventure|Sci-Fi,183
Superman Returns,United States,English,2006,Bryan Singer,Color,Action|Adventure|Sci-Fi,169
Pirates of the Caribbean: Dead Man's Chest,United States,English,2006,Gore Verbinski,Color,Action|Adventure|Fantasy,151
The Lone Ranger,United States,English,2013,Gore Verbinski,Color,Action|Adventure|Western,150

When opened in patched VSCode the file should be highlighted - this is the sign that content-based autodetection has worked and language id has been changed from 'plaintext' to 'CSV'

jrieken · 2018-07-26T07:51:26Z

src/vs/workbench/api/electron-browser/mainThreadEditors.ts

+		let textModel = mainThreadEditor.getModel();
+		let modelService = this._modelService;
+		let modeService = this._modeService;
+		if (!modelService || !modeService) {


They won't be null/undefined - the injector will explode when a service cannot be found.

jrieken · 2018-07-26T07:52:07Z

src/vs/workbench/api/electron-browser/mainThreadEditors.ts

+			return TPromise.wrapError(new Error('modeService is null for some unit tests'));
+		}
+		let mode = modeService.getOrCreateModeByLanguageName(languageId);
+		modelService.setMode(textModel, mode);


maybe check if that mode actually exists and ignore 'falsy' modes

Current implementation getOrCreateModeByLanguageName will return "plaintext" mode if requested language wasn't found, so I think the only way to check the validity of requested language is to check language registry prior to changing the mode. BTW we have another problem here: the "language_id" variable in my PR is actually language name, as I've just discovered while testing the extension and reading the source code (I didn't notice this earlier because in my test both language id and language name is "CSV", but e.g. for "cpp" -> "C++" the situation is different). So I think we should either rename "language_id" -> "language_name" and "SetLanguageById" -> "SetLanguageByName" or rewrite my code a little, so the function would accept actual "language_id" (e.g. "cpp" instead of "C++" ).

jrieken · 2018-07-26T07:55:29Z

src/vs/workbench/api/node/extHostTextEditor.ts


+	setLanguageById(language_id: string): void {
+		this._runOnProxy(() => this._proxy.$trySetLanguageById(this._id, language_id));
+	}


We need to think where we expose this as API. The catch is that the API treats a mode change as a document-close/document-open-cycle. That means having that function on TextDocument isn't a good idea (calling a method shouldn't make the receiver disappear). The same is true for editors - for the API an editor a document (instance) that's currently visible in an editor. So, may expose this as a new function in the language namespace.

To expose the API, make the change in vscode.d.ts or (better) vscode.proposed.d.ts and then fix the compile error in extHost.api.impl.ts

I totally missed this part because I've tested this with a javascript - based plugin which, unlike typescript - based, doesn't need the interface.

having that function on TextDocument isn't a good idea (calling a method shouldn't make the receiver disappear). The same is true for editors

What do you mean "disappear" ? Visually it looks like just syntax highlighting change, there is no opening/closing. And at javascript level both window.activeTextEditor and window.activeTextEditor.document are the same objects as before the language change. I even wrote a test extension to check this: https://github.com/mechatroner/vscode-wordcount/blob/bb645b08c605b71d0aea6be7c38a6131ee079a28/extension.ts#L23
So "same editor" and "same doc" will be printed in the console after language change.

Also I have the following 2 arguments for keeping the function in TextEditor (or moving it to TextDocument):

This interface is similar to ones that Atom and Sublime Text 3 provide: see TextEditor.setGrammar(grammar) https://atom.io/docs/api/v1.2.1/TextEditor#instance-setGrammar for Atom and View.set_syntax_file(syntax_file_path) https://www.sublimetext.com/docs/3/api_reference.html for Sublime.

Do I understand correctly that in language namespace this will be a two-argument function, like: changeDocumentLanguage(doc_uri, language_id) ? Because in that case getting document.uri just to immediately use it to change the language is less convenient, and most people would also wrap this in some error checking code because it is not obvious if all documents have valid/uniq URIs.

jrieken · 2018-07-26T07:59:32Z

src/vs/workbench/api/node/extHost.protocol.ts

 	$tryShowEditor(id: string, position: EditorViewColumn): TPromise<void>;
 	$tryHideEditor(id: string): TPromise<void>;
 	$trySetOptions(id: string, options: ITextEditorConfigurationUpdate): TPromise<void>;
+	$trySetLanguageById(id: string, languageId: string): TPromise<void>;


Consider moving this ExtHostLanguages, esp given below considerations. Then, the change shouldn't work for an editor id but just for a document uri.

jrieken · 2018-07-26T08:01:24Z

Looks good. The code is proper but I'd move it into a different place.

mechatroner · 2018-08-03T04:53:59Z

I've added two alternative functions: language change by name, and language change by (uri, language_id) pair from "languages" namespace as you suggested. So you can now choose which one of these 3 functions to keep =)

jrieken · 2018-08-06T12:31:31Z

src/vs/vscode.d.ts

+		 * @param languageName new language name
+		 */
+		setLanguageByName(languageName: string): void;
+


We don't want those because they will change that editor instance (changing the mode changes the document instance and that changes the editor instance). Please remove these functions and what it takes to implement them

jrieken · 2018-08-06T12:36:08Z

src/vs/vscode.d.ts

 	 */
 	export namespace languages {

+		export function setLanguageById(documentUri: Uri, languageId: string): Thenable<void>;


Would be better to accept a Document here because we can only do that for existing documents. Then, I don't think that set is a good prefix because we aren't setting an instance property of the document but we re-create that document with a different language identifier. Maybe changeLanguge and make it return a document, e.g. function changeLanguage(document: TextDocument, language: string): Thenable<TextDocument>

we re-create that document with a different language identifier

Hmm, do you mean this code in extHostDocuments.ts?

public $acceptModelModeChanged(uriComponents: UriComponents, oldModeId: string, newModeId: string): void { const uri = URI.revive(uriComponents); const strURL = uri.toString(); let data = this._documentsAndEditors.getDocument(strURL); // Treat a mode change as a remove + add this._onDidRemoveDocument.fire(data.document); data._acceptLanguageId(newModeId); this._onDidAddDocument.fire(data.document); }

Because my experiments and the code shows that document doesn't get "recreated". The following condition in extHostDocumentData.ts prevents recreation of the document:

get document(): vscode.TextDocument { if (!this._document) { const data = this; this._document = { ...

But if I change _acceptLanguageId function there by adding the last line

_acceptLanguageId(newLanguageId: string): void { ok(!this._isDisposed); this._languageId = newLanguageId; this._document = null; // I've added this line! }

then the document gets recreated indeed.

Well, we fire a remove and add event to underline at least the intention ;-) Unsure why we don't reset the document but I would leave it like that for now and tackle that independently.

mechatroner · 2018-08-15T02:12:21Z

I've renamed "setLanguageById' -> "changeLanguage" and removed unneeded code. Do you still want me to change the function interface and return a "new" document (although I am not sure how to properly implement this, and it would be very tempting to just return the input document, since it doesn't change as we've figured out) ?

mechatroner · 2018-08-16T03:16:53Z

Oops, I've just realized that I forgot to replace URI argument with TextDocument. Just fixed this. Still not sure what to do with the return value.

jrieken · 2018-08-24T15:31:27Z

src/vs/editor/common/services/modeService.ts

 	getMode(commaSeparatedMimetypesOrCommaSeparatedIds: string): IMode;
 	getOrCreateMode(commaSeparatedMimetypesOrCommaSeparatedIds: string): TPromise<IMode>;
 	getOrCreateModeByLanguageName(languageName: string): TPromise<IMode>;
+	getOrCreateModeByLanguageId(modeId: string): TPromise<IMode>;


Why was this added?

Do you suggest to use getOrCreateModeByLanguageName(getLanguageName(modeId)) instead? OK, probably this is better (and I totally missed the getLanguageName method), but this will lead to the following chain "modeId" -> "language name" -> "modeId", in my proposal there are no such conversions at the cost of adding additional method.
The alternative getMode([language_id]) is not very good either since the underlying extractModeIds() methods tries mime types first and language identifiers second, if it was the other way around, then I think, it would much better fit our case. Or is there any third option that I've missed?

jrieken · 2018-08-24T15:32:36Z

src/vs/editor/common/services/languagesRegistry.ts


+	public isRegisteredModeExact(modeId: string): boolean {
+		return hasOwnProperty.call(this._languages, modeId);
+	}


Isn't getLanguageName(modeId) enough? This looks a little boilerplate

Yep, I think getLanguageName(modeId) is enough. I didn't notice that method, good catch!

jrieken · 2018-08-24T15:34:19Z

src/vs/vscode.d.ts

 	 */
 	export namespace languages {

+		export function changeLanguage(document: TextDocument, languageId: string): Thenable<void>;


We should start with proposed API. Please move this to vscode.proposed.d.ts (inside a languages-namespace).

jrieken · 2018-08-24T15:34:54Z

src/vs/workbench/api/node/extHost.api.impl.ts

 				return extHostLanguages.getLanguages();
 			},
+			changeLanguage(document: vscode.TextDocument, languageId: string): TPromise<void> {
+				return extHostLanguages.changeLanguage(document.uri, languageId);


This should be guarded with the proposedApiFunction-util

jrieken · 2018-08-27T12:19:48Z

Today is the last day we can make changes before we enter the August endgame. Therefore, I went in and made two tweaks

The mode update logic now uses getOrCreateMode which is the way to get to a mode, esp. when VS Code hasn't loaded/activated that mode yet
I have moved the API to vscode.proposed.d.ts so that it can go the api finalisation way next month (see https://github.com/Microsoft/vscode/wiki/Extension-API-process#flow-chart-of-the-process)

jrieken · 2018-08-27T12:19:58Z

Thanks @mechatroner!

mechatroner · 2018-08-27T12:54:29Z

Thank you, @jrieken !

Add API function that changes document language: microsoft#1800

2582f42

mechatroner changed the title ~~Add API function that changes document language: #1800~~ Add API function that changes document language Jul 26, 2018

jrieken self-assigned this Jul 26, 2018

jrieken added this to the August 2018 milestone Jul 26, 2018

ffes mentioned this pull request Jul 26, 2018

Set language with modeline ctlajoie/vscode-modelines#2

Closed

jrieken requested changes Jul 26, 2018

View reviewed changes

Add two alternative language-changing interface functions

2eabbee

jrieken requested changes Aug 6, 2018

View reviewed changes

cleanup and rename

e91573d

jrieken mentioned this pull request Aug 15, 2018

PR shows wrong diff microsoft/vscode-pull-request-github#203

Closed

improve function interface

bd02bdb

jrieken reviewed Aug 24, 2018

View reviewed changes

jrieken added 2 commits August 27, 2018 14:08

use getOrCreateMode and move api to proposed-stage first

5ef859f

Merge branch 'master' into pr/mechatroner/55107-1

549d608

jrieken approved these changes Aug 27, 2018

View reviewed changes

jrieken merged commit 7bd7dbb into microsoft:master Aug 27, 2018

g-arjones mentioned this pull request Sep 29, 2018

custom manifests (manifest.*) are not highlighted properly g-arjones/vscode-autoproj#5

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

mechatroner commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mechatroner Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Jul 26, 2018

Uh oh!

mechatroner commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mechatroner commented Aug 15, 2018

Uh oh!

mechatroner commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mechatroner Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Aug 27, 2018

Uh oh!

jrieken commented Aug 27, 2018

Uh oh!

mechatroner commented Aug 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mechatroner commented Jul 26, 2018 •

edited

Loading

mechatroner Jul 29, 2018 •

edited

Loading

mechatroner Aug 26, 2018 •

edited

Loading