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

Move UTF-8 setting logic to scr.utf8 config callback on Windows #15273

Merged
merged 4 commits into from
Oct 16, 2019

Conversation

GustavoLCR
Copy link
Contributor

This should fix tests on Appveyor

@radare
Copy link
Collaborator

radare commented Oct 15, 2019

uhm. why that? isnt the env properly set when cons_init is called? i would prefer to have this code in rcons

@GustavoLCR
Copy link
Contributor Author

uhm. why that? isnt the env properly set when cons_init is called? i would prefer to have this code in rcons

@radare the problem is that if utf8 was available at all, r2 would always change the active codepage to it (even with scr.utf8=0), affecting the other tools output.

With this it will only automatically set scr.utf8=1 if utf8 codepage is already active when the tools launches, and if the user sets scr.utf8 r2 will change the codepage accordingly.

@XVilka
Copy link
Contributor

XVilka commented Oct 15, 2019

I agree this approach is better. LGTM.

@XVilka XVilka added this to the 4.0.0 milestone Oct 15, 2019
@XVilka XVilka added consoleui Windows Microsoft Windows platform support issues labels Oct 15, 2019
@radare
Copy link
Collaborator

radare commented Oct 15, 2019

despite the thing works as you expect, i dont like having code that checks for console stuff in core, when this was already in cons, which was the right place

@@ -822,6 +822,7 @@ R_API int r_cons_w32_print(const ut8 *ptr, int len, bool vmode);
R_API int r_cons_win_printf(bool vmode, const char *fmt, ...);
R_API int r_cons_win_eprintf(bool vmode, const char *fmt, ...);
R_API int r_cons_win_vhprintf(DWORD hdl, bool vmode, const char *fmt, va_list ap);
R_API void r_cons_win_set_cp(bool utf8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

just wondering.. is there any reason why the win api is separated instead of being abstracted into a generic functionality for all platforms? .. something to think about and do in a separate pr

@@ -2273,6 +2273,9 @@ static bool cb_utf8(void *user, void *data) {
RCore *core = (RCore *) user;
RConfigNode *node = (RConfigNode *) data;
core->cons->use_utf8 = node->i_value;
#if __WINDOWS__
r_cons_win_set_cp (core->cons->use_utf8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about naming it r_cons_set_utf8(), so using the setter/getter pattern instead of setting the use_utf8 variable by hand

Copy link
Collaborator

@radare radare left a comment

Choose a reason for hiding this comment

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

.

@radare radare merged commit 5017497 into radareorg:master Oct 16, 2019
@GustavoLCR GustavoLCR deleted the fix-utf8-win branch October 24, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consoleui Windows Microsoft Windows platform support issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants