Schwartzkers / cvs-scm

CVS Extension for Visual Studio Code
Other
6 stars 3 forks source link

"Changes" view with wrong encoding #9

Open viniiivs opened 1 year ago

viniiivs commented 1 year ago

In the 'Changes' view, the left file that is opened does not adhere to the configured encoding settings, unlike the right file. As a result, it becomes challenging to review the changes due to the presence of '�' characters. Could the encoding configuration be adjusted to ensure consistency in the encoding setting for both files?"

image

Schwartzkers commented 1 year ago

That is strange. I’ll have to investigate to determine if there’s anything I can do in the extension. I use the built in VS Code diff API for the Changes view. On the right is your working copy and the left is the output from “cvs -Q update -C -p ” (stdout). I’ve not adjusted any encoding so everything should be using the defaulting encoding from the vs code settings.

Do you see the encoding issue if you run the "cvs -Q update -C -p " command from a terminal?

Not sure is this following will help: https://stackoverflow.com/questions/38528384/how-to-setup-visual-studio-code-to-detect-and-set-the-correct-encoding-on-file-o

I’ll continue to investigate if there’s anything I can do in the extension to resolve the issue.

Thank you for bringing this to my attention.

Schwartzkers commented 1 year ago

I've been able to reproduce the issue. Not sure how to fix it yet, but I'll keep investigating.

viniiivs commented 1 year ago

I tried using the command "cvs -Q update -C -p" but it printed so much that I couldn't tell if it was right or not. It might be something related to my repository. However, the option to identify the encoding did not work. Nor forcing to always open with a specific encoding. I'm glad you were able to reproduce the issue, anything I can help with let me know.

GergelyZsolt commented 3 months ago

It's a problem for me too. Couldn't it be solved in a way similar to the patch below? Unfortunately, I couldn't try it.

File to modify: utility.ts

-import { Uri } from 'vscode';
+import { Uri, workspace } from 'vscode';
 import { cvsCommandLog } from './extension';

 export class CmdResult {
@@ -60,7 +60,7 @@

         const cmd = spawn(cvsCommand, [""], options);

-        cmd.stdout.setEncoding('utf8');
+        cmd.stdout.setEncoding(workspace.getConfiguration('files').get<string>('encoding') || 'utf8');
         cmd.stderr.setEncoding('utf8');

         cmd.stdout.on("data", (data: any) => {
Schwartzkers commented 3 months ago

I agree, this issue makes it very difficult to review changes. I've tried several fixes and none of them have resolved the issue. And unfortunately the proposed fix from GergelyZsolt did not work either.

I'll keep investigating. I'm open to any suggestions if you have any thoughts on how to resolve.

viniiivs commented 3 months ago

@Schwartzkers just forked the project and created a PR #10 .

To correctly set the encoding when comparing two files using a cmd command, it is necessary to use "latin1" to convert the "iso88591" output. I created a variable in settings to enforce encoding to a specific charset "cmd.encoding": "latin1", similar to the default VS Code setting 'files.encoding': 'iso88591' for opening a new file. Maybe it's a temporary change, but it's working well for ISO-8859-1 files. I also tried @GergelyZsolt's suggestion, but the command didn't obtain the correct encoding from the modified file, instead, it used the encoding from the unchanged file. Perhaps it's necessary to study the vscode library more thoroughly to understand how to do it.

Note: When I saved the file with another encodings like ISO-8859-16, for example, the file also appeared as iso88591 using file -i filename command on Linux. Meanwhile, other encodings showed as us-ascii, unknown-8bit, utf-16be and utf-16le in addition to utf8 and iso88591. Maybe this could be improved later.

Schwartzkers commented 3 months ago

@viniiivs Thank you for the analysis. It was very helpful and appreciated!

I released version 1.13.0 (pre-release) today with a slightly different approach, but based on your findings and some reading of node.js buffer encoding for ISO-8859-1. I liked how you added a config item for encoding, but I'm not sure yet if I want to keep it as a configuration item at this point.

Hopeful the pre-release will work for you and the code diff will be more useful.

viniiivs commented 3 months ago

The pre-release seems to have worked well! Thank you, @Schwartzkers. I just tested it here. I'm glad I could help, I've been procrastinating for a few weeks to send you this information, but I was busy, and I also wanted to try doing it myself to learn.

I think it's fixed for now.

Hopefully, I can assist more in the future. 😊

image

Schwartzkers commented 3 months ago

Excellent! Thank-you for letting me know it worked @viniiivs .

I'll continue to explore the encoding solution. I'm curious/worried how many other encoding types have a similar problem?

Your solution has given me someone to think about on how to allow the user to "work around" the issue on their own.

Cheers!

GergelyZsolt commented 3 months ago

I am currently working on a solution based on how the git plugin works. This would handle all encoding and auto-guessing as well.

GergelyZsolt commented 3 months ago

I sent the pull request yesterday: #11

I tested it in a latin2 repository. Please try it to see if it works well for you too. I hope that a good solution can be prepared based on this.

Schwartzkers commented 3 months ago

@GergelyZsolt great idea using the git extension as a guide. I'll review your changes, test and if it works as expected I'll merge.

Thank you for your efforts!

Schwartzkers commented 3 months ago

@GergelyZsolt your changes have been merged into the latest pre-release (v1.13.1). I made a few edits to respect copyright ethics. Thanks again for your efforts!

I did some basic testing with the changes. Everything seems to be working. I'll turn the release into a full release after some soak time to ensure no adverse affects are caught.

viniiivs commented 3 months ago

@Schwartzkers in the new pre release, my vscode didn't recognize the folder as a cvs repository. image

image

Schwartzkers commented 3 months ago

@viniiivs I saw that too. I've not encountered this before and I'm not sure what the issue is. You can revert to v1.13.1 while I try and resolve. Click on Uninstall and select the previous version.

Schwartzkers commented 3 months ago

@viniiivs The issue should now be fixed with v1.13.2. I forgot to add the run-time dependencies.

GergelyZsolt commented 3 months ago

Sorry about the dependencies error, it was my fault. The pre-release works fine for me now.

I even tried handling stderr with a corrupted .cvspass file in debug mode, but I didn't experience any errors.

Schwartzkers commented 3 months ago

@GergelyZsolt No worries, I should have caught this issue in my release testing, but I got a little lazy and skipped a step. I usually make a vsix package and test installing it and running it. The good news is that I can make pre-releases where users will be more understanding if an error occurs.

Thanks for testing it out. I'll give the pre-release a few weeks before promoting it to v1.14.0.