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

Use a format string instead of repeating almost the same string 9 times #1058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b4n
Copy link
Member

@b4n b4n commented Jun 8, 2016

This simplifies translations by being less redundant.

Note: label variable is not freed because keybindings_set_item() currently assumes static strings but for plugins, so we need not free the label right away. It's not a real problem though, as this string
is required during the whole process' lifetime, so it'd use just as much memory anyway.

Closes #953.


I'm not certain this is a great way to fix, but it works. Opinions welcome.

This simplifies translations by being less redundant.

Note: label variable is not freed because `keybindings_set_item()`
currently assumes static strings but for plugins, so we need not free
the label right away.  It's not a real problem though, as this string
is required during the whole process' lifetime, so it'd use just as
much memory anyway.

Closes geany#953.
@codebrainz
Copy link
Member

Another way might be like:

#define ADD_KB_CUSTOM_COMMAND(n, key, mod) \
    add_kb(group, GEANY_KEYS_FORMAT_SENTOCMD##n, NULL, key, mod, \
        "edit_sentocmd"#n, _("Send to Custom Command "#n), NULL)

Not sure if that works for gettext scanning or translations though. Alternatively, we could use g_intern_string() which will copy the given string, then we can free our formatted one, to avoid "leaking" it like:

#define ADD_KB_CUSTOM_COMMAND(n, key, mod) \
    do { \
        gchar *label = g_strdup_printf(_("Send to Custom Command %d"), n); \
        add_kb(group, GEANY_KEYS_FORMAT_SENDTOCMD##n, NULL, key, mod, \
                "edit_sendtocmd"#n, g_intern_string(label), NULL); \
        g_free(label); \
    } while(0)

Or just leak them...

@kugel-
Copy link
Member

kugel- commented Jul 6, 2016

Not sure it's a worthwhile improvement. It replaces one line with another (although shorter) one but adds the macro. plus having to malloc.

@b4n
Copy link
Member Author

b4n commented Jul 6, 2016

Another way might be like:

#define ADD_KB_CUSTOM_COMMAND(n, key, mod) \
    add_kb(group, GEANY_KEYS_FORMAT_SENTOCMD##n, NULL, key, mod, \
        "edit_sentocmd"#n, _("Send to Custom Command "#n), NULL)

That's not acceptable, because translators need to be able to move the number if needed.

Alternatively, we could use g_intern_string() which will copy the given string, then we can free our formatted one, to avoid "leaking" it

Mmyeah, not really different, but indeed.

Not sure it's a worthwhile improvement. It replaces one line with another (although shorter) one but adds the macro. plus having to malloc.

Me neither. It actually looks kinda ugly to me, that's why I PRd it for opinions. The advantage is that ther's then only 1 translation instead of 9, but the code here isn't very nice, and the translations now already exist, so…

@b4n b4n added the i18n Internationalization (translations, localization, …) label Jul 6, 2016
@codebrainz
Copy link
Member

👎 for the macro :)

@ntrel
Copy link
Member

ntrel commented Apr 12, 2017

I don't think we should leak memory without holding a static pointer root to it, otherwise Valgrind will report it as definitely lost (rather than possibly lost). So if we want this then either g_intern_string or this:

    static const gchar *labels[10];
    int n;

    for (n = 1; n != 10; n++)
        labels[n] = g_strdup_printf(_("Send to Custom Command %d"), n);

    ADD_KB_CUSTOM_COMMAND(1, GDK_1, GEANY_PRIMARY_MOD_MASK);
    ...

I think the macro is not so bad if it only wraps add_kb and allocation is done outside it.
(It's a shame there's no g_string_chunk_insert_printf).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Internationalization (translations, localization, …)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translation stockade in "Send to Custom Command ..."
4 participants