Because this bug is just a small aesthetic annoyance, consider setting its importance to "low".

Why does this depend on bug #667779. We need to manage the GMenu manually anyway, because it has a dynamic list of items.

I've rebased my branch to master.

Updated (and rebased) the branch again to fix some bugs and extract the dynamic menu sections more efficiently.

So there´s a big problem with the porting to gmenu: each window has its own, different menu (because the tabs menu differs), while with gtkapplication, there is only one, global, menu for all windows. Your patch ´solves´ this by changing the global menu to contain the focused window´s app menu on window focus change. This causes all windows to rebuild their gtkmenubar. I don´t think this is acceptable.

I can see two ways to solve this:

* Add a gtk_application_window_set_menubar that complements gtk_application_set_menubar in that the app window will use the global one if the per-window one is NULL. However, I think the gtk+ devs are unlikely to accept this?

* Remove the tabs menu. Instead, maybe have a down-arrow button with a tabs menu in the tabsbar, like firefox does.


This bug depends on bug 667779 because when a tab´s title changes, the tabs menu needs to be updated. Your patch entirely rebuilds the tabs menu on active tab´s title change (seems you forgot about inactive tabs?), which isn´t acceptable since tab titles can change very often. This is independent on the question above, since this needs to happen even if the tabs menu is a drop-down on a button in the tabsbar, since we cannot rebuild a menu while it is open.

I don´t like your patch removing the parameters from the actions, but since the bug about settings (action, param) pairs insensitive was WONTFIXed, I agree that it´s necessary. (It means that gmenu just lost its only plus over gtkaction*, ie. the parameters.)

I have more comments on the patch itself, but let´s discuss the issue above, first.

