Code review comment for lp://staging/~ds3/ubuntu/oneiric/synaptic/fix-for-63974

Revision history for this message
Colin Watson (cjwatson) wrote :

Thanks for your patch.

This will break translations. You can't assemble strings at run-time like this and have the result be translatable (because different languages will need to assemble the string in different orders); what you need to do instead is translate a format string and then substitute the package name into that. Awkwardly, RUserDialog::confirm only takes a single string rather than a format string plus arguments, and that looks like a pain to change, but you can just substitute it manually.

Annoyingly, vanilla C++ lacks a way to deal with format strings, aside from the C sprintf/snprintf functions which require deciding on a maximum size for the translated string (even if you dodge buffer overflow issues). I would suggest '#define _GNU_SOURCE 1' at the top of the file and then you can use asprintf. The result would then look something like this (untested):

  char *warning_fmt = _("Removing the %s package may render the "
                        "system usable.\n"
                        "Are you sure you want to do that?")
  char *warning;

  if (asprintf(&warning, warning_fmt, pkg->name()) < 0) {
     perror("asprintf");
     return;
  }
  bool confirmed = _userDialog->confirm(warning, false);
  free(warning);
  if (!confirmed)
     return;

(Not very C++ish, but hopefully Michael will have a look at this at some point and see if he can suggest anything neater!)

review: Needs Fixing

« Back to merge proposal