[gtksourceview/gnome-3-14] UndoManager: fix a crash with mixed user action/not undoable action

[Sébastien Wilmet <swilmet src gnome org>]

commit 4717602dfa4e169a9bcc5e5378d833c36fe5abd5
Author: Sébastien Wilmet <swilmet gnome org>
Date:   Mon Nov 10 20:41:20 2014 +0100

    UndoManager: fix a crash with mixed user action/not undoable action
    
    Blocking signals should be done carefully, the internal state of the
    UndoManager must still be consistent with the text buffer.
    
    So don't block the begin/end-user-action signals.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741955

 gtksourceview/gtksourceundomanagerdefault.c |  174 ++++++++++-----------------
 tests/test-undo-manager.c                   |  140 +++++++++++++++++++++-
 2 files changed, 202 insertions(+), 112 deletions(-)
---
diff --git a/gtksourceview/gtksourceundomanagerdefault.c b/gtksourceview/gtksourceundomanagerdefault.c
index 8880311..2d336bf 100644
--- a/gtksourceview/gtksourceundomanagerdefault.c
+++ b/gtksourceview/gtksourceundomanagerdefault.c
@@ -6,7 +6,7 @@
  * Copyright (C) 1998, 1999 - Alex Roberts, Evan Lawrence
  * Copyright (C) 2000, 2001 - Chema Celorio, Paolo Maggi
  * Copyright (C) 2002-2005  - Paolo Maggi
- * Copyright (C) 2014 - Sébastien Wilmet <swilmet gnome org>
+ * Copyright (C) 2014, 2015 - Sébastien Wilmet <swilmet gnome org>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -126,6 +126,16 @@ struct _GtkSourceUndoManagerDefaultPrivate
         */
        GList *location;
 
+       /* A new ActionGroup that is created when some text is inserted or
+        * deleted in the buffer. As long as a user action is running (when
+        * 'running_user_action' is TRUE) the new actions are inserted into
+        * 'new_action_group'. When the user action ends, we try to merge
+        * 'new_action_group' with the previous ActionGroup in 'action_groups'
+        * (the node on the left of 'location'). If the merging fails, a new
+        * node is inserted on the left of 'location'.
+        */
+       ActionGroup *new_action_group;
+
        /* The number of nested calls to
         * gtk_source_buffer_begin_not_undoable_action().
         */
@@ -283,6 +293,9 @@ clear_all (GtkSourceUndoManagerDefault *manager)
        manager->priv->location = NULL;
        manager->priv->saved_location = NULL;
 
+       action_group_free (manager->priv->new_action_group);
+       manager->priv->new_action_group = NULL;
+
        update_can_undo_can_redo (manager);
 }
 
@@ -376,40 +389,6 @@ check_history_size (GtkSourceUndoManagerDefault *manager)
 }
 
 static void
-insert_new_action_group (GtkSourceUndoManagerDefault *manager)
-{
-       ActionGroup *group = action_group_new ();
-
-       if (manager->priv->location != NULL)
-       {
-               g_queue_insert_before (manager->priv->action_groups,
-                                      manager->priv->location,
-                                      group);
-       }
-       else
-       {
-               g_queue_push_tail (manager->priv->action_groups,
-                                  group);
-       }
-
-       if (manager->priv->has_saved_location &&
-           manager->priv->saved_location == manager->priv->location)
-       {
-               if (manager->priv->saved_location != NULL)
-               {
-                       manager->priv->saved_location = manager->priv->saved_location->prev;
-               }
-               else
-               {
-                       manager->priv->saved_location = manager->priv->action_groups->tail;
-               }
-
-               g_assert (manager->priv->saved_location != NULL);
-               g_assert (manager->priv->saved_location->data == group);
-       }
-}
-
-static void
 remove_redo_action_groups (GtkSourceUndoManagerDefault *manager)
 {
        while (manager->priv->location != NULL)
@@ -450,74 +429,69 @@ action_group_merge (ActionGroup *group,
        return action_merge (action, new_action);
 }
 
-/* Try to merge the current action group with the previous one. The "current
- * action group" is the node on the left of location.
+/* Try to merge the new action group with the previous one (the one located on
+ * the left of priv->location). If the merge fails, a new node is inserted into
+ * the history.
  */
 static void
-try_merge_current_action_group (GtkSourceUndoManagerDefault *manager)
+insert_new_action_group (GtkSourceUndoManagerDefault *manager)
 {
-       GList *new_node = NULL;
        GList *prev_node = NULL;
-       ActionGroup *new_group = NULL;
        ActionGroup *prev_group = NULL;
+       ActionGroup *new_group = manager->priv->new_action_group;
+       gboolean can_merge = TRUE;
 
-       if (manager->priv->location != NULL)
+       if (new_group == NULL || new_group->actions->length == 0)
        {
-               new_node = manager->priv->location->prev;
-       }
-       else
-       {
-               new_node = manager->priv->action_groups->tail;
+               return;
        }
 
-       g_assert (new_node != NULL);
-       new_group = new_node->data;
+       remove_redo_action_groups (manager);
+       g_assert (manager->priv->location == NULL);
 
-       prev_node = new_node->prev;
+       prev_node = manager->priv->action_groups->tail;
 
        if (prev_node != NULL)
        {
                prev_group = prev_node->data;
 
                /* If the previous group is empty, it means that it was not correctly
-                * merged.
+                * inserted into the history.
                 */
                g_assert_cmpuint (prev_group->actions->length, >, 0);
        }
 
-       /* If the saved_location is between the two nodes, the two nodes cannot
-        * be merged. Except if the new node is empty.
+       /* If the saved_location is equal to the current location, the two
+        * ActionGroups cannot be merged, to not lose the saved_location.
         */
        if (manager->priv->has_saved_location &&
-           manager->priv->saved_location == new_node &&
-           new_group->actions->length > 0)
+           manager->priv->saved_location == manager->priv->location)
        {
-               goto end;
+               g_assert (manager->priv->saved_location == NULL);
+               can_merge = FALSE;
        }
 
-       if ((prev_group == NULL && new_group->actions->length == 0) ||
-           (prev_group != NULL && action_group_merge (prev_group, new_group)))
+       if (can_merge &&
+           prev_group != NULL &&
+           action_group_merge (prev_group, new_group))
        {
-               if (manager->priv->has_saved_location &&
-                   manager->priv->saved_location == new_node)
-               {
-                       manager->priv->saved_location = new_node->next;
-               }
-
-               /* Of course, no need to update location, since new_node is on
-                * the left of location.
-                */
-               g_assert (manager->priv->location != new_node);
-
-               action_group_free (new_group);
-               g_queue_delete_link (manager->priv->action_groups, new_node);
+               /* new_group merged into prev_group */
+               action_group_free (manager->priv->new_action_group);
+               manager->priv->new_action_group = NULL;
 
-               check_history_size (manager);
                update_can_undo_can_redo (manager);
                return;
        }
 
-end:
+       g_queue_push_tail (manager->priv->action_groups, new_group);
+       manager->priv->new_action_group = NULL;
+
+       if (manager->priv->has_saved_location &&
+           manager->priv->saved_location == NULL)
+       {
+               manager->priv->saved_location = manager->priv->action_groups->tail;
+       }
+
        /* "Archive" prev_group. It will never be mergeable again. If the user
         * does some undo's to return to this location, a new action won't be
         * merged with an "archived" action group.
@@ -535,20 +509,16 @@ static void
 insert_action (GtkSourceUndoManagerDefault *manager,
               Action                      *new_action)
 {
-       ActionGroup *group;
+       ActionGroup *new_group;
 
        g_assert (new_action != NULL);
 
-       remove_redo_action_groups (manager);
-       g_assert (manager->priv->location == NULL);
-
-       if (!manager->priv->running_user_action)
+       if (manager->priv->new_action_group == NULL)
        {
-               insert_new_action_group (manager);
+               manager->priv->new_action_group = action_group_new ();
        }
 
-       group = g_queue_peek_tail (manager->priv->action_groups);
-       g_assert (group != NULL);
+       new_group = manager->priv->new_action_group;
 
        /* Inside a group, don't try to merge the actions. It is needed to keep
         * them separate so when undoing or redoing, the cursor position is set
@@ -558,7 +528,7 @@ insert_action (GtkSourceUndoManagerDefault *manager,
         * undo, the cursor position should be placed at "a|aba", not "aa|ba"
         * (but it's a detail).
         */
-       g_queue_push_tail (group->actions, new_action);
+       g_queue_push_tail (new_group->actions, new_action);
 
        /* An action is mergeable only for an insertion or deletion of a single
         * character. If the text contains several characters, the new_action
@@ -567,12 +537,12 @@ insert_action (GtkSourceUndoManagerDefault *manager,
        if (new_action->end - new_action->start > 1 ||
            g_str_equal (new_action->text, "\n"))
        {
-               group->force_not_mergeable = TRUE;
+               new_group->force_not_mergeable = TRUE;
        }
 
        if (!manager->priv->running_user_action)
        {
-               try_merge_current_action_group (manager);
+               insert_new_action_group (manager);
        }
 }
 
@@ -1155,8 +1125,6 @@ static void
 begin_user_action_cb (GtkTextBuffer               *buffer,
                      GtkSourceUndoManagerDefault *manager)
 {
-       insert_new_action_group (manager);
-
        manager->priv->running_user_action = TRUE;
        update_can_undo_can_redo (manager);
 }
@@ -1165,7 +1133,7 @@ static void
 end_user_action_cb (GtkTextBuffer               *buffer,
                    GtkSourceUndoManagerDefault *manager)
 {
-       try_merge_current_action_group (manager);
+       insert_new_action_group (manager);
 
        manager->priv->running_user_action = FALSE;
        update_can_undo_can_redo (manager);
@@ -1181,7 +1149,9 @@ modified_changed_cb (GtkTextBuffer               *buffer,
                 * deleted.
                 */
                if (manager->priv->has_saved_location &&
-                   manager->priv->saved_location == manager->priv->location)
+                   manager->priv->saved_location == manager->priv->location &&
+                   (manager->priv->new_action_group == NULL ||
+                    manager->priv->new_action_group->actions->length == 0))
                {
                        manager->priv->has_saved_location = FALSE;
                }
