[gtk/label-mnemonic-fix] label: Fix markup parsing interactions

[Matthias Clasen <matthiasc src gnome org>]

commit 0c8d75ae12a76ec8e0e458ffdc6704bf9a0a8ae3
Author: Matthias Clasen <mclasen redhat com>
Date:   Tue Mar 2 22:30:11 2021 -0500

    label: Fix markup parsing interactions
    
    We were not handling mnemonics vs markup right
    in all cases. Rewrite the _-stripping code to
    do it during the link parsing, instead of as
    a separate function. This avoids the issue of
    stripping _ from attribute names in markup.
    
    Add tests.
    
    Fixes: 3706

 gtk/gtklabel.c            | 235 ++++++++++++++++++----------------------------
 testsuite/gtk/label.c     | 207 ++++++++++++++++++++++++++++++++++++++++
 testsuite/gtk/meson.build |   1 +
 3 files changed, 301 insertions(+), 142 deletions(-)
---
diff --git a/gtk/gtklabel.c b/gtk/gtklabel.c
index e6ca3a631c..74e7360971 100644
--- a/gtk/gtklabel.c
+++ b/gtk/gtklabel.c
@@ -3078,6 +3078,8 @@ typedef struct
   GArray *links;
   GString *new_str;
   gsize text_len;
+  gboolean strip_ulines;
+  GString *text_data;
 } UriParserData;
 
 static void
@@ -3206,6 +3208,41 @@ start_element_handler (GMarkupParseContext  *context,
     }
 }
 
