I'm halfway through with a patch, but am having troubles with appending a menu-item to the default GtkNotebook popup menu.

I doubt that it is possible to append it so either leave (and disable) the menu for now or try to generate the list of tabs yourself (as done in the Documents menu).

Created attachment 241887 [details] [review]
Patch: Close Other Tabs Enhancement (bgo#698036)

Here's a patch. On rightclicking a GtkNotebook tab, a menu appears with a "Close Other Tabs" option, which works as stated.

Review of attachment 241887 [details] [review]:

Thank you for your patch.

I think the function anjuta_docman_create_tab_popup could be declared as static or do you plan to use it somewhere else?

In the on_notebook_tab_btnpress function, why do you call anjuta_docman_set_current_document? It wasn't done before and I don't think that your patch change this.

In the on_notebook_tab_btnpress function, you call anjuta_docman_create_tab_popup even when you don't need it (if event->button != 3), I would be better to inside the condition.

In the on_notebook_tab_btnpress function, why do you set tab_pressed to TRUE? It's change comparing to the previous existing code.

Then, the close other function is useful but I would like to keep the previous function allowing to switch other notebook pages. As Johannes has told you, it's probably not possible to add your item to the default menu, so I think you need to recreate a complete menu. It's already done in the document manager plugin so we could probably reuse the same code.

Review of attachment 241887 [details] [review]:

Another thing, due to recent changes, your patch cannot be applied in the master version, could you update it.

> In the on_notebook_tab_btnpress function, why do you call
> anjuta_docman_set_current_document? It wasn't done before and I don't think
> that your patch change this.
I was setting the tab right clicked as the current document because I am using the pre-built action ActionFileCloseOther, which has been implemented for the Documents menu and is used to close all unsaved tabs other than the current.

If I am not to use this, I need to modify the on_close_other_file_activate method in action-callbacks.c such that if a tab has been rightclicked, the curr_doc variable is set to that tab (even though that tab is not the currently active tab) and then the method continues.

I tried doing this but cannot find a way to tell it that the method got activated because of a right click on the tab, and since there is no AnjutaDocmanPage in action-callbacks.c, I cannot use it to figure out which tab was clicked.

I have the following alternatives:

1. I can find out which tab was rightclicked from inside the on_notebook_tab_btnpress method in anjuta-docman.c, which I am able to, but I don't know how to pass that information to the on_close_other_file_activate in action-callbacks.c.
OR
2. Use the get_current_focus_widget method from action-callbacks.c and find out which tab was rightclicked. But once I have got the widget, how do I retrieve pages from it?


Also, I thought of adding another variable gboolean tab_rightclicked to AnjutaDocmanPriv structure in anjuta-docman.c, but again, that's not accessible from action-callbacks.c.

(In reply to comment #7)
> I was setting the tab right clicked as the current document because I am using
> the pre-built action ActionFileCloseOther, which has been implemented for the
> Documents menu and is used to close all unsaved tabs other than the current.

Ok, thank I understand the issue. Your current solution is working and is easier to implement but I think it would be better to not change the current document as soon as you display the menu.


> 1. I can find out which tab was rightclicked from inside the
> on_notebook_tab_btnpress method in anjuta-docman.c, which I am able to, but I
> don't know how to pass that information to the on_close_other_file_activate in
> action-callbacks.c.

You could store this information inside the document manager.


> 2. Use the get_current_focus_widget method from action-callbacks.c and find
> out which tab was rightclicked. But once I have got the widget, how do I
> retrieve pages from it?

I'm not sure it's working because you need to change the document after displaying the menu when Close All Other is clicked. At that time, I think the menu has the focus and not the document tab.


> Also, I thought of adding another variable gboolean tab_rightclicked to
> AnjutaDocmanPriv structure in anjuta-docman.c, but again, that's not
> accessible from action-callbacks.c.

You can add access function to get this information in action-callbacks.c. The issue is that if you trigger the ActionFileCloseOther from the main menu it could be strange if it keeps the last page where you right clicked on the tab. I think using the current page is better in this case. You can handle this by clearing this tab_rightclicked boolean when the menu is closed and use the current document as a backup, I don't know how easily you can do it.

Instead of adding a boolean in each page, you can keep a AnjutaDocmanPage pointer inside AnjutaDocman to keep the currently focused page. But again you have to set it back to the current page when the menu is closed.


Another solution could be to create the menu item "manually" and not use the ActionFileCloseOther. In this case you can use the activate signal of the menu item and use a AnjutaDocmanPage as callback argument set up when you create the menu. In this case you don't need additional variable inside the document manager.

I would like to keep the same items of the default notebook menu, so this solution is perhaps better but I think you will have to recreate all these items too.

Created attachment 243621 [details]
Query for g_signal_connect

I have succeeded in generating a popup menu with all the open tabs as menuitems, and the close others option using the attached method. I need some help with passing parameters to the G_CALLBACK s. I want to pass the menuitem clicked along with the docman to on_nth_item_tab_popup_click. Please help me figure out the parameters for the g_signal_connect.

You can pass only a single pointer to the callback in g_signal_connect. 

If you really need to pass more information, you need to allocate some memory for a structure and put all your data inside. You will have to free in the callback.

Most of the time, you can find a object with all the needed information. By example if you pass a AnjutaDocmanPage, you can perhaps get the document manager by looking at its parent.

Created attachment 243680 [details] [review]
Patch: Close Other Tabs Enhancement (bgo#698036)

Patch involves custom generation of the popup menu via a anjuta_docman_create_tab_popup method added in anjuta-docman.c. I have also added a variable gint nth_tab_pressed to know which tab has been clicked.

Review of attachment 243680 [details] [review]:

Good, this patch implements everything and is working fine but I still have several requests.


* I think it's better to no declare static functions if it possible by reordering it. In your patch, you don't need to declare anjuta_docman_create_tab_popup, on_close_other_file_activate_from_popup and on_tab_popup_clicked if you put tanjuta_docman_create_tab_popup after the two others and before on_notebook_tab_btnpress.


* It's better to declare variable in the smallest scope as possible. So instead of
	GtkMenu *popup_menu;
	GList *node;
	gint n;

 	if (event->type == GDK_BUTTON_PRESS && event->button != 3)	/* right-click is for menu */
 		docman->priv->tab_pressed = TRUE;
 
	if (event->type == GDK_BUTTON_PRESS && event->button == 3)
	{
		AnjutaDocmanPage *page;
		popup_menu = anjuta_docman_create_tab_popup (docman);
		for (n = 0, node = docman->priv->pages; node != NULL; node = g_list_next (node), n++)
		{
			page = (AnjutaDocmanPage *) node->data;
You can write
 	if (event->type == GDK_BUTTON_PRESS && event->button != 3)	/* right-click is for menu */
 		docman->priv->tab_pressed = TRUE;
 
	if (event->type == GDK_BUTTON_PRESS && event->button == 3)
	{
		GtkMenu *popup_menu;
		GList *node;
		gint n;

		popup_menu = anjuta_docman_create_tab_popup (docman);
		for (n = 0, node = docman->priv->pages; node != NULL; node = g_list_next (node), n++)
		{
			AnjutaDocmanPage *page;
			page = (AnjutaDocmanPage *) node->data;


* When you add a string displayed in the user interface, you need to surround it by _() to mark it as a translatable string. By example menuitem = gtk_menu_item_new_with_label (_("Close Others"));


* The compiler display a few warnings with your patch, it's better to fix them. You have by example
static void on_tab_popup_clicked (GtkWidget* widget, AnjutaDocman* docman)
{
	/* functions same as the default GtkNotebook's popup,  */
	AnjutaDocmanPage *page;
	gchar *tab_name;
	gchar *page_label;
instead of 
static void on_tab_popup_clicked (GtkWidget* widget, AnjutaDocman* docman)
{
	/* functions same as the default GtkNotebook's popup,  */
	AnjutaDocmanPage *page;
	const gchar *tab_name;
	const gchar *page_label;
And a few missing cast to GTK_MENU_SHELL by example gtk_menu_shell_append (GTK_MENU_SHELL (menu), menuitem);


* Indeed, you would like to pass to your menu item callback functions, two informations: a pointer to the document manager and the corresponding page but there is only one argument. It's a common issue, but there is a function in the GtkMenu menu API which can help you: gtk_menu_attach_to_widget. You can attach your menu to the document manager object. And then in your callback function you can retrieve it by getting the parent of the menu item (your menu) and use gtk_menu_get_attach_widget. Then, you can use the callback data to pass the current document or page. By example

static void on_close_other_file_activate_from_popup (GtkWidget* widget, IAnjutaDocument *doc)
{
	GtkWidget *parent;
	AnjutaDocman *docman;

	parent = gtk_widget_get_parent (widget);
	docman = ANJUTA_DOCMAN (gtk_menu_get_attach_widget (GTK_MENU (parent)));

	anjuta_docman_set_current_document (docman, doc);
	on_close_other_file_activate (NULL, docman->priv->plugin);
}

By the way, for the close_other_file, I think it's better to reuse the already existing function on_close_other_file_activate. When the menu is clicked you can change the current page as the other will be removed anyway.

* Finally a last more difficult point, There is an issue in your code because you never destroy the menu that you have created, after selecting an item the menu is only hidden but it is still existing. It's not very easy to destroy though. One solution is to keep a pointer on this menu in the document manager object, so you can reuse a previous menu if it exist or destroy the previous one before creating a new one. Another solution is to call gtk_widget_destroy on the menu item when it is deactivated. The issue is that if you call gtk_widget_destroy immediately, the menu will be destroyed before calling the activate callback of the selected menu item. So you need to call it later in an idle handler which make the code more cumbersome. By example:

static gboolean
on_idle_gtk_widget_destroy (gpointer user_data)
{
	gtk_widget_destroy (GTK_WIDGET (user_data));
	return FALSE;
}

static void
on_menu_deactivate (GtkMenuShell *menushell,
                    gpointer      user_data)
{
	g_idle_add (on_idle_gtk_widget_destroy, menushell);
}

static GtkMenu*
anjuta_docman_create_tab_popup (AnjutaDocman *docman, IAnjutaDocument *doc)
{
	GtkWidget *menu;

	menu = gtk_menu_new ();
	g_signal_connect (menu, "deactivate", G_CALLBACK (on_menu_deactivate), NULL);

Created attachment 243763 [details] [review]
Patch: Close Other Tabs Enhancement (bgo#698036)

Thanks for the help.

> * I think it's better to no declare static functions if it possible by
> reordering it. In your patch, you don't need to declare
> anjuta_docman_create_tab_popup, on_close_other_file_activate_from_popup and
> on_tab_popup_clicked if you put tanjuta_docman_create_tab_popup after the two
> others and before on_notebook_tab_btnpress.

I have removed the anjuta_docman_create_tab_popup and add the code to on_notebook_tab_btnpress, but I think it will be better if I leave on_close_other_file_activate and on_tab_popup_clicked because they are needed to connect to the "activate" signal emitted by the menuitems.

I was using the predefined on_close_other_activate before, but the problem with it is that it requires me to pass a GtkAction, and so I cannot use it from anjuta-docman.c . Earlier, I was adding the menuitem from the xml file, and had connected it to ActionFileCloseOther but now, I am building the menu entirely from anjuta-docman.c . Is there any way to connect this menuitem to ActionFileCloseOther already in action-callbacks.c? Hence, I figured that writing a new function on_close_other_activate_from_popup would be the best thing to do.

I have fixed the warnings, added the destroy functions, and changed the scope of variables.

(In reply to comment #13)
> I have removed the anjuta_docman_create_tab_popup and add the code to
> on_notebook_tab_btnpress, but I think it will be better if I leave
> on_close_other_file_activate and on_tab_popup_clicked because they are needed
> to connect to the "activate" signal emitted by the menuitems.

I have not asked to remove the anjuta_docman_create_tab_popup function. I think it is fine to put the code inside a function instead of putting it in on_notebook_tab_btnpress. I have only asked you to reorder your 3 functions to avoid the functions declaration, so remove:
 static GtkMenu * anjuta_docman_create_tab_popup ();
 static void on_close_other_file_activate_from_popup (GtkWidget* widget, AnjutaDocman* docman);
 static void on_tab_popup_clicked (GtkWidget* widget, AnjutaDocman* docman);

And just add in the file on_close_other_file_activate_from_popup and on_tab_popup_clicked before anjuta_docman_create_tab_popup.


> I was using the predefined on_close_other_activate before, but the problem
> with it is that it requires me to pass a GtkAction, and so I cannot use it
> from anjuta-docman.c

You can pass a NULL instead of a GtkAction pointer. The GtkAction pointer is not used in the function and it's already done in some other part of the code. It is not perfect but I think it's better than duplicating the code.


Finally, you have kept the nth_tab_pressed variable in the document manager instead of my proposal. That's fine but why?

Created attachment 243816 [details] [review]
Patch: Close Other Tabs Enhancement (bgo#698036)

I have restructured the code and am not declaring the functions now. But why is it not good to do so? 
I have changed the scope of the variables used, and am now using the predefined on_close_other_file_activate; Also, variable nth_tab_pressed has been removed.

(In reply to comment #15)
> I have restructured the code and am not declaring the functions now. But why is
> it not good to do so? 

I think it's better because it's one line less, you don't have to update it if you change the function name. Moreover, it forces you to order your functions so you know that you have to go backward to find them. Now, someone else could have a different opinion.


> I have changed the scope of the variables used, and am now using the predefined
> on_close_other_file_activate; Also, variable nth_tab_pressed has been removed.

Ok, but note I have only asked you why you have kept this nth_tab variable, I haven't asked you to use my proposal. You often have several solutions with different advantages and disadvantages. I think it's important to know why you have chosen one and sometime there are some disagreements.


Anyway, I think your patch is good enough now. I still have two remarks but I can fix them myself.

* I cannot apply your latest patch alone, I need the first one. You can easily merge them with git using git rebase -i.

* You have used gtk_menu_get_attach_widget in the on_close_other_file_activate_from_popup callback to get the document manager but you could use the same trick in on_tab_popup_clicked and pass directly the right document to each menu item, so you don't have to search for it again in the callback.

> Ok, but note I have only asked you why you have kept this nth_tab variable, I
> haven't asked you to use my proposal. You often have several solutions with
> different advantages and disadvantages. I think it's important to know why you
> have chosen one and sometime there are some disagreements.

I had kept it before because it didn't require me to pass two arguments to the callback function; I did not need to attach the docman to the menu and then pass it. But now, I think it is better not to modify the AnjutaDocmanPriv if the same thing can be done in a different way.

> Anyway, I think your patch is good enough now. I still have two remarks but I
> can fix them myself.
> 
> * I cannot apply your latest patch alone, I need the first one. You can easily
> merge them with git using git rebase -i.
> 
> * You have used gtk_menu_get_attach_widget in the
> on_close_other_file_activate_from_popup callback to get the document manager
> but you could use the same trick in on_tab_popup_clicked and pass directly the
> right document to each menu item, so you don't have to search for it again in
> the callback.

Alright, I'll keep these things in mind from now on. Thanks for the help.