-
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 #5294: 1. reopen file with another encoding; 2. set default encod… #5371
Conversation
57f9478
to
e66c48f
Compare
e66c48f
to
334a668
Compare
334a668
to
64dec32
Compare
73f8586
to
3686967
Compare
This PR introduces implicit dependencies between editor and filesystem extensions. It should be refactored to remove coupling. These extensions cannot depend each other. There should be some sort of intermediate API to make it work. |
@akosyakov Thanks for your advice! I removed the coupling between editor and filesystem extensions by introducing 'Resource'. Codes are updated. Could you please make a further review?
|
888d018
to
9b1ce97
Compare
All commits merged into one and rebased to solve the conflict. |
9b1ce97
to
c115d0c
Compare
@akosyakov Hi, I fixed all but the 'IEncodingSupport' and 'overwriteEncoding' (please see comments above). Thanks for your precious advice. And please let me know if further modifications are ought to be made. |
c115d0c
to
3704722
Compare
@akosyakov Could you please take a further look at this? The PR has been up here for over two months, so I want to focus on it in the following few days and hopefully meet your requirement soon. |
We have many PRs and work sorry. PRs addressing VS Code API compatibility issues gets highest priority right now on my list. I will have a look when i have time. |
Could someone test against similar behaviour in VS Code? We also need to look out for race conditions with saving, i.e. changing encoding and starting typing immediately or with delay and see whether it leads to any troubles. |
3704722
to
c9c262c
Compare
Code-wise it looks good to me now. But I don't have time to verify according to #5371 (comment). |
…. configuration for default encoding; Signed-off-by: Cai Xuye <[email protected]>
c9c262c
to
9f18c86
Compare
@a1994846931931 Could you define tests for this PR? We will test that changes don't break anything without changing encoding and then use your tests to test encoding/decoding plus check whether expectations of tests are aligned with behaviour of VS Code. If everything is fine we land it. Also just FYI: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-build-and-test |
@akosyakov I have rewritten the PR message, based on the new PR template. So |
@westbury @kittaakos If you can try tests from @westbury @kittaakos maybe we can have os-windows team? 🤔 |
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.
Code-wise it looks good, I've tested according How to test
and it seems to behave well and does not break existing behaviour.
@vince-fugnitto please can you clarify your suspicious about CQs? We cannot merge without it.
I think it's alright, the original CQ covered both the |
…ing;
Signed-off-by: Cai Xuye [email protected]
What it does
Added dependencies:
How to test
Test File
TEST1
Preparation:
files.encoding
toutf8
Process:
TEST2
Preparation:
files.encoding
toutf8
Process:
TEST3
Preparation:
files.encoding
toutf8
Process:
file -i ${FILENAME}
command on linux (though it could be wrong);TEST4
Preparation:
Process:
files.encoding
(Files -> encoding) to the encoding of your test file;NOTE
Currently, I have only tested it on Ubuntu.
Review checklist
Reminder for reviewers