Created attachment 349106 [details] [review]
notebook: close tab with middle click

Review of attachment 349106 [details] [review]:

The commit message needs a bit more work.

"All tabs based applications have this possibility" -> how do you know that? :) How about: "In other applications such as browsers tabs can be closed using middle click."

The commit message does not need to mention the functions you used. People can see that by looking at your code. You should focus on explaining what you did and how you did it: "Implement this by emitting a tab close request when a user middle clicks a tab."

::: src/nautilus-notebook.c
@@ +162,3 @@
     }
 
+if (event->type == GDK_BUTTON_PRESS &&

This line is unindented.

@@ +166,3 @@
+        (event->state & gtk_accelerator_get_default_mod_mask ()) == 0)
+    {
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));

This line is too long.
Also, the first parameter for gtk_notebook_get_nth_page has to be of type GtkNotebook *. You should cast it because right now the patch introduces a compile warning.

@@ +168,3 @@
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));
+
+        if (tab_clicked == -1)

This check should be done before calling g_signal_emit.

@@ +171,3 @@
+        {
+            /* consume event, so that we don't pop up the context menu when
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));

This comment is not relevant.

@@ +176,3 @@
+        }
+
+        if (tab_clicked == -1)

This comment is no longer relevant. Be more careful when copy-pasting code!
Also, maybe you should mark the event as consumed here, since you are going to close the tab anyway.

Review of attachment 349106 [details] [review]:

The commit message needs a bit more work.

"All tabs based applications have this possibility" -> how do you know that? :) How about: "In other applications such as browsers tabs can be closed using middle click."

The commit message does not need to mention the functions you used. People can see that by looking at your code. You should focus on explaining what you did and how you did it: "Implement this by emitting a tab close request when a user middle clicks a tab."

::: src/nautilus-notebook.c
@@ +162,3 @@
     }
 
+if (event->type == GDK_BUTTON_PRESS &&

This line is unindented.

@@ +166,3 @@
+        (event->state & gtk_accelerator_get_default_mod_mask ()) == 0)
+    {
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));

This line is too long.
Also, the first parameter for gtk_notebook_get_nth_page has to be of type GtkNotebook *. You should cast it because right now the patch introduces a compile warning.

@@ +168,3 @@
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));
+
+        if (tab_clicked == -1)

This check should be done before calling g_signal_emit.

@@ +171,3 @@
+        {
+            /* consume event, so that we don't pop up the context menu when
+        g_signal_emit (notebook, signals[TAB_CLOSE_REQUEST], 0, gtk_notebook_get_nth_page (notebook, tab_clicked));

This comment is not relevant.

@@ +176,3 @@
+        }
+
+        if (tab_clicked == -1)

This comment is no longer relevant. Be more careful when copy-pasting code!
Also, maybe you should mark the event as consumed here, since you are going to close the tab anyway.

Review of attachment 349106 [details] [review]:

::: src/nautilus-notebook.c
@@ +163,3 @@
 
+if (event->type == GDK_BUTTON_PRESS &&
+        event->button == 2 &&

GDK_BUTTON_MIDDLE

@@ +173,3 @@
+             * the mouse if not over a tab label
+             */
+            return TRUE;

Try to use GDK_EVENT_STOP and GDK_EVENT_PROPAGATE.

Design wise this topic is something we avoided in the past. As opposed to what the first commit message says, no GNOME app implements this behavior. So this need to be checked with designers.

Discussion continues on gitlab https://gitlab.gnome.org/GNOME/nautilus/issues/47

*** Bug 687267 has been marked as a duplicate of this bug. ***