Re: [gedit-list] i18n fixes for "search and replace" in gedit



Thank you for the patch, it looks good but I have a couple of comments

*  It lacks ChangeLog entries, please add them


>         if (settings->font == NULL)
>                 settings->font = g_strdup (DEFAULT_FONT);
> +       if (strchr(settings->font, ',') >= 0) 
> +               settings->use_fontset = TRUE;
> +       else
> +               settings->use_fontset = FALSE;
>         

This is a "hacky" way to determine if we are using a fontset.
I prefere something like :

if (font->type == GDK_FONT_FONTSET) 
...


> +               int len_s = mblen(src, MB_CUR_MAX);
> +               int len_d;

what is len_s and len_d ? We use longer variables in gedit, even if it
means
that we need to do a little more typing but makes the code easy to
remember what
it did


This : 

> +               if (len_s == len_d && (len_s == 1 || 
> +                   ((case_sensitive?strncmp(src, dest, len_s):g_strncasecmp(src, dest, len_s)) == 0))) 

and :

> +               if ((buffer_in[p1] == search_text[p3]) || (buffer_in[p1] < 0x80 && (buffer_in [p1]|case_sensitive_mask) == (search_text [p3]|case_sensitive_mask)))

are getting a little cryptic. How about if we write a couple of inline
functions
something like :

gedit_search_compare_charcter (const gchar *in_1, const gchar *in_2,
gint case_sensitive_mask)

>                         if (p3 == search_text_length)



> +char *
> +mb_index(char *s, int pos)
> +{
> +    while (pos--) {
> +       int i = mblen(s, MB_CUR_MAX);
> +       if (i == 0) return s;
> +       if (i < 0) s++;
> +       else s += i;
> +    }
> +    return s;
> +}

all the functions in gedit have a prefix, can we find a good name
for this functions ? Again, even if it means more typing but it 
has a prefix and they are clear on what they do

gedit_util_string_index 
gedit_util_strlen 


also please follow the current gedit style.
- we use gchar/gint not chars/int
- the indentation is different than the rest of the code



> +
> +int
> +mb_strlen(const char *s)
> +{
> +    int i;
> +    int len = 0;
> +    while (*s) {
> +       i = mblen(s, MB_CUR_MAX);
> +       if (i <= 0) i = 1;
> +       s += i;
> +       len++;
> +    }
> +    return len;
> +}
> +

same comments here.


Every thing else loooks fine, thank you very much

regards,
Chema




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]