[gtk/wip/chergert/fix-class-private-data-usage] widget: fix class private data usage to be _init() safe

[Christian Hergert <chergert src gnome org>]

commit 7a7e4e154ee156a78e6491716cbba1a35200cb93
Author: Christian Hergert <chergert redhat com>
Date:   Fri Mar 20 09:22:29 2020 -0700

    widget: fix class private data usage to be _init() safe
    
    Before this commit, adding GtkWidgetAction to class private data would
    require copying the actions to each subclass as they were built or
    modified. This was convenient in that it is a sort of "copy on write"
    semantic.
    
    However, due to the way that GTypeInstance works with base _init()
    functions, the "g_class" pointer in GTypeInstance is updated as each
    _init() function is called. That means you cannot access the subclasses
    class private data, but only the parent class private data.
    
    If instead we use a singly linked list of GtkWidgetAction, each subclass
    has their own "head" yet all subclasses share the tail of the
    GtkWidgetAction chain.
    
    This creates one bit of complexity though. You need a stable way to know
    which "bit" is the "enabled" bit of the action so we can track enabled
    GAction state. That is easily solved by calculating the distance to the
    end of the chain for a given action so that base classes sort ahead of
    subclasses. Since the parent class always knows its parent's actions, the
    position is stable.
    
    A new dynamic bitarray helper also helps us avoid allocations in all the
    current cases (up to 64 actions per widget) and dynamically switches to
    malloc if that is to ever be exceeded.

 gtk/gtkactionmuxer.c        | 188 +++++++++++++++++++++++---------------------
 gtk/gtkactionmuxerprivate.h |  15 ++--
 gtk/gtkapplication.c        |   2 +-
 gtk/gtkwidget.c             |  31 ++------
 testsuite/gtk/action.c      |  16 ++--
 5 files changed, 123 insertions(+), 129 deletions(-)
---
diff --git a/gtk/gtkactionmuxer.c b/gtk/gtkactionmuxer.c
index 521f77e6f0..bc9e8fc6db 100644
--- a/gtk/gtkactionmuxer.c
+++ b/gtk/gtkactionmuxer.c
@@ -25,9 +25,11 @@
 #include "gtkactionobserverprivate.h"
 #include "gtkintl.h"
 #include "gtkmarshalers.h"
-#include "gtkwidget.h"
+#include "gtkwidgetprivate.h"
 #include "gsettings-mapping.h"
 
