Skip to content

Conversation

ralfbrown
Copy link
Collaborator

Ref #19431

@ralfbrown ralfbrown added the bugfix pull request fixing a bug label Oct 4, 2025
@dterrahe
Copy link
Member

dterrahe commented Oct 4, 2025

The implementation is rather inefficient; building a list of strings and then concatenating that with dt_util_glist_to_str, which internally allocates an array of strings and uses g_strjoinv. Why not do that directly here? Like this:

gchar *dt_util_localize_segmented_name(const char *s,
                                       const gboolean with_space)
{
  if(!s || !*s)
    return g_strdup(s);

  const char *sep = with_space ? " | " : "|";

  if(g_str_has_prefix(s, BUILTIN_PREFIX))
   return g_strdup(_(s+sizeof(BUILTIN_PREFIX)-1));

  gchar **split = g_strsplit(s, "|", 0);
  gchar **loc_split = g_new0(char *, g_strv_length(split) + 1);

  for(int i = 0; split[i] != NULL; i++)
    loc_split[i] = (char *)dt_util_localize_string(g_strstrip(split[i]));
  gchar *localized = g_strjoinv(sep, loc_split);

  g_free(loc_split);
  g_strfreev(split);

  return localized;
}

This also checks up front if the string is NULL, which would be the only reason for g_strsplit to return NULL. Can't easily test with the reported issue unfortunately.

This also corrects handling of built-in presets. These are all marked with the BUILTIN_PREFIX and marked for translation as a whole. There is no support for translating them per-segment, so we should not fall back to trying to do this when no translation is found (which would likely be because we are using the English locale; no harm done, because we won't find any _l10n_ markers anyway, just a lot of wasted cycles).

EDIT: Ah, this now doesn't do the cleanup of irregular spacing around "|". So I guess we'll have to do the split-and-rejoin for builtins no matter what, even if a as-a-whole translation is found, just to support both "|" and " | " anyway, even if all original and translated strings did spacing consistently. We could skip dt_util_localize_string for built-ins or just count on it not finding any _l10n_ markers.

local_s was also translated twice...

@ralfbrown
Copy link
Collaborator Author

local_s was also translated twice...

Missed that - it's easy to miss the _
The fix for that would be to omit it from the line which assigns local_s, which then should probably be renamed since it isn't localized. Cascading changes....

@dterrahe
Copy link
Member

dterrahe commented Oct 4, 2025

Since we have two alternative paths, _builtins_ with early translation and then split/whitespace removal/reconstitution, and non-builtins with split/whitespace removal/late potential translation/reconstitution, its probably better to use less-descriptive names. I've used t in the below. This does everything we want, except possibly removing leading/trailing spaces that a translator might have inserted into _l10n_ marked segment strings.

gchar *dt_util_localize_segmented_name(const char *s,
                                       const gboolean with_space)
{
  if(!s || !*s)
    return g_strdup(s);

  const char *sep = with_space ? " | " : "|";
  const gboolean is_builtin = g_str_has_prefix(s, BUILTIN_PREFIX);
  const char *t = is_builtin ? _(s+sizeof(BUILTIN_PREFIX)-1) : s;

  gchar **split = g_strsplit(t, "|", 0);
  gchar **loc_split = g_new0(char *, g_strv_length(split) + 1);

  for(int i = 0; split[i] != NULL; i++)
  {
    loc_split[i] = is_builtin ? g_strstrip(split[i])
                 : (char *)dt_util_localize_string(g_strstrip(split[i]));
  }

  gchar *localized = g_strjoinv(sep, loc_split);

  g_free(loc_split);
  g_strfreev(split);

  return localized;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants