GNOME Bugzilla – Bug 456959
Remove custom implementation of tab reordering and dragging
Last modified: 2010-10-05 08:43:03 UTC
Get rid of custom implementation of tab reordering and dragging. This implementation can be found in GeditNotebook. Use the API provided by gtk >= 2.11 for tab reordering, dragging between windows and creating windows.
Created attachment 91789 [details] [review] Use GtkNotebook API where possible This patches make gedit use the GtkNotebook API for the tabs reordering/dragging. It kills a big chunk of GeditNotebook. This patch alter config.ac to check dependency for gtk >=2.11
Created attachment 91806 [details] [review] Some cleaning in gedit-notebook.c/.h Change since last patch : * Remove unused signal and public methods declaration in gedit.h. * Remove gedit_notebook_reorder_tab and gedit_notebook_set_always_show_tabs public methods declaration and implementation * Remove unused GtkTooltips reference in sync_name Note that this patch (and the previous one) break GeditNotebook API (signals and public methods). I'll provide later a patch for backward compatibility.
Created attachment 91807 [details] [review] Fix a typo while connecting the 'swith-page' signal (line 198) Update since last patch : * replace "switch_page" by the correct signal identifier "switch-page" at notebook init. The connected handler is called 2 times for each switch ! Is that a normal behaviour ?
Note that 'foo-bar' and 'foo_bar' are equivalent for properties and signals (glib "normalizes" it).
(In reply to comment #4) > Note that 'foo-bar' and 'foo_bar' are equivalent for properties and signals > (glib "normalizes" it). > Ok, I was not aware of that. Thanks for the hint !
Created attachment 91817 [details] [review] Backward API compatibilty header This header diff applies on top of the previous patch to restore the API. You also need to apply the supplied diff for the source file (see next attachment).
Created attachment 91819 [details] [review] Backward API compatibilty code This patch must be applied on top of the previous patch. It restore API backward compatibility by exporting the previously deleted public methods (gedit_notebook_get/set_drag_and_drop_enabled, gedit_notebook_set_always_show_tabs and gedit_notebook_reorder_tab) and emits the old signals when receiving GtkNotebook ones.
Created attachment 91827 [details] [review] Updated patch * Coding style updates * Removing of the always_show_tabs field in GeditNotebook (we ALWAYS show them) * Avoid unesseray cast in signal handlers (typing the gpointer as GeditWindow)
Comment on attachment 91827 [details] [review] Updated patch Patch looks good to me, but it seems there was some misunderstandings about coding style, so I'll point them out one by one here :) >Index: gedit/gedit-documents-panel.c >=================================================================== >--- gedit/gedit-documents-panel.c (revision 5713) >+++ gedit/gedit-documents-panel.c (working copy) >@@ -649,10 +649,10 @@ > if (new_position > old_position) > new_position = MAX (0, new_position - 1); > >- gedit_notebook_reorder_tab (GEDIT_NOTEBOOK (nb), >- tab, >+ gtk_notebook_reorder_child (GTK_NOTEBOOK (nb), >+ GTK_WIDGET(tab), missing space before (tab) > new_position); >- >+ > panel->priv->is_reodering = FALSE; > } > trailing tab added on the empty line >Index: gedit/gedit-notebook.c >=================================================================== >--- gedit/gedit-notebook.c (revision 5713) >+++ gedit/gedit-notebook.c (working copy) >@@ -65,15 +65,8 @@ > > struct _GeditNotebookPrivate > { >- GList *focused_pages; >- GeditTooltips *title_tips; >- gulong motion_notify_handler_id; >- gint x_start; >- gint y_start; >- gint drag_in_progress : 1; >- gint always_show_tabs : 1; >- gint close_buttons_sensitive : 1; >- gint tab_drag_and_drop_enabled : 1; >+ GList *focused_pages; >+ gint close_buttons_sensitive : 1; > }; > Use tabs for indentation. Make sure that tabs are displayed as 8-spaces long so that things are aligned correctly. >- TABS_REORDERED, >- TAB_DETACHED, >- TAB_CLOSE_REQUEST, >- LAST_SIGNAL >+ TAB_CLOSE_REQUEST, >+ LAST_SIGNAL > }; > ditto >- >- gdk_window_get_origin (GDK_WINDOW (tab->window), >- &x_root, &y_root); >+ GObjectClass *object_class = G_OBJECT_CLASS (klass); > >- max_x = x_root + tab->allocation.x + tab->allocation.width; >- max_y = y_root + tab->allocation.y + tab->allocation.height; >+ object_class->finalize = gedit_notebook_finalize; > >- if (((tab_pos == GTK_POS_TOP) || >- (tab_pos == GTK_POS_BOTTOM)) && >- (abs_x <= max_x)) >- { >- return page_num; >- } >- else if (((tab_pos == GTK_POS_LEFT) || >- (tab_pos == GTK_POS_RIGHT)) && >- (abs_y <= max_y)) >- { >- return page_num; >- } >+ signals[TAB_CLOSE_REQUEST] = >+ g_signal_new ("tab-close-request", >+ G_OBJECT_CLASS_TYPE (object_class), >+ G_SIGNAL_RUN_LAST, >+ G_STRUCT_OFFSET (GeditNotebookClass, tab_close_request), >+ NULL, NULL, >+ g_cclosure_marshal_VOID__OBJECT, >+ G_TYPE_NONE, >+ 1, >+ GEDIT_TYPE_TAB); > >- ++page_num; >- } >- >- return AFTER_ALL_TABS; >+ g_type_class_add_private (object_class, sizeof(GeditNotebookPrivate)); > } same. } >- >-static gboolean >-button_press_cb (GeditNotebook *notebook, >- GdkEventButton *event, >- gpointer data) >+ GeditNotebook *dest, >+ GeditTab *tab, >+ gint dest_position) > { same. >- } >- } >+ g_return_if_fail (GEDIT_IS_NOTEBOOK (src)); >+ g_return_if_fail (GEDIT_IS_NOTEBOOK (dest)); >+ g_return_if_fail (src != dest); >+ g_return_if_fail (GEDIT_IS_TAB (tab)); > >- return FALSE; >+ /* make sure the tab isn't destroyed while we move it */ >+ g_object_ref (tab); >+ gedit_notebook_remove_tab (src, tab); >+ gedit_notebook_add_tab (dest, tab, dest_position, TRUE); >+ g_object_unref (tab); > } > more of the same :) > GtkWidget * >@@ -602,9 +127,11 @@ > GtkWidget * > gedit_notebook_new (void) > { >- return GTK_WIDGET (g_object_new (GEDIT_TYPE_NOTEBOOK, NULL)); >+ return GTK_WIDGET (g_object_new (GEDIT_TYPE_NOTEBOOK, NULL)); > } > same. >+/* CHECK : called twice for each switch >+ */ > static void > gedit_notebook_switch_page_cb (GtkNotebook *notebook, > GtkNotebookPage *page, >@@ -611,46 +138,26 @@ > guint page_num, > gpointer data) > { >- GeditNotebook *nb = GEDIT_NOTEBOOK (notebook); >- GtkWidget *child; >- GeditView *view; >+ GeditNotebook *nb = GEDIT_NOTEBOOK (notebook); >+ GtkWidget *child; >+ GeditView *view; > >- child = gtk_notebook_get_nth_page (notebook, page_num); >+ child = gtk_notebook_get_nth_page (notebook, page_num); > ditto. >- /* Remove the old page, we dont want to grow unnecessarily >- * the list */ >- if (nb->priv->focused_pages) >- { >- nb->priv->focused_pages = >- g_list_remove (nb->priv->focused_pages, child); >- } >+ /* Remove the old page, we dont want to grow unnecessarily >+ * the list */ >+ if (nb->priv->focused_pages) >+ { >+ nb->priv->focused_pages = >+ g_list_remove (nb->priv->focused_pages, child); >+ } > >- nb->priv->focused_pages = g_list_append (nb->priv->focused_pages, >- child); >+ nb->priv->focused_pages = g_list_append (nb->priv->focused_pages, >+ child); > same. beside 'child' should be aligned with nb->priv->focused_pages. >- gtk_notebook_set_show_tabs (GTK_NOTEBOOK (nb), show_tabs); >+ /* give focus to the view */ >+ view = gedit_tab_get_view (GEDIT_TAB (child)); >+ gtk_widget_grab_focus (GTK_WIDGET (view)); > } > ok, I guess you got the idea by now :) I'll stop here for \t vs 4 spaces > static void >@@ -656,35 +163,18 @@ > static void > gedit_notebook_init (GeditNotebook *notebook) > { >- notebook->priv = GEDIT_NOTEBOOK_GET_PRIVATE (notebook); >- >- notebook->priv->close_buttons_sensitive = TRUE; >- notebook->priv->tab_drag_and_drop_enabled = TRUE; >- >- gtk_notebook_set_scrollable (GTK_NOTEBOOK (notebook), TRUE); >- gtk_notebook_set_show_border (GTK_NOTEBOOK (notebook), FALSE); >- gtk_notebook_set_show_tabs (GTK_NOTEBOOK (notebook), FALSE); >- >- notebook->priv->title_tips = gedit_tooltips_new (); >- g_object_ref_sink (notebook->priv->title_tips); >- >- notebook->priv->always_show_tabs = TRUE; >+ notebook->priv = GEDIT_NOTEBOOK_GET_PRIVATE (notebook); > >- g_signal_connect (notebook, >- "button-press-event", >- (GCallback)button_press_cb, >- NULL); >- g_signal_connect (notebook, >- "button-release-event", >- (GCallback)button_release_cb, >- NULL); >- gtk_widget_add_events (GTK_WIDGET (notebook), >- GDK_BUTTON1_MOTION_MASK); >+ notebook->priv->close_buttons_sensitive = TRUE; >+ >+ gtk_notebook_set_scrollable (GTK_NOTEBOOK (notebook), TRUE); >+ gtk_notebook_set_show_border (GTK_NOTEBOOK (notebook), FALSE); >+ gtk_notebook_set_show_tabs (GTK_NOTEBOOK (notebook), TRUE); > >- g_signal_connect_after (G_OBJECT (notebook), >- "switch_page", >- G_CALLBACK (gedit_notebook_switch_page_cb), >- NULL); >+ g_signal_connect_after (G_OBJECT (notebook), >+ "switch-page", >+ G_CALLBACK (gedit_notebook_switch_page_cb), >+ NULL); > } > > static void >@@ -690,14 +180,12 @@ > static void > gedit_notebook_finalize (GObject *object) > { >- GeditNotebook *notebook = GEDIT_NOTEBOOK (object); >- >- if (notebook->priv->focused_pages) >- g_list_free (notebook->priv->focused_pages); >- >- g_object_unref (notebook->priv->title_tips); >+ GeditNotebook *notebook = GEDIT_NOTEBOOK (object); > >- G_OBJECT_CLASS (gedit_notebook_parent_class)->finalize (object); >+ if (notebook->priv->focused_pages) >+ g_list_free (notebook->priv->focused_pages); >+ \t on empty line. I also think that g_list_free handles null list fine, so if should be superflous (double check though). >+ G_OBJECT_CLASS (gedit_notebook_parent_class)->finalize (object); > } > > > static void > close_button_clicked_cb (GtkWidget *widget, >- GtkWidget *tab) >+ /* Set minimal size */ >+ g_signal_connect (hbox, "style-set", >+ G_CALLBACK (tab_label_style_set_cb), NULL); >+ when splitting function calls across lines put each argument on its own line, aligned. >+notebook_tab_added (GtkNotebook *notebook, >+ GtkWidget *tab, >+ guint page_num, >+ GeditWindow *window) No tabs when spacing out argument name and type. > { > GeditView *view; > GeditDocument *doc; >@@ -2538,9 +2536,9 @@ > GeditDocument *doc; > GtkTargetList *tl; > GtkAction *action; >- >+ > gedit_debug (DEBUG_WINDOW); >- >+ > g_return_if_fail ((window->priv->state & GEDIT_WINDOW_STATE_SAVING_SESSION) == 0); > No tabs on empty lines > ++window->priv->num_tabs; >@@ -2555,8 +2553,8 @@ > gtk_action_set_sensitive (action, > window->priv->num_tabs > 1); > >- view = gedit_tab_get_view (tab); >- doc = gedit_tab_get_document (tab); >+ view = gedit_tab_get_view (GEDIT_TAB(tab)); >+ doc = gedit_tab_get_document (GEDIT_TAB(tab)); > spaces before (tab > /* IMPORTANT: remember to disconnect the signal in notebook_tab_removed > * if a new signal is connected here */ >@@ -2636,8 +2634,9 @@ > } > > static void >-notebook_tab_removed (GeditNotebook *notebook, >- GeditTab *tab, >+notebook_tab_removed (GtkNotebook *notebook, >+ GtkWidget *tab, >+ guint page_num, > GeditWindow *window) > { > GeditView *view; >@@ -2643,7 +2642,7 @@ > GeditView *view; > GeditDocument *doc; > GtkAction *action; >- >+ > gedit_debug (DEBUG_WINDOW); > > g_return_if_fail ((window->priv->state & GEDIT_WINDOW_STATE_SAVING_SESSION) == 0); >@@ -2650,8 +2649,8 @@ > > --window->priv->num_tabs; > >- view = gedit_tab_get_view (tab); >- doc = gedit_tab_get_document (tab); >+ view = gedit_tab_get_view (GEDIT_TAB(tab)); >+ doc = gedit_tab_get_document (GEDIT_TAB(tab)); > ditto. > g_signal_handlers_disconnect_by_func (tab, > G_CALLBACK (sync_name), >@@ -2752,8 +2751,10 @@ > } > > static void >-notebook_tabs_reordered (GeditNotebook *notebook, >- GeditWindow *window) >+notebook_tabs_reordered (GtkNotebook *notebook, >+ GtkWidget *child, >+ guint page_num, >+ GeditWindow *window) > { > update_documents_list_menu (window); > update_next_prev_doc_sensitivity_per_window (window); >@@ -2761,24 +2762,30 @@ > g_signal_emit (G_OBJECT (window), signals[TABS_REORDERED], 0); > } > >-static void >-notebook_tab_detached (GeditNotebook *notebook, >- GeditTab *tab, >- GeditWindow *window) >+static GtkNotebook* >+notebook_tab_detached (GtkNotebook *source, >+ GtkWidget *page, >+ gint x, >+ gint y, >+ GeditWindow *window) > { > GeditWindow *new_window; > >+ /* If the tab s the only one in the source notebook, then >+ * we forbid the drag operation >+ */ >+ if (gtk_notebook_get_n_pages (source) < 2) >+ return NULL; >+ > new_window = clone_window (window); >- >- gedit_notebook_move_tab (notebook, >- GEDIT_NOTEBOOK (_gedit_window_get_notebook (new_window)), >- tab, 0); >- >+ > gtk_window_set_position (GTK_WINDOW (new_window), > GTK_WIN_POS_MOUSE); >- >+ > gtk_widget_show (GTK_WIDGET (new_window)); >-} >+ >+ return GTK_NOTEBOOK(new_window->priv->notebook); >+} space before (new > > static void > notebook_tab_close_request (GeditNotebook *notebook, >@@ -3187,7 +3194,12 @@ > > gedit_debug_message (DEBUG_WINDOW, "Create gedit notebook"); > window->priv->notebook = gedit_notebook_new (); >- gtk_paned_pack1 (GTK_PANED (window->priv->vpaned), >+ /* Set the current app pointer as the notebook group, >+ * so we can move tabs between windows of the same app */ >+ gtk_notebook_set_group (GTK_NOTEBOOK(window->priv->notebook), >+ gedit_app_get_default ()); >+ there's an extra space in the comment before 'so'. missing space before (window align gedit_app_get_default () with GTK_NOTEBOOK
Created attachment 91873 [details] [review] Updated patch Seems like my editor like space chars, i should use gedit ;) Indentation and code style fixes : * Use tabs instead of space chars * remove \t on empty lines * align function args * separate function names and args opening braklets Hope it will suits you. Next step is making the whole window as drop target for tabs (currently half-working, or half-broken, if you prefer ;))
Created attachment 91891 [details] [review] Updated patch, set the window as drop target for tabs. Updates since last patch : * add GTK_NOTEBOOK_TAB as allowed drop type * implement the logic for tab dropping in the drag_data_received_cb callback. Previous behaviour (with previous patch applied) : only the notebook is a target for tab dropping. Expected behaviour : the whole window is a target for tab dropping. Resulting behaviour : The notebook and the view (the document text area) are drop targets for tabs. As you can see the is a delta between expected and resulting behaviour and I cannot identify what's going wrong : the whole window should accept GTK_NOTEBOOK_TAB drop type and the logic is implemented in the handler for the signal "drag-data-received"... Need help !
Created attachment 91904 [details] [review] Updated patch Changes since last patch : * break the "drag-data-received" signal handler into 2 functions, one for uris, the second for tabs * delegate tab dropping to the GtkNotebook default handler for the "drag-data-received" signal * translate tabdrop coordinates into the notebook coord system (allows correct ordering) * rename the drag type id to TARGET_NOTEBOOK_TAB for consistency sake
I played a bit with the patch and noticed a couple of slightly annoying things (not showstoppers), though I am not sure they are fixable or not. - Maybe we should prevent the drag when there is only one tab and one window - when dragging the tab on another with the old notebook the tab attached right away to the notebook of the target window while now there is no visual indication difference between dropping on another gedit window and detaching a tab.
(In reply to comment #13) Hi pbor, > I played a bit with the patch and noticed a couple of slightly annoying things > (not showstoppers), though I am not sure they are fixable or not. > > - Maybe we should prevent the drag when there is only one tab and one window Yeah, we can work around it using the gtk_notebook_set_tab_detachable method. For now, any tab created is set as detacheable and reorderable at creation time. We can set tab undetachable by default, then as soon as a new tab or window is created reset it draggable. I'll have a look at it tomorrow (hell, it is friday night ;)) If app->windows has only one entry and the notebook only one tab > - when dragging the tab on another with the old notebook the tab attached > right away to the notebook of the target window while now there is no visual > indication difference between dropping on another gedit window and detaching a > tab. Do you mean visual feedback when dragging the tab after it has been detached from it notebook ? If so, I completely agree, there should be a clue of what's happening and where the tab is gonna 'fall' when dropped. I think we may need to attach a callback to the ::drag-motion signal that proxy it to the notebook after coord translation the same way we did for ::drag_data_received. This way we'll have the same effect (sliding tab) as when a tab is reordered within the same notebook. What do you think ?
I've changed the workings of gedit dnd a bit which interferes with this patch. Nothing major as far as I can see, but the patch needs to be updated according to these changes. You'll see that the view now handles dropping files and emits a new signal 'drop-uris' when uris have been dropped in the view.
Created attachment 171330 [details] [review] gtknotebook dnd v1 So here we go.
Created attachment 171617 [details] [review] gtknotebook dnd v2 Version without the global hook. I think there is also a bug when you detach a tab, the window is created and the tab can't be closed for some reason.
Created attachment 171632 [details] [review] gtknotebook dnd v3 Fixed minor issues pointed by irc. The previous bug already persist.
Created attachment 171719 [details] [review] gtknotebook dnd v4 Fixed the previous bug but found out another: ** (gedit:28039): CRITICAL **: gedit_window_get_active_document: assertion `GEDIT_IS_WINDOW (window)' failed
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.