Re: [PATCH] proper indentation in merge conflict menu

On Thu, Jun 05, 2008 at 06:17:17PM -0400, Karl Fogel wrote:
> Whoa, whoa, whoa. Let's slow down and think about this carefully.
>
> I'm not sure r31600 is a generally applicable solution. After all, a
> translation might be longer instead of shorter.
>
> Instead, perhaps we have to write multiline prompts in a way that allows
> translations to work regardless of word length. I don't think counting
> characters is the way to go, unless there's no other solution.

Regardless of whether or not we're going to count characters,
here's a diff that does the counting properly, just to illustrate
how this could be done.

I *really* hope someone has a better idea. The amount of hoops
this patch has to jump through is a bit crazy. Maybe there's also
a way of doing this without wide chars, but I don't know how.

[[[

Fix indentation for translations of the interactive merge conflict
resolution menu.

* subversion/svn/conflict-callbacks.c
  (calc_prompt_indentation): New function.
  (svn_cl__conflict_handler): Try to indent the multi-line menu
   properly, regardless of the active language.

* configure.ac: Check for wide char support.

Found by: jensseidel

]]]

Index: subversion/svn/conflict-callbacks.c
===================================================================
--- subversion/svn/conflict-callbacks.c (revision 31609)
+++ subversion/svn/conflict-callbacks.c (working copy)
@@ -35,6 +35,10 @@
 
 #include "svn_private_config.h"
 
+#ifdef HAVE_WCHAR_H
+#include <wchar.h>
+#endif
+
 
 
 
@@ -224,7 +228,37 @@ launch_resolver(svn_boolean_t *performed_edit,
   return SVN_NO_ERROR;
 }
 
+/* Try to calculate the right amount of whitespace characters
+ * needed to indent up to the length of the string PROMPT.
+ * The right amount depends on the current language.
+ */
+static int
+calc_prompt_indentation(const char *prompt, apr_pool_t *pool)
+{
+ int len = strlen(prompt); /* Default to byte count. */
 
+#if defined(HAVE_MBSTOWCS) && defined(HAVE_WCSWIDTH)
+ /* We have wide chars, so try to be safe for multi-byte characters. */
+ int num_wchars = mbstowcs(NULL, prompt, 0);
+ if (num_wchars > 0)
+ {
+ /* From http://www.cl.cam.ac.uk/~mgk25/unicode.html:
+ * With UTF-8, neither a byte nor a character count will predict
+ * the display width, because ideographic characters (Chinese,
+ * Japanese, Korean) will occupy two column positions, whereas
+ * control and combining characters occupy none.
+ * To determine the width of a string on the terminal screen,
+ * it is necessary to decode the UTF-8 sequence and then use
+ * [...] wcswidth to measure the entire string. */
+ wchar_t *wprompt = apr_palloc(pool, sizeof(wchar_t) * (num_wchars + 1));
+ if (mbstowcs(wprompt, prompt, num_wchars) > 0)
+ len = wcswidth(wprompt, num_wchars);
+ }
+#endif
+
+ return len;
+}
+
 /* Implement svn_wc_conflict_resolver_func_t; resolves based on
    --accept option if given, else by prompting. */
 svn_error_t *
@@ -364,6 +398,10 @@ svn_cl__conflict_handler(svn_wc_conflict_result_t
   {
       const char *answer;
       char *prompt;
+ char *select = _("Select: ");
+ svn_stringbuf_t *whitespace;
+ apr_size_t indent_len;
+ int i;
       svn_boolean_t diff_allowed = FALSE;
       /* Have they done something that might have affected the merged
          file (so that we need to save a .edited copy)? */
@@ -423,11 +461,18 @@ svn_cl__conflict_handler(svn_wc_conflict_result_t
           || (!desc->base_file && desc->my_file && desc->their_file))
         diff_allowed = TRUE;
 
+ /* Create suitable amount of whitespace for prompt indentation. */
+ indent_len = calc_prompt_indentation(select, subpool);
+ whitespace = svn_stringbuf_create_ensure(indent_len + 1, pool);
+ for (i = 0; i < indent_len; i++)
+ svn_stringbuf_appendcstr(whitespace, " ");
+
       while (TRUE)
         {
           svn_pool_clear(subpool);
 
- prompt = apr_pstrdup(subpool, _("Select: (p) postpone"));
+ prompt = apr_pstrdup(subpool, select);
+ prompt = apr_pstrcat(subpool, prompt, _("(p) postpone"), NULL);
 
           if (diff_allowed)
             {
@@ -442,7 +487,8 @@ svn_cl__conflict_handler(svn_wc_conflict_result_t
               if (! desc->is_binary &&
                   desc->kind != svn_wc_conflict_kind_property)
                 prompt = apr_pstrcat(subpool, prompt,
- _(",\n (mc) mine-conflict, "
+ ",\n", whitespace->data,
+ _("(mc) mine-conflict, "
                                        "(tc) theirs-conflict"),
                                      NULL);
             }
@@ -451,13 +497,12 @@ svn_cl__conflict_handler(svn_wc_conflict_result_t
               if (knows_something)
                 prompt = apr_pstrcat(subpool, prompt, _(", (r) resolved"),
                                      NULL);
- prompt = apr_pstrcat(subpool, prompt,
- _(",\n "
- "(mf) mine-full, (tf) theirs-full"),
+ prompt = apr_pstrcat(subpool, prompt, ",\n", whitespace->data,
+ _("(mf) mine-full, (tf) theirs-full"),
                                    NULL);
             }
 
- prompt = apr_pstrcat(subpool, prompt, ",\n ", NULL);
+ prompt = apr_pstrcat(subpool, prompt, ",\n", whitespace->data, NULL);
           prompt = apr_pstrcat(subpool, prompt,
                                _("(s) show all options: "),
                                NULL);
Index: configure.ac
===================================================================
--- configure.ac (revision 31609)
+++ configure.ac (working copy)
@@ -610,7 +610,10 @@ AC_FUNC_VPRINTF
 dnl check for functions needed in special file handling
 AC_CHECK_FUNCS(symlink readlink)
 
-
+dnl Check for wide character support functions
+AC_CHECK_HEADERS(wchar.h)
+AC_CHECK_FUNCS(mbstowcs wcswidth)
+
 dnl Process some configuration options ----------
 
 AC_ARG_WITH(ssl,

So watcha think?

Stefan