[gtksourceview/wip/undo-redo-fix] UndoManager: fix a crash with mixed user action/not undoable action (wip)

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

commit f4984f908af2820482e590787bbb36da535482bc
Author: Sébastien Wilmet <swilmet gnome org>
Date:   Fri Jan 2 12:31:21 2015 +0100

    UndoManager: fix a crash with mixed user action/not undoable action (wip)
    
    Blocking signals should be done carefully.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741955

 gtksourceview/gtksourceundomanagerdefault.c |  113 +++++++++------------------
 tests/test-undo-manager.c                   |  104 ++++++++++++++++++++++++-
 2 files changed, 141 insertions(+), 76 deletions(-)
---
diff --git a/gtksourceview/gtksourceundomanagerdefault.c b/gtksourceview/gtksourceundomanagerdefault.c
index 7e2b238..a178950 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().
         */
@@ -376,32 +386,6 @@ check_history_size (GtkSourceUndoManagerDefault *manager)
 }
 
 static void
-insert_new_action_group (GtkSourceUndoManagerDefault *manager)
-{
-       ActionGroup *group = action_group_new ();
-
-       g_queue_insert_before (manager->priv->action_groups,
-                              manager->priv->location,
-                              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)
@@ -442,47 +426,49 @@ 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;
+
+       if (new_group == NULL || new_group->actions->length == 0)
+       {
+               return;
+       }
+
+       remove_redo_action_groups (manager);
+       g_assert (manager->priv->location == NULL);
 
        if (manager->priv->location != NULL)
        {
-               new_node = manager->priv->location->prev;
+               prev_node = manager->priv->location->prev;
        }
        else
        {
-               new_node = manager->priv->action_groups->tail;
+               prev_node = manager->priv->action_groups->tail;
        }
 
-       g_assert (new_node != NULL);
-       new_group = new_node->data;
-
-       prev_node = new_node->prev;
-
        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;
        }
@@ -527,20 +513,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
@@ -550,7 +532,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
@@ -559,12 +541,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);
        }
 }
 
@@ -1147,8 +1129,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);
 }
@@ -1157,7 +1137,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);
@@ -1191,7 +1171,6 @@ modified_changed_cb (GtkTextBuffer               *buffer,
                 */
                if (manager->priv->running_user_action)
                {
-                       try_merge_current_action_group (manager);
                        insert_new_action_group (manager);
                }
        }
@@ -1214,14 +1193,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);
 }
@@ -1243,14 +1214,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);
 }
diff --git a/tests/test-undo-manager.c b/tests/test-undo-manager.c
index a145477..ecacc79 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,105 @@ 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;
+
+       g_object_unref (source_buffer);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -757,5 +856,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 ();
 }