@@ -1190,18 +1160,17 @@ modified_changed_cb (GtkTextBuffer               *buffer,
        /* saved */
        else
        {
-               manager->priv->saved_location = manager->priv->location;
-               manager->priv->has_saved_location = TRUE;
-
                /* Saving a buffer during a user action is allowed, the user
                 * action is split.
                 * FIXME and/or a warning should be printed?
                 */
                if (manager->priv->running_user_action)
                {
-                       try_merge_current_action_group (manager);
                        insert_new_action_group (manager);
                }
+
+               manager->priv->saved_location = manager->priv->location;
+               manager->priv->has_saved_location = TRUE;
        }
 }
 
@@ -1222,14 +1191,6 @@ block_signal_handlers (GtkSourceUndoManagerDefault *manager)
                                         manager);
 
        g_signal_handlers_block_by_func (manager->priv->buffer,
-                                        begin_user_action_cb,
-                                        manager);
-
-       g_signal_handlers_block_by_func (manager->priv->buffer,
-                                        end_user_action_cb,
-                                        manager);
-
-       g_signal_handlers_block_by_func (manager->priv->buffer,
                                         modified_changed_cb,
                                         manager);
 }
@@ -1251,14 +1212,6 @@ unblock_signal_handlers (GtkSourceUndoManagerDefault *manager)
                                           manager);
 
        g_signal_handlers_unblock_by_func (manager->priv->buffer,
-                                          begin_user_action_cb,
-                                          manager);
-
-       g_signal_handlers_unblock_by_func (manager->priv->buffer,
-                                          end_user_action_cb,
-                                          manager);
-
-       g_signal_handlers_unblock_by_func (manager->priv->buffer,
                                           modified_changed_cb,
                                           manager);
 }
