Created attachment 343790 [details] [review]
support for multi cursor in default key board mode

supporting these in default keyboard and insert mode currently:
	> selecting some text and <ctrl>r will create a new cursor
	> pressing escape or <ctrl>f or right clicking in some place results in 
           removing all cursors.
	> moving all cursors simultaniously
	> deleting at all cursors by pressing DEL, Backspace
	> insertion at one cursor results in insertion at all cursors

changes:
	> added these virtual functions, ide_source_view_real_add_virtual_cursor,
          ide_source_view_real_remove_virtual_cursors, to IdeSourceView Class.
	>overrided move_cursor, backspace, delete_from_cursor funtions in TextView 
          class.

Not all features are supported currently. But how about the idea and implementation?

Review of attachment 343790 [details] [review]:

I like the idea in theory. What concerns me is how twisted of a maze the editor is getting with regards to key themes like Vim and Emacs. We really need to change some fundamental pieces to make those easier and this is certainly going to complicate things. I definitely want to see this sort of feature supported though.

It might be a good idea for us to try to land a feature like this in concert with the wip/chergert/shortcuts branch. In particular, I'd like to see a bunch of the VIM/Emacs operations (like movements and such) move into external helper objects which can be keybinding activated. And those will need to take into account things like multiple cursors.

Having this feature might make some other stuff possible too (like column selection) by automating creation/destruction of additional cursors.

::: data/keybindings/default.css
@@ +9,3 @@
                   "clear-snippets" ()
+                  "hide-completion" () 
+                  "remove-virtual-cursors" () };

We would need this for "Escape" (and similar) in vim.css too. Possibly in emacs, but I don't remember.

::: libide/sourceview/ide-source-view.c
@@ +1268,3 @@
+  gtk_text_buffer_get_iter_at_mark (buffer, &insert_iter, gtk_text_buffer_get_insert(buffer));
+    
+   // check whether iter lies at cursor or not and proceed

We always use /* */ C89 style comments in Builder.

@@ +1275,3 @@
+        virtual_cursors = priv->virtual_cursors;
+       
+        while (virtual_cursors != NULL)

I think I'd prefer to see this with a for loop like:

  for (const GList *iter = priv->virtual_cursors; iter != NULL; iter = iter->next)
    {
      VirtualCursor *vc = iter->data;

      ...
    }

@@ +1282,3 @@
+            // since real cursor lies at the end of list if next element is NULL then break
+            // so that default handler will print there
+            if (virtual_cursors == NULL)

And then:

  if (iter->next == NULL)

