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.

Sign up for GitHub

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

Jump to bottom

feat(native): add Locale module #938

Merged
merged 11 commits into from
Jul 3, 2020
Merged

feat(native): add Locale module #938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fda7ded
Add locale bindings on macOS
zbaylin Jul 3, 2020
2d3ba98
Merge branch 'master' of https://github.com/revery-ui/revery into fea…
zbaylin Jul 3, 2020
510b807
Add Locale implementation on Linux
zbaylin Jul 3, 2020
72d9e2f
Add locale implementation on Windows
zbaylin Jul 3, 2020
40c72ec
Add locale to Environment
zbaylin Jul 3, 2020
1f0efe3
Add a CAMLlocal to return
zbaylin Jul 3, 2020
26da73b
Add missing newline to format script
zbaylin Jul 3, 2020
49cf346
Free Windows pointer
zbaylin Jul 3, 2020
f07d3fc
Add test
zbaylin Jul 3, 2020
2eed6b0
Fix tests
zbaylin Jul 3, 2020
eedb70b
Fix freeing based on platform
zbaylin Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Native/locale.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@ CAMLprim value revery_getUserLocale() {
char *ret;
#ifdef __APPLE__
ret = revery_getUserLocale_cocoa();
camlRet = caml_copy_string(ret);
#elif WIN32
ret = revery_getUserLocale_win32();
camlRet = caml_copy_string(ret);
#else
setlocale(LC_CTYPE, "");
ret = setlocale(LC_CTYPE, NULL);
#endif
camlRet = caml_copy_string(ret);
free(ret);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like it's hitting the Windows path - just linux.

I guess we should check:

  1. Does ret need to be freed for Apple?
  2. Does ret need to be freed for Windows? I think the answer is yes
  3. Does ret need to be freed for Linux?

If the answer is to all of those, we could free(ret) after the #ifdef/#endif block. Otherwise, if we just need to free some of them - we'd need to include it in the platform condition

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that was my bad. I fixed it in eedb70b. On Linux and macOS we dont need to free (in fact we segfault if we do, predictably). I added a shouldFree variable that I think makes it a bit clearer. Let me know if you think this is better!

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me! Thanks @zbaylin

#endif
CAMLreturn(camlRet);
}
11 changes: 8 additions & 3 deletions test/Core/EnvironmentTests.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ open Revery_Core;

open TestFramework;

describe("Environment", ({test, _}) =>
describe("Environment", ({test, _}) => {
test("executingDirectory", _ =>
test(
"validate we can load a file adjacent to the executable", ({expect, _}) => {
Expand All @@ -11,5 +11,10 @@ describe("Environment", ({test, _}) =>
).
toBeTrue()
})
)
);
);
test("userLocale", _ =>
test("locale is not empty", ({expect, _}) => {
expect.notSame(Environment.userLocale, "")
})
);
});