@@ -1386,6 +1339,8 @@ gtk_source_undo_manager_default_finalize (GObject *object)
        g_queue_free_full (manager->priv->action_groups,
                           (GDestroyNotify) action_group_free);
 
+       action_group_free (manager->priv->new_action_group);
+
        G_OBJECT_CLASS (gtk_source_undo_manager_default_parent_class)->finalize (object);
 }
 
@@ -1485,11 +1440,8 @@ gtk_source_undo_manager_undo_impl (GtkSourceUndoManager *undo_manager)
        }
 
        g_assert (new_location != NULL);
-       group = new_location->data;
 
-       /* Empty groups are inserted at the beginning of a user action, but
-        * during a user action can_undo is FALSE.
-        */
+       group = new_location->data;
        g_assert_cmpuint (group->actions->length, >, 0);
 
        block_signal_handlers (manager);
diff --git a/tests/test-undo-manager.c b/tests/test-undo-manager.c
index a145477..b66300f 100644
--- a/tests/test-undo-manager.c
+++ b/tests/test-undo-manager.c
@@ -2,7 +2,7 @@
 /* test-undo-manager.c
  * This file is part of GtkSourceView
  *
- * Copyright (C) 2013, 2014 - Sébastien Wilmet <swilmet gnome org>
+ * Copyright (C) 2013, 2014, 2015 - Sébastien Wilmet <swilmet gnome org>
  *
  * GtkSourceView is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -719,6 +719,141 @@ test_bug_672893_selection_restoring (void)
        g_object_unref (source_buffer);
 }
 
+static void
+test_mix_user_action_and_not_undoable_action (void)
+{
+       GtkSourceBuffer *source_buffer;
+       GtkTextBuffer *text_buffer;
+       GList *contents_history = NULL;
+
+       source_buffer = gtk_source_buffer_new (NULL);
+       text_buffer = GTK_TEXT_BUFFER (source_buffer);
+
+       gtk_source_buffer_set_max_undo_levels (source_buffer, -1);
+
+       /* Case 1 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_text_buffer_end_user_action (text_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 2 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_text_buffer_end_user_action (text_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 3 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_text_buffer_end_user_action (text_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 4 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+       gtk_text_buffer_end_user_action (text_buffer);
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "b\n", -1);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 5 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_text_buffer_end_user_action (text_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 6 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_text_buffer_end_user_action (text_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "b\n", -1);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       /* Case 7 */
+       gtk_text_buffer_set_text (text_buffer, "", -1);
+
+       gtk_text_buffer_begin_user_action (text_buffer);
+       gtk_text_buffer_insert_at_cursor (text_buffer, "a\n", -1);
+       gtk_source_buffer_begin_not_undoable_action (source_buffer);
+       gtk_source_buffer_end_not_undoable_action (source_buffer);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+       gtk_text_buffer_end_user_action (text_buffer);
+
+       gtk_text_buffer_insert_at_cursor (text_buffer, "b\n", -1);
+       contents_history = g_list_append (contents_history, get_contents (source_buffer));
+
+       check_contents_history (source_buffer, contents_history);
+
+       g_list_free_full (contents_history, g_free);
+       contents_history = NULL;
+
+       g_object_unref (source_buffer);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -757,5 +892,8 @@ main (int argc, char **argv)
        g_test_add_func ("/UndoManager/test-bug-672893-selection-restoring",
                         test_bug_672893_selection_restoring);
 
+       g_test_add_func ("/UndoManager/mix-user-action-and-not-undoable-action",
+                        test_mix_user_action_and_not_undoable_action);
+
        return g_test_run ();
 }