-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix #3338 Format Document on save #3640
Conversation
packages/editorconfig/src/browser/editorconfig-document-manager.ts
Outdated
Show resolved
Hide resolved
packages/editorconfig/src/browser/editorconfig-document-manager.ts
Outdated
Show resolved
Hide resolved
this.formatDocument(monacoEditor) | ||
]).then(() => { | ||
if (edit) { | ||
edits.push(edit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.getEditsTrimmingTrailingWhitespaces()
also returns "edit" that should be pushed in to edits
. I believe it is missing in the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, adjusted the return value
5d64706
to
360b9c3
Compare
@lmcbout why does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested. Works.
@vince-fugnitto If we allow to formatOnSave when the auto-save = "on", depending of the file type it becomes annoying to see the empty space or empty line goes away before you type new chars into the file because the file has been saved and re-formatted |
@lmcbout |
@akosyakov Do you have any comments before we merge it since it has already been approved. |
monacoEditor.cursor = cursor; | ||
}, 0); | ||
} | ||
if (edit) { edits.push(edit); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be broken into multiple lines :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, Done
@lmcbout I don't think we should limit ourselves to |
Format on Save is only available when the preference autoSave is "OFF" and the preference 'editor.formatOnSave' is "true" Signed-off-by: Jacques Bouthillier <jacques.bouthillier@ericsson.com>
Why is it related to editorconfig extension? It should work even without this extension, only with editor + monaco extensions. |
@akosyakov could you please suggest which package this logic belongs to, if not put into editorconfig? why do we not want to put all the related things in one place ? |
Ideally, it should go to the editor extension, but since it is relays on monaco apis i would implement it in the monaco extension.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Anton said, this change shouldn't live in editorconfig
.
Also, after further investigation, it would seem that onSave contributions such as edits are not correctly handled in Theia as of now. Mostly concurrency issues.
Fix #3338
Depends on issue #3617
Note: If #3617 not available, then the java file will need to be saved twice when the user wants to format the file and cleaning up the lines from empty spaces.
Format on Save is only available when the preference autoSave is "OFF"
and the preference 'editor.formatOnSave' is "true"
Signed-off-by: Jacques Bouthillier jacques.bouthillier@ericsson.com