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

Handle utf8 nicks with mk_wcwidth() #480

Merged
merged 14 commits into from May 18, 2016

Conversation

Projects
None yet
2 participants
@xavierog
Contributor

xavierog commented May 13, 2016

Following pull request #471, I created a whole new branch in my fork (hence that separate pull request) which, I believe, reflects ailin-nemui's advices. Feel free to review, comment and/or integrate these changes.

/* Provide is_utf8(): */
#include "recode.h"
int string_advance(char const **str, int policy)

This comment has been minimized.

@ailin-nemui

ailin-nemui May 13, 2016

Contributor

can you convert the policy to enum?

This comment has been minimized.

@xavierog

xavierog May 13, 2016

Contributor

Sure, no problem :)
Edit: actually, could you clarify what you wish? Converting #define to enum is okay but should I also alter the functions prototypes so that they do not show "int policy" anymore?

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented May 13, 2016

thank you very much for your continued effort on this, I'm looking forward to integrating this into irssi.

mhm, it looks like we don't need the characters count and probably never will. (my suggestion was in error) how would you like renaming string_chars_for_width and make it return the bytes instead?

I'm a bit ambivalent about the policy-autodetection with -1 (tending to dislike). anyone else got an opinion on that? @dequis ?

int string_policy(const char *str)
{
if (is_utf8()) {
if (!str || g_utf8_validate(str, -1, NULL)) {

This comment has been minimized.

@ailin-nemui

ailin-nemui May 13, 2016

Contributor

in irssi code, please always use explicit str == NULL

This comment has been minimized.

@xavierog

xavierog May 13, 2016

Contributor

Sure, no problem :)

g_return_val_if_fail(str != NULL, -1);
/* Handle the dummy case where n is 0: */
if (!n) {

This comment has been minimized.

@ailin-nemui

ailin-nemui May 13, 2016

Contributor

write n == 0

This comment has been minimized.

@xavierog

xavierog May 13, 2016

Contributor

Sure, no problem :)

/* Handle the dummy case where n is 0: */
if (!n) {
if (bytes) {

This comment has been minimized.

@ailin-nemui

ailin-nemui May 13, 2016

Contributor

bytes != NULL

This comment has been minimized.

@xavierog

xavierog May 13, 2016

Contributor

Sure, no problem :)

@xavierog

This comment has been minimized.

Contributor

xavierog commented May 13, 2016

mhm, it looks like we don't need the characters count and probably never will. (my suggestion was in error) how would you like renaming string_chars_for_width and make it return the bytes instead?

Actually, the function returns the amount of characters and optionally provides the number of bytes. It turns out that my former pull request needed characters whereas my new pull request needs bytes; therefore, it does not seem unlikely to me that the number of characters could be needed in the future (never say never...). Plus, "he who can do more can do less": it does not hurt to have extra features like this in that kind of toolbox (because this is what we are doing here: glib and libc functions are not sufficient for us, so we are building our own little toolbox on top of them); for instance: have you noticed that string_length() is absolutely unused as of now? :)

I'm a bit ambivalent about the policy-autodetection with -1 (tending to dislike).

I implemented that approach because, when writing the code, it seemed obvious to me that, without this mechanism, we would just end up writing a lot of "foobar(str, string_policy(str))" everywhere in the code and possibly regret it at some point as it goes against one of the most fundamental principle of programming: DRY -- Don't Repeat Yourself. How come you disapprove of it?

@ailin-nemui ailin-nemui merged commit 74d3868 into irssi:master May 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

fix dist compilation failure
remove illegal wcwidth.c include and compile wcwidth.c
fallout from irssi#480

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

fix dist compilation failure
remove illegal wcwidth.c include and compile wcwidth.c
correct #include in wcwidth.c
fallout from irssi#480

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

fix dist compilation failure
remove illegal wcwidth.c include and compile wcwidth.c
correct #include in wcwidth.c
fallout from irssi#480

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

fix dist compilation failure
remove illegal wcwidth.c include and compile wcwidth.c
correct #include in wcwidth.c
fallout from irssi#480

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request May 18, 2016

fix dist compilation failure
remove illegal wcwidth.c include and compile wcwidth.c
correct #include in wcwidth.c
fallout from irssi#480

@ailin-nemui ailin-nemui referenced this pull request May 18, 2016

Closed

Handle utf8 nicks #471

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