+static char *
+strip_ulines (const char *text)
+{
+  char *new_text;
+  const char *p;
+  char *q;
+  gboolean after_uline = FALSE;
+
+  new_text = malloc (strlen (text) + 1);
+
+  q = new_text;
+  for (p = text; *p; p++)
+    {
+      if (*p == '_' && !after_uline)
+        {
+          after_uline = TRUE;
+          continue;
+        }
+
+      *q = *p;
+      q++;
+      after_uline = FALSE;
+    }
+
+  if (after_uline)
+    {
+      *q = '_';
+      q++;
+    }
+
+  *q = '\0';
+
+  return new_text;
+}
+
 static void
 end_element_handler (GMarkupParseContext  *context,
                      const char           *element_name,
@@ -3214,6 +3251,34 @@ end_element_handler (GMarkupParseContext  *context,
 {
   UriParserData *pdata = user_data;
 
+  if (pdata->text_data->len > 0)
+    {
+      char *text;
+      gsize text_len;
+      char *newtext;
+
+      if (pdata->strip_ulines && strchr (pdata->text_data->str, '_'))
+        {
+          text = strip_ulines (pdata->text_data->str);
+          text_len = strlen (text);
+        }
+      else
+        {
+          text = pdata->text_data->str;
+          text_len = pdata->text_data->len;
+        }
+
+      newtext = g_markup_escape_text (text, text_len);
+      g_string_append (pdata->new_str, newtext);
+      pdata->text_len += text_len;
+      g_free (newtext);
+
+      if (text != pdata->text_data->str)
+        g_free (text);
+
+      g_string_set_size (pdata->text_data, 0);
+    }
+
   if (!strcmp (element_name, "a"))
     {
       GtkLabelLink *link = &g_array_index (pdata->links, GtkLabelLink, pdata->links->len - 1);
@@ -3235,12 +3300,8 @@ text_handler (GMarkupParseContext  *context,
               GError              **error)
 {
   UriParserData *pdata = user_data;
-  char *newtext;
 
-  newtext = g_markup_escape_text (text, text_len);
-  g_string_append (pdata->new_str, newtext);
-  pdata->text_len += text_len;
-  g_free (newtext);
+  g_string_append_len (pdata->text_data, text, text_len);
 }
 
 static const GMarkupParser markup_parser =
@@ -3261,8 +3322,9 @@ xml_isspace (char c)
 static gboolean
 parse_uri_markup (GtkLabel      *self,
                   const char    *str,
+                  gboolean       strip_ulines,
                   char         **new_str,
-                  GtkLabelLink  **links,
+                  GtkLabelLink **links,
                   guint         *out_n_links,
                   GError       **error)
 {
@@ -3279,6 +3341,8 @@ parse_uri_markup (GtkLabel      *self,
   pdata.links = NULL;
   pdata.new_str = g_string_sized_new (length);
   pdata.text_len = 0;
+  pdata.strip_ulines = strip_ulines;
+  pdata.text_data = g_string_new ("");
 
   while (p != end && xml_isspace (*p))
     p++;
@@ -3307,6 +3371,8 @@ parse_uri_markup (GtkLabel      *self,
 
   g_markup_parse_context_free (context);
 
+  g_string_free (pdata.text_data, TRUE);
+
   *new_str = g_string_free (pdata.new_str, FALSE);
 
   if (pdata.links)
@@ -3351,83 +3417,10 @@ gtk_label_ensure_has_tooltip (GtkLabel *self)
   gtk_widget_set_has_tooltip (GTK_WIDGET (self), has_tooltip);
 }
 
-/* Reads @text and extracts the accel key, if any.
- * @new_text will be set to the given text with the first _ removed.
- *
- * Returned will be the one underline attribute used for the mnemonic
- * */
 static void
-extract_mnemonic_keyval (const char      *text,
-                         guint           *out_accel_key,
-                         char           **out_new_text,
-                         PangoAttribute **out_mnemonic_attribute)
-{
-  const gsize text_len = strlen (text);
-  gunichar c;
-  const char *p;
-
-  p = text;
-  for (;;)
-    {
-      const char *_index;
-
-      c = g_utf8_get_char (p);
-
-      if (c == '\0')
-        break;
-
-      if (c != '_')
-        {
-          p = g_utf8_next_char (p);
-          continue;
-        }
-
-      _index = p;
-
-      p = g_utf8_next_char (p);
-      c = g_utf8_get_char (p);
-
-      if (c != '_' && c != '\0')
-        {
-          const gsize byte_index = p - text - 1; /* Of the _ */
-
-          /* c is the accel key */
-          if (out_accel_key)
-            *out_accel_key = gdk_keyval_to_lower (gdk_unicode_to_keyval (c));
-          if (out_new_text)
-            {
-              *out_new_text = g_malloc (text_len);
-              memcpy (*out_new_text, text, byte_index);
-              memcpy (*out_new_text + byte_index, p, text_len - byte_index);
-            }
-
-          if (out_mnemonic_attribute)
-            {
-              PangoAttribute *attr = pango_attr_underline_new (PANGO_UNDERLINE_LOW);
-              attr->start_index = _index - text;
-              attr->end_index = p - text;
-              *out_mnemonic_attribute = attr;
-            }
-
-          return;
-        }
-
-      p = g_utf8_next_char (p);
-    }
-
-  /* No accel key found */
-  if (out_accel_key)
-    *out_accel_key = GDK_KEY_VoidSymbol;
-  if (out_new_text)
-    *out_new_text = NULL;
-  if (out_mnemonic_attribute)
-    *out_mnemonic_attribute = NULL;
-}
-
-static void
-gtk_label_set_markup_internal (GtkLabel    *self,
+gtk_label_set_markup_internal (GtkLabel   *self,
                                const char *str,
-                               gboolean     with_uline)
+                               gboolean    with_uline)
 {
   char *text = NULL;
   GError *error = NULL;
@@ -3435,9 +3428,18 @@ gtk_label_set_markup_internal (GtkLabel    *self,
   char *str_for_display = NULL;
   GtkLabelLink *links = NULL;
   guint n_links = 0;
-  PangoAttribute *mnemonic_attr = NULL;
-
-  if (!parse_uri_markup (self, str, &str_for_display, &links, &n_links, &error))
+  guint accel_keyval = 0;
+  gboolean do_mnemonics;
+
+  do_mnemonics = self->mnemonics_visible &&
+                 gtk_widget_is_sensitive (GTK_WIDGET (self)) &&
+                 (!self->mnemonic_widget || gtk_widget_is_sensitive (self->mnemonic_widget));
+
+  if (!parse_uri_markup (self, str,
+                         with_uline && !do_mnemonics,
+                         &str_for_display,
+                         &links, &n_links,
+                         &error))
     goto error_set;
 
   if (links)
@@ -3449,62 +3451,12 @@ gtk_label_set_markup_internal (GtkLabel    *self,
       gtk_widget_add_css_class (GTK_WIDGET (self), "link");
     }
 
-  if (!with_uline)
-   {
-no_uline:
-      /* Extract the text to display */
-      if (!pango_parse_markup (str_for_display, -1, 0, &attrs, &text, NULL, &error))
-        goto error_set;
-   }
-  else /* Underline AND markup is a little more complicated... */
-   {
-      char *new_text = NULL;
-      guint accel_keyval;
-      gboolean auto_mnemonics = TRUE;
-      gboolean do_mnemonics = self->mnemonics_visible &&
-                              (!auto_mnemonics || gtk_widget_is_sensitive (GTK_WIDGET (self))) &&
-                              (!self->mnemonic_widget || gtk_widget_is_sensitive (self->mnemonic_widget));
-
-      /* Remove the mnemonic underline */
-      extract_mnemonic_keyval (str_for_display,
-                               &accel_keyval,
-                               &new_text,
-                               NULL);
-      if (!new_text) /* No underline found anyway */
-        goto no_uline;
-
-      self->mnemonic_keyval = accel_keyval;
-
-      /* Extract the text to display */
-      if (!pango_parse_markup (new_text, -1, '_',
-                               do_mnemonics ? &attrs : NULL, &text, NULL, &error))
-        {
-          g_free (new_text);
-          goto error_set;
-        }
-
-      if (do_mnemonics)
-        {
-          /* text is now the final text, but we need to parse str_for_display once again
-           * *with* the mnemonic underline so we can remove the markup tags and get the
-           * proper attribute indices */
-          char *text_for_accel;
-
-          if (!pango_parse_markup (str_for_display, -1, 0, NULL, &text_for_accel, NULL, &error))
-            {
-              g_free (new_text);
-              goto error_set;
-            }
-
-          extract_mnemonic_keyval (text_for_accel,
-                                   NULL,
-                                   NULL,
-                                   &mnemonic_attr);
-          g_free (text_for_accel);
-        }
-
-      g_free (new_text);
-   }
+  if (!pango_parse_markup (str_for_display, -1,
+                           with_uline && do_mnemonics ? '_' : 0,
+                           &attrs, &text,
+                           with_uline && do_mnemonics ? &accel_keyval : NULL,
+                           &error))
+    goto error_set;
 
   g_free (str_for_display);
 
@@ -3514,8 +3466,7 @@ no_uline:
   g_clear_pointer (&self->markup_attrs, pango_attr_list_unref);
   self->markup_attrs = attrs;
 
-  if (mnemonic_attr)
-    pango_attr_list_insert_before (self->markup_attrs, mnemonic_attr);
+  self->mnemonic_keyval = accel_keyval;
 
   return;
 
diff --git a/testsuite/gtk/label.c b/testsuite/gtk/label.c
new file mode 100644
index 0000000000..11f1eebf23
--- /dev/null
+++ b/testsuite/gtk/label.c
@@ -0,0 +1,207 @@
+#include <gtk/gtk.h>
+
+void
+print_attribute (PangoAttribute *attr, GString *string)
+{
+  GEnumClass *class;
+  GEnumValue *value;
+
+  g_string_append_printf (string, "[%d,%d]", attr->start_index, attr->end_index);
+
+  class = g_type_class_ref (pango_attr_type_get_type ());
+  value = g_enum_get_value (class, attr->klass->type);
+  g_string_append_printf (string, "%s=", value->value_nick);
+  g_type_class_unref (class);
+
+  switch (attr->klass->type)
+    {
+    case PANGO_ATTR_LANGUAGE:
+      g_string_append (string, pango_language_to_string (((PangoAttrLanguage *)attr)->value));
+      break;
+    case PANGO_ATTR_FAMILY:
+    case PANGO_ATTR_FONT_FEATURES:
+      g_string_append (string, ((PangoAttrString *)attr)->value);
+      break;
+    case PANGO_ATTR_STYLE:
+    case PANGO_ATTR_WEIGHT:
+    case PANGO_ATTR_VARIANT:
+    case PANGO_ATTR_STRETCH:
+    case PANGO_ATTR_SIZE:
+    case PANGO_ATTR_ABSOLUTE_SIZE:
+    case PANGO_ATTR_UNDERLINE:
+    case PANGO_ATTR_OVERLINE:
+    case PANGO_ATTR_STRIKETHROUGH:
+    case PANGO_ATTR_RISE:
+    case PANGO_ATTR_FALLBACK:
+    case PANGO_ATTR_LETTER_SPACING:
+    case PANGO_ATTR_GRAVITY:
+    case PANGO_ATTR_GRAVITY_HINT:
+    case PANGO_ATTR_FOREGROUND_ALPHA:
+    case PANGO_ATTR_BACKGROUND_ALPHA:
+    case PANGO_ATTR_ALLOW_BREAKS:
+    case PANGO_ATTR_INSERT_HYPHENS:
+    case PANGO_ATTR_SHOW:
+      g_string_append_printf (string, "%d", ((PangoAttrInt *)attr)->value);
+      break;
+    case PANGO_ATTR_FONT_DESC:
+      {
+        char *text = pango_font_description_to_string (((PangoAttrFontDesc *)attr)->desc);
+        g_string_append (string, text);
+        g_free (text);
+      }
+      break;
+    case PANGO_ATTR_FOREGROUND:
+    case PANGO_ATTR_BACKGROUND:
+    case PANGO_ATTR_UNDERLINE_COLOR:
+    case PANGO_ATTR_OVERLINE_COLOR:
+    case PANGO_ATTR_STRIKETHROUGH_COLOR:
+      {
+        char *text = pango_color_to_string (&((PangoAttrColor *)attr)->color);
+        g_string_append (string, text);
+        g_free (text);
+      }
+      break;
+    case PANGO_ATTR_SHAPE:
+      g_string_append_printf (string, "shape");
+      break;
+    case PANGO_ATTR_SCALE:
+      {
+        char val[20];
+
+        g_ascii_formatd (val, 20, "%f", ((PangoAttrFloat *)attr)->value);
+        g_string_append (string, val);
+     }
+      break;
+    case PANGO_ATTR_INVALID:
+    default:
+      g_assert_not_reached ();
+      break;
+    }
+}
+
+void
+print_attr_list (PangoAttrList *attrs, GString *string)
+{
+  PangoAttrIterator *iter;
+
+  if (!attrs)
+    return;
+
+  iter = pango_attr_list_get_iterator (attrs);
+  do {
+    gint start, end;
+    GSList *list, *l;
+
+    pango_attr_iterator_range (iter, &start, &end);
+    g_string_append_printf (string, "range %d %d\n", start, end);
+    list = pango_attr_iterator_get_attrs (iter);
+    for (l = list; l; l = l->next)
+      {
+        PangoAttribute *attr = l->data;
+        print_attribute (attr, string);
+        g_string_append (string, "\n");
+      }
+    g_slist_free_full (list, (GDestroyNotify)pango_attribute_destroy);
+  } while (pango_attr_iterator_next (iter));
+
+  pango_attr_iterator_destroy (iter);
+}
+
+static void
+test_label_markup (void)
+{
+  GtkWidget *window;
+  GtkWidget *label;
+  PangoAttrList *attrs;
+  GString *str;
+  const char *text;
+
+  window = gtk_window_new ();
+  label = gtk_label_new ("");
+
+  gtk_window_set_child (GTK_WINDOW (window), label);
+  gtk_window_set_mnemonics_visible (GTK_WINDOW (window), TRUE);
+
+  gtk_label_set_use_underline (GTK_LABEL (label), TRUE);
+  gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
+  gtk_label_set_label (GTK_LABEL (label), "<a href=\"test\"><span font_style=\"italic\">abc</span> 
_def</a>");
+
+  g_assert_cmpuint (gtk_label_get_mnemonic_keyval (GTK_LABEL (label)), ==, 'd');
+
+  text = pango_layout_get_text (gtk_label_get_layout (GTK_LABEL (label)));
+  g_assert_cmpstr (text, ==, "abc def");
+
+  attrs = pango_layout_get_attributes (gtk_label_get_layout (GTK_LABEL (label)));
+  str = g_string_new ("");
+  print_attr_list (attrs, str);
+
+  g_assert_cmpstr (str->str, ==,
+    "range 0 3\n"
+    "[0,4]underline=1\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "[0,3]style=2\n"
+    "range 3 4\n"
+    "[0,4]underline=1\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "range 4 5\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "[4,5]underline=3\n"
+    "range 5 8\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "[5,8]underline=1\n"
+    "range 8 2147483647\n");
+
+
+  gtk_window_set_mnemonics_visible (GTK_WINDOW (window), FALSE);
+
+  text = pango_layout_get_text (gtk_label_get_layout (GTK_LABEL (label)));
+  g_assert_cmpstr (text, ==, "abc def");
+
+  attrs = pango_layout_get_attributes (gtk_label_get_layout (GTK_LABEL (label)));
+  g_string_set_size (str, 0);
+  print_attr_list (attrs, str);
+
+  g_assert_cmpstr (str->str, ==,
+    "range 0 3\n"
+    "[0,7]underline=1\n"
+    "[0,7]foreground=#1b1b6a6acbcb\n"
+    "[0,3]style=2\n"
+    "range 3 7\n"
+    "[0,7]underline=1\n"
+    "[0,7]foreground=#1b1b6a6acbcb\n"
+    "range 7 2147483647\n");
+
+  gtk_window_set_mnemonics_visible (GTK_WINDOW (window), TRUE);
+  gtk_label_set_use_underline (GTK_LABEL (label), FALSE);
+
+  text = pango_layout_get_text (gtk_label_get_layout (GTK_LABEL (label)));
+  g_assert_cmpstr (text, ==, "abc _def");
+
+  attrs = pango_layout_get_attributes (gtk_label_get_layout (GTK_LABEL (label)));
+  g_string_set_size (str, 0);
+  print_attr_list (attrs, str);
+
+  g_assert_cmpstr (str->str, ==,
+    "range 0 3\n"
+    "[0,8]underline=1\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "[0,3]style=2\n"
+    "range 3 8\n"
+    "[0,8]underline=1\n"
+    "[0,8]foreground=#1b1b6a6acbcb\n"
+    "range 8 2147483647\n");
+
+  g_string_free (str, TRUE);
+
+  gtk_window_destroy (GTK_WINDOW (window));
+}
+
+int
+main (int argc, char *argv[])
+{
+  gtk_test_init (&argc, &argv);
+
+  g_test_add_func ("/label/markup-parse", test_label_markup);
+
+  return g_test_run ();
+}
diff --git a/testsuite/gtk/meson.build b/testsuite/gtk/meson.build
index b0af6dc642..4e04908485 100644
--- a/testsuite/gtk/meson.build
+++ b/testsuite/gtk/meson.build
@@ -50,6 +50,7 @@ tests = [
   { 'name': 'grid' },
   { 'name': 'grid-layout' },
   { 'name': 'icontheme' },
+  { 'name': 'label' },
   { 'name': 'listbox' },
   { 'name': 'main' },
   { 'name': 'maplistmodel' },