Skip to content
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

Set $LANG env var using languageCode, countryCode #1087

Merged
merged 4 commits into from
May 14, 2018

Conversation

sodiumjoe
Copy link
Contributor

@sodiumjoe sodiumjoe commented Feb 3, 2018

src/locale.rs Outdated
env::set_var("LANG", &locale_id);
env::set_var("LC_CTYPE", &locale_id);
// env::set_var("LC_CTYPE", &locale_id);
Copy link
Contributor

@nixpulvis nixpulvis Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This something that should be deleted, seemed a bit unclear from the dicsussion of #993 if it should be set or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried both, and it didn't seem to make a difference. iTerm doesn't appear to set it, so I'm opting to leave it out, but I'd be happy to put it back in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a Mac user anymore, so I can't really weigh in here, but copying iTerm seems reasonable.

@sodiumjoe
Copy link
Contributor Author

ping

@sodiumjoe
Copy link
Contributor Author

@jwilm anything else I need to do to get this merged?

@chrisduerr
Copy link
Member

@sodiumjoe Probably mostly a matter of time. There are a few PRs that are good to go but just waiting for approval.

@sodiumjoe
Copy link
Contributor Author

@chrisduerr ok, sounds good, thanks!

@sodiumjoe
Copy link
Contributor Author

fixes #1203

@jussi-kalliokoski
Copy link

Works for me, LC_TYPE is now correctly set to en_US.UTF-8 for me instead of en_FI.UTF-8 that — while accurate — is probably not a recognized locale code. :)

@jwilm
Copy link
Contributor

jwilm commented May 5, 2018

Went to try this out today and discovered that it doesn't run on macOS 10.11 (El Capitan). The problem seems to be that at least [NSLocale languageCode] didn't exist until 10.12.

@sodiumjoe
Copy link
Contributor Author

@jwilm ok I'm learning piecemeal why each part of the iterm code for this exists. I guess that's what this else block handles: https://github.com/gnachman/iTerm2/blob/79aff4d59fd591e7628649bcabe5f27541740bf6/sources/PTYSession.m#L7099

I can try to add that into this branch, but I'm going to have a hard time testing, since I don't have a machine with <10.12 on it.

What exactly happens when you run this on El Capitan?

@sodiumjoe
Copy link
Contributor Author

It looks like even localeIdentifier is 10.4+.

Does alacritty have an official set of OS versions it supports?

@chrisduerr
Copy link
Member

I think there never was any official decision from jwilm which versions are supported. Until now the issue just didn't come up where people had issues running it on old versions.

@sodiumjoe
Copy link
Contributor Author

@chrisduerr ok, for now I'll work on porting the conditional code from iterm2, I realized I have an older imac I can test on.

@jwilm
Copy link
Contributor

jwilm commented May 7, 2018

@sodiumjoe we don't have an official macOS version we are aiming to support, but the unofficial minimum version is the one running on my laptop 😉 (El Capitan).

The observed behavior is simply a segfault and message from the Objective C runtime about the languageCode selector not existing.

@sodiumjoe
Copy link
Contributor Author

@jwilm so what you're saying is I just need to upgrade your laptop 😂

Thanks for that info, I'll try to get to that tonight.

@chrisduerr
Copy link
Member

That's one way of solving it. :D

@sodiumjoe
Copy link
Contributor Author

@jwilm Added logic to fallback to localeIdentifier if the newer selectors are not available.

@jwilm
Copy link
Contributor

jwilm commented May 14, 2018

@sodiumjoe this looks great; thanks for making the modifications to fall back to localeIdentifier!

One last thing -- there are conflicts with master which prevents this from being merged. Would you mind rebasing?

@sodiumjoe
Copy link
Contributor Author

@jwilm np, is it acceptable practice to rebase and git push -f to this branch?

@jwilm
Copy link
Contributor

jwilm commented May 14, 2018

@sodiumjoe yes, that would even be ideal.

@sodiumjoe
Copy link
Contributor Author

@jwilm done

@jwilm
Copy link
Contributor

jwilm commented May 14, 2018

great; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

LC_CTYPE set to some strange (invalid?) value in alacritty
5 participants