-
Notifications
You must be signed in to change notification settings - Fork 461
fix(process): improve stdout encoding handling in read_line functions #1471 #3148
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
base: v2
Are you sure you want to change the base?
Conversation
|
CC @FabianLars, this is initial start. Potential to continue improve with your advice.
If auto encoding is required, current hardcoded for encoding need change to system encoding page detect, and brings more cross-platform support. |
Package Changes Through f5858bfThere are 21 changes which include dialog-js with minor, dialog with minor, log with minor, log-js with minor, localhost with patch, barcode-scanner with patch, barcode-scanner-js with patch, deep-link with patch, deep-link-js with patch, shell with patch, shell-js with patch, http with patch, http-js with patch, nfc with patch, nfc-js with patch, updater with minor, updater-js with minor, upload with minor, upload-js with minor, websocket with patch, websocket-js with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
f31eb16 to
f5858bf
Compare
|
@Legend-Master Do you know if this could also cause issues? This feels like it depends on the spawned process eg iirc node also uses utf8 on all platforms by default 🤔 |
|
Since this is the raw output of the command in And for the encoding, I think it really depends on the command/program you spawned, it's not something we could assume (by the way, windows-1252 code page / encoding isn't the one used in all applications, it's only for apps using ANSI with
|
let decoder = encoding_rs::WINDOWS_1252;
let (cow, _, had_errors) = recoder.decode(&line);
let output: String = cow.into_owned();
Shall we leave this and documents on the usage of re-export Encoding? #1471 isn't a bug, user should consider encoding when use. |
|
Our JavaScript API defaulting to assume UTF-8 which I think is more or less in line with the Rust behavior (since JS one defaults to
Yeah, the program can output whatever they want technically, we can only provide sensible defaults that works for most |
sturcture Command {
encoding: Encoding
...
}
app_handle.sidecar(BIN).output_line_encoding(Encoding::Type)Some thing like this? The |
|
Ah, I see, let's just do the documentation work for now then |
I developed on macOS, need to make sure encoding behavior same as Windows.
Sidecar:
User side, code snippet
Output with mainline
Output with PR