@@ +1289,3 @@
+            gtk_text_buffer_get_iter_at_mark (buffer, &selection_bound_iter, virtual_cursor->selection_bound);
+            if (!gtk_text_iter_equal (&insert_iter, &selection_bound_iter))
+            {

Indent block 2 spaces.

@@ +1680,3 @@
+
+      while (virtual_cursors != NULL)
+      {

Indent block 2 spaces.

@@ +1690,3 @@
+        gtk_text_buffer_select_range (gtk_text_view_get_buffer (text_view), &vinsert_iter, &vselection_bound_iter);
+
+        GTK_TEXT_VIEW_CLASS(ide_source_view_parent_class)->backspace (text_view);

Space before opening parens

@@ +1697,3 @@
+  else
+    {
+      GTK_TEXT_VIEW_CLASS(ide_source_view_parent_class)->backspace (text_view);

Same

@@ +1707,3 @@
+{
+  IdeSourceViewPrivate *priv = ide_source_view_get_instance_private (IDE_SOURCE_VIEW (text_view));
+

Pre-condition g_assert()'s go here in static functions.

@@ +1747,3 @@
+{
+  IdeSourceViewPrivate *priv = ide_source_view_get_instance_private (IDE_SOURCE_VIEW (text_view));
+

We always assert for valid parameter preconditions after our declarations and before code statements. (The get_instance_private is special because it is pointer math only, and not a real function call).

So something like:

  g_assert (IDE_IS_SOURCE_VIEW (text_view));

is probably fine in this case.

Created attachment 344470 [details] [review]
support for multiple cursors in all keyboard modes and overwrite mode

sorry for the delay!

Added a new class IdeCursor which will manage cursor operations. I tried to put as much as possible in IdeCursor. But some operations like inserting text, adding and removing text are made through source view.

Quick highlight is overriding highlight of selection for secondary cursors, so I just put underline for selection for secondary cursors.

I wanted to let you know that I haven't forgotten about this, I'm just waiting until after 3.24 to go through the process of landing/refactoring. It's just to risky to land something that can potentially change so much of how the editor works after the beginning of our development cycle.

So I anticipate a few more weeks before we can realistically land it (or else we have to start branching immediately for 3.25 which isn't really ideal).

Created attachment 348701 [details] [review]
Multi Cursor support in editor

Made few changes to be able to apply patch to current master.

Review of attachment 348701 [details] [review]:

It took me a little while to figure out how to use this, but I think I've got the hang of it now.

It looks like it uses the highlighted matches to add/remove additional cursors with ctrl+shift+[du]. That seems to work well and is a cool way to do it.

What I would like to see before we merge this, is the ability to add/remove cursors without having to use the matching/selected text. I want to be able to do something like: 

 - ctrl+shift+d
 - move the cursor down a couple of lines
 - ctrl+shift+d again
 - move the cursor down a couple of lines
 - now start typing...
 - Escape to clear the cursors

Do you think this can be adjusted to support that?

Created attachment 349606 [details] [review]
Multiple Cursor support in editor

New cursors can be added in 3 ways:
-By selcting text and pressing <ctrl><shift>e a new cursor in every row of
 selected text in same column will be created.
-By holding <ctrl> and clicking any where in editor.
-By selecting text and presing <ctrl><shift>d a new cursor will be created at
 next occurence of selected text.

All cursors can be removed by pressing Escape.

Everything is working but segfault is happening when builder is closed quickly after editing with multiple cursors some times and not all the time. Segfault is happening after finalizing IdeCursor and IdeSourceView, which I checked using g_print statements.

I just put keybindings for temporary purposes. What keybindings would you like to put for adding new cursors?

Review of attachment 349606 [details] [review]:

Excellent work. There is some stuff to iterate on, but you should be very proud of your work!

::: libide/sourceview/ide-cursor.c
@@ +1,3 @@
+/* ide-cursor.c
+ *
+ * Copyright (C) 2015 Christian Hergert <christian@hergert.me>

Set yourself as copyright owner, and 2017

@@ +40,3 @@
+} VirtualCursor;
+
+G_DEFINE_TYPE (IdeCursor, ide_cursor, G_TYPE_OBJECT);

stray trailing ; should be removed

@@ +63,3 @@
+                                   VirtualCursor *vc);
+
+// toggles the visibility of cursors

We still use /* */ style comments in the code-base for consistency everywhere.

@@ +321,3 @@
+ide_cursor_add_cursor (IdeCursor *self,
+                       guint      type)
+{

Always have precondition checks, so in this case:

 g_return_if_fail (IDE_IS_CURSOR (self));
 g_return_if_fail (type >= .. && <= ..);

@@ +335,3 @@
+                        gint       len)
+{
+  g_assert (IDE_IS_CURSOR (self));

For non-static functions, we use g_return_if_fail() or g_return_val_if_fail() instead of g_assert()

@@ +403,3 @@
+
+          if (gtk_text_iter_equal (&begin, &end))
+              gtk_text_buffer_backspace (buffer, &end, TRUE, gtk_text_view_get_editable (text_view));

2 space indentation, not 4

@@ +452,3 @@
+            ide_text_util_delete_line (text_view, count);
+          else
+            GTK_TEXT_VIEW_CLASS (G_OBJECT_GET_CLASS (text_view))->delete_from_cursor (text_view,

GTK_TEXT_VIEW_GET_CLASS (text_view)->delete_from_cursor

@@ +537,3 @@
+          ide_cursor_select (self, buffer, iter->data);
+
+          GTK_TEXT_VIEW_CLASS (G_OBJECT_GET_CLASS (text_view))->move_cursor (text_view,

GTK_TEXT_VIEW_GET_CLASS (text_view)->move_cursor

@@ +699,3 @@
+  g_assert (IDE_IS_SOURCE_VIEW (source_view));
+
+  g_signal_connect_after (source_view, "move-cursor", G_CALLBACK (ide_cursor_move_cursor), object);

This is probably where your segfault is coming from. Lots of signal connections that are not being disconnected.

I suggest using EggSignalGroup to do all of the connections, and then you can just free the signal group in finalize to disconnect everything.

@@ +723,3 @@
+                                       NULL);
+
+  g_clear_object (&search_settings);

Is there no g_autoptr() support for GtkSourceSearchSettings?

@@ +741,3 @@
+
+  g_clear_object (&self->search_context);
+  g_object_unref (self->highlight_tag);

We like to liberally use g_clear_object(), even when we know its non-NULL.

@@ +745,3 @@
+  if (self->cursors != NULL)
+    {
+      VirtualCursor *vc;

This can go inside the inner loop scope.

@@ +757,3 @@
+        }
+
+      g_list_free (self->cursors);

I like to clear the memory to NULL so that it is easy to tell when something was freed from within the debugger. So maybe, g_clear_pointer (&self->cursors, g_list_free);

@@ +784,3 @@
+ide_cursor_init (IdeCursor *self)
+{
+  self->highlight_tag = g_object_new (GTK_TYPE_TEXT_TAG,

Long term it would probably be nice to allow the colorschemes to style this. But fine for now.

::: libide/sourceview/ide-source-view.c
@@ +1306,3 @@
+  gtk_text_buffer_get_iter_at_mark (buffer, &insert, gtk_text_buffer_get_insert(buffer));
+  if (gtk_text_iter_equal (iter, &insert))
+  {

2 space indentation for {} scope indentation.

@@ +2666,3 @@
+        {
+          if (!ide_cursor_is_enabled (priv->cursor))
+            ide_cursor_add_cursor (priv->cursor, 2);

Instead of using integers like 2 for the type, add an enum to ide-cursor.h and use that instead. It's opaque as to what is going on otherwise.

@@ +2728,3 @@
+
+  if (event->button == GDK_BUTTON_PRIMARY && (event->state & GDK_CONTROL_MASK))
+  gboolean ret;

Indent is 2 spaces.

@@ +7133,3 @@
+                  G_TYPE_NONE,
+                  1,
+                  G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION,

With ide-cursor.h including an enum for the types, we should be able to use something like IDE_TYPE_CURSOR_TYPE instead of G_TYPE_UINT. All we need to do is ensure the type gets created for it in ide-enums.[ch].

To do that, we need to add the proper #include to ide-enums.c.tmpl and add it to the enums files in libide/Makefile.am so that it scans the file for enum types.

Created attachment 350554 [details] [review]
Multiple Cursor support in editor

changes you mentioned are done. Segfault when closing Builder after editing with multiple cursors are occurring very randomly. But, now I tested multiple times there are no segfaults when closing Builder.

Review of attachment 350554 [details] [review]:

I just went ahead and made these changes while doing the review, so I'll post an updated patch shortly.

::: data/keybindings/default.css
@@ +12,3 @@
   bind "<shift>F7" { "action" ("frame", "show-spellcheck", "1") };
   bind "<ctrl>f" { "action" ("frame", "find", "3") };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };

0 => column

@@ -11,2 +12,4 @@
   bind "<shift>F7" { "action" ("frame", "show-spellcheck", "1") };
   bind "<ctrl>f" { "action" ("frame", "find", "3") };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };
+  bind "<ctrl><shift>d" { "add-cursor" (2) };

2 => match

::: data/keybindings/emacs.css
@@ -55,2 +55,4 @@
                   "clear-snippets" ()
-                  "hide-completion" () };
+                  "hide-completion" ()
+                  "remove-cursors" () };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };

0 => column

@@ +57,3 @@
+                  "remove-cursors" () };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };
+  bind "<ctrl><shift>d" { "add-cursor" (2) };

2 => match

::: data/keybindings/vim.css
@@ +110,3 @@
   bind "<ctrl>equal" { "increase-font-size" () };
   bind "<ctrl>0" { "reset-font-size" () };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };

0 => column

@@ +111,3 @@
   bind "<ctrl>0" { "reset-font-size" () };
+  bind "<ctrl><shift>e" { "add-cursor" (0) };
+  bind "<ctrl><shift>d" { "add-cursor" (2) };

2 => match

::: libide/sourceview/ide-cursor.c
@@ +31,3 @@
+  GList                       *cursors;
+
+  gboolean                     overwrite;

guint overwrite : 1;

Our convention in the code base is to use unsigned to avoid signed errors, and bit-width of 1.

@@ +45,3 @@
+G_DEFINE_TYPE (IdeCursor, ide_cursor, G_TYPE_OBJECT)
+
+enum

Cuddle {

@@ +49,3 @@
+  PROP_0,
+  PROP_IDE_SOURCE_VIEW,
+  LAST_PROP

N_PROPS

@@ +661,3 @@
+  GtkTextView *text_view;
+  GtkTextBuffer *buffer;
+  g_autoptr(GtkSourceSearchSettings) search_settings;

Always assign autoptr/autofree to NULL

@@ +801,3 @@
+
+void
+ide_cursor_destruct (IdeCursor *self)

This should be put into a GObjectClass.dispose vfunc implementation to ensure it is always called. Then use g_object_run_dispose() to force disposal of the object.

Note that dispose can be called multiple times, so you need to be safe against NULL in this case.

::: libide/sourceview/ide-cursor.h
@@ +43,3 @@
+                                               gint       len);
+gboolean     ide_cursor_is_enabled            (IdeCursor *self);
+void         ide_cursor_destruct              (IdeCursor *self);

drop this, and use g_object_run_dispose()

::: libide/sourceview/ide-source-view.c
@@ +1781,3 @@
   egg_signal_group_set_target (priv->completion_providers_signals, NULL);
 
+  ide_cursor_destruct (priv->cursor);

if (priv->cursor != NULL)
  {
    g_object_run_dispose (G_OBJECT (priv->cursor));
    g_clear_object (&priv->cursor);
  }

::: libide/sourceview/ide-source-view.h
@@ -299,2 +299,5 @@
   void (*reset_font_size)             (IdeSourceView           *self);
   void (*begin_rename)                (IdeSourceView           *self);
+  void (*add_cursor)                  (IdeSourceView           *self,
+                                       guint                    type);
+  void (*remove_cursors)              (IdeSourceView           *self);

Normally, we would remove a couple of padding fields to coincide with this, but we aren't stable ABI yet so this is fine.

Created attachment 351392 [details] [review]
Multiple Cursor support in editor

New cursors can be added in 3 ways:
-By selcting text and pressing <ctrl><shift>e a new cursor in every row of
 selected text in same column will be created.
-By holding <ctrl> and clicking any where in editor.
-By selecting text and presing <ctrl><shift>d a new cursor will be created at
 next occurence of selected text.

All cursors can be removed by pressing Escape.

Attachment 351392 [details] pushed as 057b619 - Multiple Cursor support in editor