(In reply to Christian Persch from comment #4)
> I can see two ways to solve this:
> 
> * Add a gtk_application_window_set_menubar that complements
> gtk_application_set_menubar in that the app window will use the global one
> if the per-window one is NULL. However, I think the gtk+ devs are unlikely
> to accept this?

We could do that. It's always irked me that there's no way for an application to set different menus for different windows. I've been meaning to fix it as soon as enough people complain, but that hasn't happened yet. Apparently it's not that useful of a feature now that many applications are single-windowed and don't have menus at all anymore.

The only problem I'm seeing with this approach is osx support. I don't think they have per-window menus.


> * Remove the tabs menu. Instead, maybe have a down-arrow button with a tabs
> menu in the tabsbar, like firefox does.

I favor this approach. I don't know if many people use the tabs menu at all (I never do). If you're ok with this, I'll implement it.


> This bug depends on bug 667779 because when a tab´s title changes, the tabs
> menu needs to be updated. Your patch entirely rebuilds the tabs menu on
> active tab´s title change (seems you forgot about inactive tabs?), which
> isn´t acceptable since tab titles can change very often. This is independent
> on the question above, since this needs to happen even if the tabs menu is a
> drop-down on a button in the tabsbar, since we cannot rebuild a menu while
> it is open.

Ah, fair enough.  I don't think rebuilding the entire menu is prohibitively expensive, but it's certainly very ugly and I agree that we should solve it.


> I don´t like your patch removing the parameters from the actions, but since
> the bug about settings (action, param) pairs insensitive was WONTFIXed, I
> agree that it´s necessary. (It means that gmenu just lost its only plus over
> gtkaction*, ie. the parameters.)
> 
> I have more comments on the patch itself, but let´s discuss the issue above,
> first.

Thanks.

(In reply to Lars Uebernickel from comment #5)
> > * Remove the tabs menu. Instead, maybe have a down-arrow button with a tabs
> > menu in the tabsbar, like firefox does.
> 
> I favor this approach. I don't know if many people use the tabs menu at all
> (I never do). If you're ok with this, I'll implement it.

Fine with me.

Some comments on the patch itself:

-  g_setenv ("UBUNTU_MENUPROXY", "0", TRUE);
-  g_setenv ("NO_UNITY_GTK_MODULE", "1", TRUE);

Are there other mechanisms in place to make these modules nonoperational? Otherwise should not delete these but move them outside the #ifdef  ENABLE_DISTRO_PACKAGING.

+static char *
+escape_underscores (const char *name)

Since the only caller is now menu_append_numbered(), could combine that into one, using GString to build the label without multiple allocations... 

+  menubar = gtk_application_get_menubar (GTK_APPLICATION (app));
+  if (menubar == NULL)
+    return;
+
+  profiles = terminal_profiles_list_ref_children_sorted (app->profiles_list);
+
+  new_terminal_section = gtk_application_get_menu_by_id (GTK_APPLICATION (app), "new-terminal-section");
+  set_profile_section = gtk_application_get_menu_by_id (GTK_APPLICATION (app), "set-profile-section");
+  popup_set

Use a local variable to avoid the repeated GTK_APPLICATION cast. (Same in the other functions below.)

+          /* only connect if we haven't seen this profile before */
+          if (g_signal_handler_find (profile, G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DATA,
+                                     0, 0, NULL, terminal_app_update_profile_menus, app) == 0)
+            g_signal_connect_swapped (profile, "changed::" TERMINAL_PROFILE_VISIBLE_NAME_KEY,
+                                      G_CALLBACK (terminal_app_update_profile_menus), app);

A bit hidden to put this inside the update_profile_menus... maybe have another callback on children-changed that only manages this? I see you saved disconnecting them again which seems fine since the app determines the lifetime of the process, but maybe just add a comment here to document this?

+static void
+terminal_app_update_tabs_menu (TerminalApp *app)

As discussed above, should move the dynamic tabs menu section to a popup on the tabs bar. So I'm not commenting on the rest of the tabs menu related changes below.

+  g_signal_connect_swapped (application, "notify::active-window",
+                            G_CALLBACK (terminal_app_update_tabs_menu), application);
+

... which will make this obsolete.

   app->profiles_list = terminal_profiles_list_new ();
+  g_signal_connect_swapped (app->profiles_list, "children-changed",
+                            G_CALLBACK (terminal_app_update_profile_menus), app);
 
Hmm, you added the first terminal_app_update_profile_menus in terminal_app_startup()... why not move it here?

You do the update of the encodings menu in the class handler for the  And the ENCODINGS_LIST_CHANGED signal. But nobody should listen to this anymore since the menu is built in the app not in the window? If so, remove the signal and just update the encodings menu in terminal_app_encoding_list_notify_cb().

+  action = g_action_map_lookup_action (G_ACTION_MAP (window), name);
+  g_return_val_if_fail (action != NULL, NULL);

g_assert.

+action_edit_encodings_cb (GSimpleAction *action,
+                          GVariant      *parameter,
+                          gpointer       user_data)
+{
+  TerminalWindow *window = user_data;
+
+  terminal_app_edit_encodings (terminal_app_get (), GTK_WINDOW (window));
+}

NULL instead of GTK_WINDOW(window). (The param is obsolete and should probably just be removed.)

+  g_variant_get (parameter, "(uu)", &width, &height);
+  vte_terminal_set_size (VTE_TERMINAL (priv->active_screen), width, height);

Sanity-check width and height first (just to be sure)? Same ranges as profile-preferences.ui says for the default w/h, ie h=4..256, w=16..256 ?

-static gboolean
-terminal_window_accel_activate_cb 

Have you checked this removal doesn't regress bug 453193, bug 138609 and bug 559728 ?

(Note that those comments apply to the cumulative diff of the branch, not individual commits therein.)

(I've moved the tabs menu to the notebook now, in git master.)

(In reply to Lars Uebernickel from comment #5)

> The only problem I'm seeing with this approach is osx support. I don't think
> they have per-window menus.

I have no clue if they osx has per-window menus, but just for the record: indeed neither Terminal.app, nor iTerm2 have window-specific menu entries (such as a Tabs menu).

Support for non-free OSes is irrelevant.

Any chance of an updated patch addressing the issues in comment 7 ?

Fixed on master.

Please file *new* bugs for any issues you find with it.

I filed bug 794935 for the notebook popup being misplaced on Wayland.