+#include "bitarray.h"
+
 #include <string.h>
 
 /*< private >
@@ -73,8 +75,8 @@ struct _GtkActionMuxer
   GtkActionMuxer *parent;
 
   GtkWidget *widget;
-  GPtrArray *widget_actions;
-  gboolean *widget_actions_enabled;
+
+  BitArray widget_actions_enabled;
 };
 
 G_DEFINE_TYPE_WITH_CODE (GtkActionMuxer, gtk_action_muxer, G_TYPE_OBJECT,
@@ -86,7 +88,6 @@ enum
   PROP_0,
   PROP_PARENT,
   PROP_WIDGET,
-  PROP_WIDGET_ACTIONS,
   NUM_PROPERTIES
 };
 
@@ -109,6 +110,19 @@ typedef struct
   gulong        handler_ids[4];
 } Group;
 
+static inline guint
+get_action_position (GtkWidgetAction *action)
+{
+  guint slot;
+  /* We use the length of @action to the end of the chain as the slot so that
+   * we have stable positions for any class or it's subclasses. Doing so helps
+   * us avoid having mutable arrays in the class data as we will not have
+   * access to the ClassPrivate data during instance _init() functions.
+   */
+  for (slot = 0; action->next != NULL; slot++, action = action->next) {}
+  return slot;
+}
+
 static void
 gtk_action_muxer_append_group_actions (const char *prefix,
                                        Group      *group,
@@ -143,15 +157,14 @@ gtk_action_muxer_list_actions (GActionGroup *action_group)
       const char *prefix;
       Group *group;
 
-      if (muxer->widget_actions)
+      if (muxer->widget)
         {
-          int i;
+          GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+          GtkWidgetClassPrivate *priv = klass->priv;
+          GtkWidgetAction *action;
 
-          for (i = 0; i < muxer->widget_actions->len; i++)
-            {
-              GtkWidgetAction *action = g_ptr_array_index (muxer->widget_actions, i);
-              g_hash_table_add (actions, g_strdup (action->name));
-            }
+          for (action = priv->actions; action; action = action->next)
+            g_hash_table_add (actions, g_strdup (action->name));
         }
 
       g_hash_table_iter_init (&iter, muxer->groups);
@@ -217,22 +230,26 @@ gtk_action_muxer_action_enabled_changed (GtkActionMuxer *muxer,
                                          const gchar    *action_name,
                                          gboolean        enabled)
 {
+  GtkWidgetAction *iter;
   Action *action;
   GSList *node;
 
-  if (muxer->widget_actions)
+  if (muxer->widget)
     {
-      int i;
-      for (i = 0; i < muxer->widget_actions->len; i++)
+      GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+      GtkWidgetClassPrivate *priv = klass->priv;
+
+      for (iter = priv->actions; iter; iter = iter->next)
         {
-          GtkWidgetAction *a = g_ptr_array_index (muxer->widget_actions, i);
-          if (strcmp (a->name, action_name) == 0)
+          if (strcmp (action_name, iter->name) == 0)
             {
-              muxer->widget_actions_enabled[i] = enabled;
+              guint position = get_action_position (iter);
+              bit_array_set (&muxer->widget_actions_enabled, position, enabled);
               break;
             }
         }
     }
+
   action = g_hash_table_lookup (muxer->observed_actions, action_name);
   for (node = action ? action->watchers : NULL; node; node = node->next)
     gtk_action_observer_action_enabled_changed (node->data, GTK_ACTION_OBSERVABLE (muxer), action_name, 
enabled);
@@ -517,21 +534,21 @@ prop_action_notify (GObject    *object,
                     gpointer    user_data)
 {
   GtkActionMuxer *muxer = user_data;
-  int i;
+  GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+  GtkWidgetClassPrivate *priv = klass->priv;
   GtkWidgetAction *action = NULL;
   GVariant *state;
 
   g_assert ((GObject *)muxer->widget == object);
 
-  for (i = 0; i < muxer->widget_actions->len; i++)
+  for (action = priv->actions; action; action = action->next)
     {
-      action = g_ptr_array_index (muxer->widget_actions, i);
       if (action->pspec == pspec)
         break;
-      action = NULL;
     }
 
   g_assert (action != NULL);
+  g_assert (action->pspec == pspec);
 
   state = prop_action_get_state (muxer->widget, action);
   gtk_action_muxer_action_state_changed (muxer, action->name, state);
@@ -541,14 +558,20 @@ prop_action_notify (GObject    *object,
 static void
 prop_actions_connect (GtkActionMuxer *muxer)
 {
-  int i;
+  GtkWidgetClassPrivate *priv;
+  GtkWidgetAction *action;
+  GtkWidgetClass *klass;
+
+  if (!muxer->widget)
+    return;
 
-  if (!muxer->widget || !muxer->widget_actions)
+  klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+  priv = klass->priv;
+  if (!priv->actions)
     return;
 
-  for (i = 0; i < muxer->widget_actions->len; i++)
+  for (action = priv->actions; action; action = action->next)
     {
-      GtkWidgetAction *action = g_ptr_array_index (muxer->widget_actions, i);
       char *detailed;
 
       if (!action->pspec)
@@ -572,41 +595,46 @@ gtk_action_muxer_query_action (GActionGroup        *action_group,
                                GVariant           **state)
 {
   GtkActionMuxer *muxer = GTK_ACTION_MUXER (action_group);
+  GtkWidgetAction *action;
   Group *group;
   const gchar *unprefixed_name;
 
-  if (muxer->widget_actions)
+  if (muxer->widget)
     {
-      int i;
+      GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+      GtkWidgetClassPrivate *priv = klass->priv;
 
-      for (i = 0; i < muxer->widget_actions->len; i++)
+      for (action = priv->actions; action; action = action->next)
         {
-          GtkWidgetAction *action = g_ptr_array_index (muxer->widget_actions, i);
-          if (strcmp (action->name, action_name) == 0)
-            {
-              if (enabled)
-                *enabled = muxer->widget_actions_enabled[i];
-              if (parameter_type)
-                *parameter_type = action->parameter_type;
-              if (state_type)
-                *state_type = action->state_type;
+          guint position;
 
-              if (state_hint)
-                *state_hint = NULL;
-              if (state)
-                *state = NULL;
+          if (strcmp (action->name, action_name) != 0)
+            continue;
 
-              if (action->pspec)
-                {
-                  if (state)
-                    *state = prop_action_get_state (muxer->widget, action);
-                  if (state_hint)
-                    *state_hint = prop_action_get_state_hint (muxer->widget, action);
-                }
+          position = get_action_position (action);
+
+          if (enabled)
+            *enabled = bit_array_get (&muxer->widget_actions_enabled, position);
+          if (parameter_type)
+            *parameter_type = action->parameter_type;
+          if (state_type)
+            *state_type = action->state_type;
+
+          if (state_hint)
+            *state_hint = NULL;
+          if (state)
+            *state = NULL;
 
-              return TRUE;
+          if (action->pspec)
+            {
+              if (state)
+                *state = prop_action_get_state (muxer->widget, action);
+              if (state_hint)
+                *state_hint = prop_action_get_state_hint (muxer->widget, action);
             }
-       }
+
+          return TRUE;
+        }
     }
 
   group = gtk_action_muxer_find_group (muxer, action_name, &unprefixed_name);
@@ -629,19 +657,22 @@ gtk_action_muxer_activate_action (GActionGroup *action_group,
                                   GVariant     *parameter)
 {
   GtkActionMuxer *muxer = GTK_ACTION_MUXER (action_group);
-  Group *group;
   const gchar *unprefixed_name;
+  Group *group;
 
-  if (muxer->widget_actions)
+  if (muxer->widget)
     {
-      int i;
+      GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+      GtkWidgetClassPrivate *priv = klass->priv;
+      GtkWidgetAction *action;
 
-      for (i = 0; i < muxer->widget_actions->len; i++)
+      for (action = priv->actions; action; action = action->next)
         {
-          GtkWidgetAction *action = g_ptr_array_index (muxer->widget_actions, i);
           if (strcmp (action->name, action_name) == 0)
             {
-              if (muxer->widget_actions_enabled[i])
+              guint position = get_action_position (action);
+
+              if (bit_array_get (&muxer->widget_actions_enabled, position))
                 {
                   if (action->activate)
                     action->activate (muxer->widget, action->name, parameter);
@@ -668,16 +699,17 @@ gtk_action_muxer_change_action_state (GActionGroup *action_group,
                                       GVariant     *state)
 {
   GtkActionMuxer *muxer = GTK_ACTION_MUXER (action_group);
-  Group *group;
+  GtkWidgetAction *action;
   const gchar *unprefixed_name;
+  Group *group;
 
-  if (muxer->widget_actions)
+  if (muxer->widget)
     {
-      int i;
+      GtkWidgetClass *klass = GTK_WIDGET_GET_CLASS (muxer->widget);
+      GtkWidgetClassPrivate *priv = klass->priv;
 
-      for (i = 0; i < muxer->widget_actions->len; i++)
+      for (action = priv->actions; action; action = action->next)
         {
-          GtkWidgetAction *action = g_ptr_array_index (muxer->widget_actions, i);
           if (strcmp (action->name, action_name) == 0)
             {
               if (action->pspec)
@@ -803,10 +835,9 @@ gtk_action_muxer_finalize (GObject *object)
   if (muxer->primary_accels)
     g_hash_table_unref (muxer->primary_accels);
 
-  g_free (muxer->widget_actions_enabled);
+  bit_array_clear (&muxer->widget_actions_enabled);
 
-  G_OBJECT_CLASS (gtk_action_muxer_parent_class)
-    ->finalize (object);
+  G_OBJECT_CLASS (gtk_action_muxer_parent_class)->finalize (object);
 }
 
 static void
@@ -859,10 +890,6 @@ gtk_action_muxer_get_property (GObject    *object,
       g_value_set_object (value, muxer->widget);
       break;
 
-    case PROP_WIDGET_ACTIONS:
-      g_value_set_boxed (value, muxer->widget_actions);
-      break;
-
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
     }
@@ -886,18 +913,6 @@ gtk_action_muxer_set_property (GObject      *object,
       muxer->widget = g_value_get_object (value);
       break;
 
-    case PROP_WIDGET_ACTIONS:
-      muxer->widget_actions = g_value_get_boxed (value);
-      if (muxer->widget_actions)
-        {
-          int i;
-
-          muxer->widget_actions_enabled = g_new (gboolean, muxer->widget_actions->len);
-          for (i = 0; i < muxer->widget_actions->len; i++)
-            muxer->widget_actions_enabled[i] = TRUE;
-        }
-      break;
-
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
     }
@@ -908,6 +923,7 @@ gtk_action_muxer_init (GtkActionMuxer *muxer)
 {
   muxer->observed_actions = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, 
gtk_action_muxer_free_action);
   muxer->groups = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, gtk_action_muxer_free_group);
+  bit_array_init (&muxer->widget_actions_enabled, TRUE);
 }
 
 static void
@@ -959,13 +975,6 @@ gtk_action_muxer_class_init (GObjectClass *class)
                                                  G_PARAM_CONSTRUCT_ONLY |
                                                  G_PARAM_STATIC_STRINGS);
 
-  properties[PROP_WIDGET_ACTIONS] = g_param_spec_boxed ("widget-actions", "Widget actions",
-                                                        "Widget actions",
-                                                        G_TYPE_PTR_ARRAY,
-                                                        G_PARAM_READWRITE |
-                                                        G_PARAM_CONSTRUCT_ONLY |
-                                                        G_PARAM_STATIC_STRINGS);
-
   g_object_class_install_properties (class, NUM_PROPERTIES, properties);
 }
 
@@ -1060,17 +1069,14 @@ gtk_action_muxer_remove (GtkActionMuxer *muxer,
 /*< private >
  * gtk_action_muxer_new:
  * @widget: the widget to which the muxer belongs
- * @actions: widget actions
  *
  * Creates a new #GtkActionMuxer.
  */
 GtkActionMuxer *
-gtk_action_muxer_new (GtkWidget *widget,
-                      GPtrArray *actions)
+gtk_action_muxer_new (GtkWidget *widget)
 {
   return g_object_new (GTK_TYPE_ACTION_MUXER,
                        "widget", widget,
-                       "widget-actions", actions,
                        NULL);
 }
 
diff --git a/gtk/gtkactionmuxerprivate.h b/gtk/gtkactionmuxerprivate.h
index a37d30cf7e..0c5670c6f8 100644
--- a/gtk/gtkactionmuxerprivate.h
+++ b/gtk/gtkactionmuxerprivate.h
@@ -31,7 +31,13 @@ G_BEGIN_DECLS
 #define GTK_IS_ACTION_MUXER(inst)                           (G_TYPE_CHECK_INSTANCE_TYPE ((inst),             
        \
                                                              GTK_TYPE_ACTION_MUXER))
 
-typedef struct {
+typedef struct _GtkWidgetAction GtkWidgetAction;
+typedef struct _GtkActionMuxer GtkActionMuxer;
+
+struct _GtkWidgetAction
+{
+  GtkWidgetAction *next;
+
   char *name;
   GType owner;
 
@@ -40,13 +46,10 @@ typedef struct {
 
   const GVariantType *state_type;
   GParamSpec *pspec;
-} GtkWidgetAction;
-
-typedef struct _GtkActionMuxer                              GtkActionMuxer;
+};
 
 GType                   gtk_action_muxer_get_type                       (void);
-GtkActionMuxer *        gtk_action_muxer_new                            (GtkWidget      *widget,
-                                                                         GPtrArray      *actions);
+GtkActionMuxer *        gtk_action_muxer_new                            (GtkWidget      *widget);
 
 void                    gtk_action_muxer_insert                         (GtkActionMuxer *muxer,
                                                                          const gchar    *prefix,
diff --git a/gtk/gtkapplication.c b/gtk/gtkapplication.c
index 768915c807..99eae4248a 100644
--- a/gtk/gtkapplication.c
+++ b/gtk/gtkapplication.c
@@ -402,7 +402,7 @@ gtk_application_init (GtkApplication *application)
 {
   GtkApplicationPrivate *priv = gtk_application_get_instance_private (application);
 
-  priv->muxer = gtk_action_muxer_new (NULL, NULL);
+  priv->muxer = gtk_action_muxer_new (NULL);
 
   priv->accels = gtk_application_accels_new ();
 }
diff --git a/gtk/gtkwidget.c b/gtk/gtkwidget.c
index 325cc69794..df69b3eb4b 100644
--- a/gtk/gtkwidget.c
+++ b/gtk/gtkwidget.c
@@ -11095,7 +11095,7 @@ _gtk_widget_get_action_muxer (GtkWidget *widget,
 
   if (create || priv->actions)
     {
-      muxer = gtk_action_muxer_new (widget, priv->actions);
+      muxer = gtk_action_muxer_new (widget);
       g_object_set_qdata_full (G_OBJECT (widget),
                                quark_action_muxer,
                                muxer,
@@ -12673,29 +12673,12 @@ gtk_widget_class_add_action (GtkWidgetClass  *widget_class,
 {
   GtkWidgetClassPrivate *priv = widget_class->priv;
 
-  if (priv->actions == NULL)
-    priv->actions = g_ptr_array_new ();
-  else
-    {
-      GtkWidgetClass *parent_class = GTK_WIDGET_CLASS (g_type_class_peek (g_type_parent (G_TYPE_FROM_CLASS 
(widget_class))));
-      GtkWidgetClassPrivate *parent_priv = parent_class->priv;
-      GPtrArray *parent_actions = parent_priv->actions;
-
-      if (priv->actions == parent_actions)
-        {
-          int i;
-
-          priv->actions = g_ptr_array_new ();
-          for (i = 0; i < parent_actions->len; i++)
-            g_ptr_array_add (priv->actions, g_ptr_array_index (parent_actions, i));
-        }
-    }
-
   GTK_NOTE(ACTIONS, g_message ("%sClass: Adding %s action\n",
                                g_type_name (G_TYPE_FROM_CLASS (widget_class)),
                                action->name));
 
-  g_ptr_array_add (priv->actions, action);
+  action->next = priv->actions;
+  priv->actions = action;
 }
 
 /*
@@ -12875,11 +12858,13 @@ gtk_widget_class_query_action (GtkWidgetClass      *widget_class,
                                const char         **property_name)
 {
   GtkWidgetClassPrivate *priv = widget_class->priv;
+  GtkWidgetAction *action = priv->actions;
 
-  if (priv->actions && index_ < priv->actions->len)
-    {
-      GtkWidgetAction *action = g_ptr_array_index (priv->actions, index_);
+  for (; index_ > 0 && action != NULL; index_--)
+    action = action->next;
 
+  if (action != NULL && index_ == 0)
+    {
       *owner = action->owner;
       *action_name = action->name;
       *parameter_type = action->parameter_type;
diff --git a/testsuite/gtk/action.c b/testsuite/gtk/action.c
index 23a78fa23e..1812d45ba5 100644
--- a/testsuite/gtk/action.c
+++ b/testsuite/gtk/action.c
@@ -358,15 +358,15 @@ test_introspection (void)
     const char *params;
     const char *property;
   } expected[] = {
-    { GTK_TYPE_TEXT, "text.undo", NULL, NULL },
-    { GTK_TYPE_TEXT, "text.redo", NULL, NULL },
-    { GTK_TYPE_TEXT, "clipboard.cut", NULL, NULL },
-    { GTK_TYPE_TEXT, "clipboard.copy", NULL, NULL },
-    { GTK_TYPE_TEXT, "clipboard.paste", NULL, NULL },
-    { GTK_TYPE_TEXT, "selection.delete", NULL, NULL },
-    { GTK_TYPE_TEXT, "selection.select-all", NULL, NULL },
-    { GTK_TYPE_TEXT, "misc.insert-emoji", NULL, NULL },
     { GTK_TYPE_TEXT, "misc.toggle-visibility", NULL, "visibility" },
+    { GTK_TYPE_TEXT, "misc.insert-emoji", NULL, NULL },
+    { GTK_TYPE_TEXT, "selection.select-all", NULL, NULL },
+    { GTK_TYPE_TEXT, "selection.delete", NULL, NULL },
+    { GTK_TYPE_TEXT, "clipboard.paste", NULL, NULL },
+    { GTK_TYPE_TEXT, "clipboard.copy", NULL, NULL },
+    { GTK_TYPE_TEXT, "clipboard.cut", NULL, NULL },
+    { GTK_TYPE_TEXT, "text.redo", NULL, NULL },
+    { GTK_TYPE_TEXT, "text.undo", NULL, NULL },
   };
 
   i = 0;