-
Notifications
You must be signed in to change notification settings - Fork 3k
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
terminal: Improve default locale handling #18967
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have @shish on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
@cla-bot check (although zed.dev appears to be melting - 90% "internal server error" responses at every step of the CLA signing process...) |
The cla-bot has been summoned, and re-checked this pull request! |
Heads up that macOS has some "special" locale requirements. Not sure that that applies to this change or not (I don't think it should if you are just changing which variable is set). #13691 (comment) |
I don't have a mac device to test with, but reading the code and the specs, I think this new code should still work there :) Though reading that thread makes me wonder what's up - that thread seems to imply that the Regardless, while I'm not certain what the best fix is, I am pretty confident that this PR is a good step forwards compared to the status quo :) |
Hey! Thanks for opening this. I'm not as deep in this topic as you are, so I was wondering whether you think your PR here clashes with what we discussed here: #13691 (comment) |
Already mentioned above. |
So digging into this even further, I think we're both not-quite-optimal and I had misunderstood what the My gut says that if you launch zed from a terminal, and then launch a terminal inside zed, then the inner terminal should have the same environment as the outside terminal (minus a few zed-specific additions such as For the MacOS case of "sometimes there is no locale set, which breaks things", we probably do want to ensure some kind of fallback locale is set (but using the low-priority So my current code as-is would cover all cases correctly, but not optimally (with the current code, if the user has locale settings, then we copy-paste them into the override list -- it'd be better to leave the override list empty if an override isn't needed) |
Actually there are even more layers in this rabbit hole -- looks like the [0] zed/crates/project/src/terminals.rs Lines 119 to 127 in 9bc4e3b
So... the correct-est behaviour would be to fix that inheritance process, and then only set LANG if it doesn't exist inside |
745fa15
to
817fb55
Compare
Looking into "reasons why the zed CLI might report an empty environment", I believe my case is a related-but-separate bug ( [0] zed/crates/project/src/project.rs Line 736 in 596d8b2
|
Man, I swear I didn't see the two comments above mine 🤦 Sorry about that!
It's not a bug. When you run When you run the Zed CLI — also called (I actually wrote even more about this whole environment stuff here: https://github.com/zed-industries/zed/blob/main/docs/src/environment.md) Now, all that being said, if you think this change is good and @someone13574 also thinks it makes sense — let's try it? |
* Use `LANG` instead of `LC_ALL` (`LC_ALL` is the highest priority which will override any other end-user settings; when that isn't set things fall back to separate `LC_*` variables; and when those aren't set things fall back to `LANG`). [0] * Only set `LANG` for our child if necessary (if it already exists in the parent, then the child will inherit that, no need for us to do anything) [0] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02 Tested cases: - `unset LANG ; cargo run`: locale inside zed's terminal is set to `en_US.UTF-8` - `export LANG=en_GB.UTF-8 ; cargo run`: locale inside zed's terminal is set to `en_GB.UTF-8` Release Notes: - Use the system locale in the terminal instead of forcing `en_US.UTF-8`
Thank you for the explanation, that has connected some dots :) With that deeper understanding, and my misleading part-of-a-comment removed, I think this is good to go (the only way I can think of to be more-correct would be to look up the OS locale using platform-specific APIs and use that in place of hard-coded |
Let's give it a shot! |
terminal: Improve default locale handling * Use `LANG` instead of `LC_ALL` (`LC_ALL` is the highest priority which will override any other end-user settings; when that isn't set things fall back to separate `LC_*` variables; and when those aren't set things fall back to `LANG`). [0] * Only set `LANG` for our child if necessary (if it already exists in the parent, then the child will inherit that, no need for us to do anything) [0] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02 Tested cases: - `unset LANG ; cargo run`: locale inside zed's terminal is set to `en_US.UTF-8` - `export LANG=en_GB.UTF-8 ; cargo run`: locale inside zed's terminal is set to `en_GB.UTF-8` Release Notes: - Use the system locale in the terminal instead of forcing `en_US.UTF-8`
Looks like it's not working. But in zed terminal still: |
@josser are you launching zed by clicking the This PR fixes my problem (ie, it pays attention to the environment variables, when they exist) - however the original problem of "macos launches GUI apps without environment variables, so we need to guess or hardcode something as a fallback" remains |
terminal: Improve default locale handling
LANG
instead ofLC_ALL
(LC_ALL
is the highest priority which will override any other end-user settings; when that isn't set things fall back to separateLC_*
variables; and when those aren't set things fall back toLANG
). [0]LANG
for our child if necessary (if it already exists in the parent, then the child will inherit that, no need for us to do anything)[0] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02
Tested cases:
unset LANG ; cargo run
: locale inside zed's terminal is set toen_US.UTF-8
export LANG=en_GB.UTF-8 ; cargo run
: locale inside zed's terminal is set toen_GB.UTF-8
Release Notes:
en_US.UTF-8