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.

Sign up for GitHub

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

Jump to bottom

fix #3338 Format Document on save #4012

Merged
merged 1 commit into from
Jan 18, 2019
Merged

fix #3338 Format Document on save #4012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/browser/saveable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export namespace Saveable {
}
// tslint:disable-next-line:no-any
export async function save(arg: any): Promise<void> {
const saveable = getDirty(arg);
const saveable = get(arg);
Copy link
Member

Choose a reason for hiding this comment

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

it should be reverted right? save should not do anything if a document is not dirty

if (saveable) {
await saveable.save();
}
Expand Down
6 changes: 6 additions & 0 deletions packages/editor/src/browser/editor-preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,11 @@ export const editorPreferenceSchema: PreferenceSchema = {
'default': false,
'description': 'Enable auto indentation adjustment.'
},
'editor.formatOnSave': {
'type': 'boolean',
'default': false,
'description': 'Enable format on manual save.'
},
'editor.formatOnType': {
'type': 'boolean',
'default': false,
Expand Down Expand Up @@ -519,6 +524,7 @@ export interface EditorConfiguration {
'editor.autoClosingBrackets'?: boolean
'editor.autoIndent'?: boolean
'editor.formatOnType'?: boolean
'editor.formatOnSave'?: boolean
'editor.formatOnPaste'?: boolean
'editor.dragAndDrop'?: boolean
'editor.suggestOnTriggerCharacters'?: boolean
Expand Down
2 changes: 1 addition & 1 deletion packages/monaco/src/browser/monaco-editor-model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
}

protected async doSave(reason: TextDocumentSaveReason, token: CancellationToken): Promise<void> {
if (token.isCancellationRequested || !this.resource.saveContents || !this.dirty) {
if (token.isCancellationRequested || !this.resource.saveContents) {
Copy link
Member

Choose a reason for hiding this comment

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

it should be reverted? save should be aborted if a document is not dirty

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess what @lmcbout wanted to do here is "formatting the file on users hitting CTRL+S even if the file is not dirty" - vsCode does it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly the reason, to allow "CTRL+S" like vscode does

Copy link
Member

@akosyakov akosyakov Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please check whether VS Code actually save changes or only try to apply formatting. In cloud case sending unchanged content is expensive and we should avoid doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried VSCode with the file (theia/.vscode/launch.json). The file is not dirty. By selecting "CTRL+S", it reformatted the file and saved it.

Copy link
Member

Choose a reason for hiding this comment

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

I meant in our case if formatting does nothing we should not write anything to the disk. It is expensive for large files.

Copy link
Contributor Author

@lmcbout lmcbout Jan 16, 2019 •
edited by paul-marechal
Loading

Choose a reason for hiding this comment

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

After the formatting, the frontend is using the monaco-editor-model.doSave()
In that method, we check if there is some changes (~ line 320) code:

        const changes = this.popContentChanges();
        if (changes.length === 0) {
            return;
        }

This is the same as testing isDirty(). In fact we could use:

if (this.dirty) return;

to replace those lines above and it would do the same here . So as a resume, the file remains in the frontend if there is no formatting , they are not sent to the backend when there is no changes.

return;
}

Expand Down
11 changes: 10 additions & 1 deletion packages/monaco/src/browser/monaco-editor-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { EditorPreferenceChange, EditorPreferences, TextEditor, DiffNavigator, E
import { DiffUris } from '@theia/core/lib/browser/diff-uris';
import { inject, injectable } from 'inversify';
import { DisposableCollection } from '@theia/core/lib/common';
import { MonacoToProtocolConverter, ProtocolToMonacoConverter } from 'monaco-languageclient';
import { MonacoToProtocolConverter, ProtocolToMonacoConverter, TextDocumentSaveReason } from 'monaco-languageclient';
import { MonacoCommandServiceFactory } from './monaco-command-service';
import { MonacoContextMenuService } from './monaco-context-menu';
import { MonacoDiffEditor } from './monaco-diff-editor';
Expand Down Expand Up @@ -142,6 +142,15 @@ export class MonacoEditorProvider {
const options = this.createMonacoEditorOptions(model);
const editor = new MonacoEditor(uri, model, document.createElement('div'), this.m2p, this.p2m, options, override);
toDispose.push(this.editorPreferences.onPreferenceChanged(event => this.updateMonacoEditorOptions(editor, event)));
editor.document.onWillSaveModel(event => {
event.waitUntil(new Promise<monaco.editor.IIdentifiedSingleEditOperation[]>(async resolve => {
if (event.reason === TextDocumentSaveReason.Manual && this.editorPreferences['editor.formatOnSave']) {
await this.commandServiceFactory().executeCommand('monaco.editor.action.formatDocument');
}
resolve([]);
}));
});

return editor;
}
protected createMonacoEditorOptions(model: MonacoEditorModel): MonacoEditor.IOptions {
Expand Down