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

Add API function that changes document language #55107

Merged
merged 6 commits into from Aug 27, 2018

Conversation

@mechatroner
Copy link
Contributor

commented Jul 26, 2018

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'

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

@jrieken jrieken self-assigned this Jul 26, 2018

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

@ffes ffes referenced this pull request Jul 26, 2018

Closed

Set language with modeline #2

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

This comment has been minimized.

Copy link
@jrieken

jrieken Jul 26, 2018

Member

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

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

This comment has been minimized.

Copy link
@jrieken

jrieken Jul 26, 2018

Member

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

This comment has been minimized.

Copy link
@mechatroner

mechatroner Jul 29, 2018

Author Contributor

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++" ).

@@ -435,6 +435,10 @@ export class ExtHostTextEditor implements vscode.TextEditor {
this._trySetSelection();
}

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

This comment has been minimized.

Copy link
@jrieken

jrieken Jul 26, 2018

Member

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.

This comment has been minimized.

Copy link
@jrieken

jrieken Jul 26, 2018

Member

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

This comment has been minimized.

Copy link
@mechatroner

mechatroner Jul 29, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@mechatroner

mechatroner Jul 30, 2018

Author Contributor

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):

  1. 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.
  2. 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.
@@ -193,6 +193,7 @@ export interface MainThreadTextEditorsShape extends IDisposable {
$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>;

This comment has been minimized.

Copy link
@jrieken

jrieken Jul 26, 2018

Member

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

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

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

@mechatroner

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

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 =)

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

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 6, 2018

Member

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

@@ -7375,6 +7391,8 @@ declare module 'vscode' {
*/
export namespace languages {

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

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 6, 2018

Member

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>

This comment has been minimized.

Copy link
@mechatroner

mechatroner Aug 8, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 14, 2018

Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

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) ?

@jrieken jrieken referenced this pull request Aug 15, 2018

Closed

PR shows wrong diff #203

@mechatroner

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

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.

@@ -46,5 +46,6 @@ export interface IModeService {
getMode(commaSeparatedMimetypesOrCommaSeparatedIds: string): IMode;
getOrCreateMode(commaSeparatedMimetypesOrCommaSeparatedIds: string): TPromise<IMode>;
getOrCreateModeByLanguageName(languageName: string): TPromise<IMode>;
getOrCreateModeByLanguageId(modeId: string): TPromise<IMode>;

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 24, 2018

Member

Why was this added?

This comment has been minimized.

Copy link
@mechatroner

mechatroner Aug 26, 2018

Author Contributor

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?

@@ -211,6 +211,10 @@ export class LanguagesRegistry {
return Object.keys(this._nameMap);
}

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

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 24, 2018

Member

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

This comment has been minimized.

Copy link
@mechatroner

mechatroner Aug 26, 2018

Author Contributor

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

@@ -7375,6 +7375,8 @@ declare module 'vscode' {
*/
export namespace languages {

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

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 24, 2018

Member

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

@@ -258,6 +258,9 @@ export function createApiFactory(
getLanguages(): TPromise<string[]> {
return extHostLanguages.getLanguages();
},
changeLanguage(document: vscode.TextDocument, languageId: string): TPromise<void> {
return extHostLanguages.changeLanguage(document.uri, languageId);

This comment has been minimized.

Copy link
@jrieken

jrieken Aug 24, 2018

Member

This should be guarded with the proposedApiFunction-util

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

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

@jrieken

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Thanks @mechatroner!

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

1 of 2 checks passed

VSTS: VS Code in progress
Details
license/cla All CLA requirements met.
Details
@mechatroner

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Thank you, @jrieken !

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.