Created attachment 367856 [details] [review]
Fix

Let's squeeze in another bug here, because its patch pretty much reverts the previous :)

There's been a couple of reports to Ubuntu that the terminal's menubar has two Terminal entries, and duplicates some of the features. Indeed, the terminal's menu goes like this, no matter if shown in the desktop's topmost menubar or in the window titlebar:

  Terminal File Edit View Search Terminal Help

That's because the first one is the appmenu, the rest is the menubar. Towards the user, there's no distinction between them. Also I can't recall any other app doing this.

So how about this? If the shell shows both things, there's no need for the appmenu (it's more confusing than useful).

Created attachment 367857 [details] [review]
Fix, v2

Fix both issues in a single patch.

Created attachment 367889 [details] [review]
Fix v3

Like v2 (both fixes in a single patch), updated against current HEAD.

(In reply to Egmont Koblinger from comment #0)
> Most gnome apps have moved their application menu from the shell's top bar
> to the window bar. Next to the standard 3 buttons (close, maximize,
> minimize) there's an app-specific icon displayed which opens the appmenu.
>  
> In g-t it's not present since it's explicitly disabled in the source.

This part wasn't by design, no. The not installing the app menu was to make absolutely sure it's not put inside the window (as happens when the shell doesn't show the appmenu). However this is *also* ensured by setting the show-menubar property of GtkAppplicationWindow to FALSE, so after ensure all our app windows have that set, it should be safe to keep the appmenu installed. (I just checked and it appears they all do.)
 
> Is this by design (why?) or by mistake? Shall we bring it back?

Yes, we should. If the user sets the pref for putting the appmenu in the window decorations (which is the *default* even), that must work.
 
> (Mind you, this happens to be a good workaround against bug 787470 /
> https://bugs.launchpad.net/ubuntu/+source/gnome-terminal/+bug/1724250.)

Well, this is an icon theme bug, so we really shouldn't work around it like this :-)

(In reply to Egmont Koblinger from comment #2)
> Let's squeeze in another bug here, because its patch pretty much reverts the
> previous :)
> 
> There's been a couple of reports to Ubuntu that the terminal's menubar has
> two Terminal entries, and duplicates some of the features. 

Yes, that's also what happens if you set both the menubar and appmenu properties of a GtkApplication, the shell doesn't show the app menu, and you app window has show-menubar = TRUE.

> Indeed, the
> terminal's menu goes like this, no matter if shown in the desktop's topmost
> menubar or in the window titlebar:
> 
>   Terminal File Edit View Search Terminal Help
> 
> That's because the first one is the appmenu, the rest is the menubar.
> Towards the user, there's no distinction between them. Also I can't recall
> any other app doing this.

That's probably because few other apps uses GtkApplication::menubar, where this will happen automatically, as above.

> So how about this? If the shell shows both things, there's no need for the
> appmenu (it's more confusing than useful).

Conceptually, this seems wrong to me. But since this should work out to the right thing on both gnome-shell and unity, and fixes a bug on the latter, let's do it :-)

However the condition doesn't match what the comment describes?
+ if (shell_shows_appmenu && shell_shows_menubar)
  [set appmenu to NULL]

Shouldn't that be more like
+ if (!shell_shows_appmenu || shell_shows_menubar)
  [set appmenu to NULL]

?

(In reply to Christian Persch from comment #5)

> Yes, we should. If the user sets the pref for putting the appmenu in the
> window decorations (which is the *default* even), that must work.

Might be GNOME's or Fedora's default, not Ubuntu's :-) It shows the appmenu at the top-left of the desktop by default, it takes a gnome-tweak-tool change to place it in the window title/headerbar as the 4th button.

> > (Mind you, this happens to be a good workaround against bug 787470 /
> > https://bugs.launchpad.net/ubuntu/+source/gnome-terminal/+bug/1724250.)
> 
> Well, this is an icon theme bug, so we really shouldn't work around it like
> this :-)

I'd rather say gnome-shell/mutter bug than icon theme bug, but it's irrelevant. I agree we shouldn't try to work it around, and actually my proposed patch doesn't, the overall experience is still buggy.

> > There's been a couple of reports to Ubuntu that the terminal's menubar has
> > two Terminal entries, and duplicates some of the features. 
> 
> Yes, that's also what happens if you set both the menubar and appmenu
> properties of a GtkApplication, the shell doesn't show the app menu, and you
> app window has show-menubar = TRUE.

Is this a desired user experience though? Do you have examples of apps doing this? The appmenu and the menubar right next to each other, as if they were one menu, doesn't make too much sense to me.

> >   Terminal File Edit View Search Terminal Help
> > 
> > That's because the first one is the appmenu, the rest is the menubar.
> > Towards the user, there's no distinction between them. Also I can't recall
> > any other app doing this.
> 
> That's probably because few other apps uses GtkApplication::menubar, where
> this will happen automatically, as above.

So do you mean it's okay and we should leave it this way, with the two Terminal menus?

> > So how about this? If the shell shows both things, there's no need for the
> > appmenu (it's more confusing than useful).
> 
> Conceptually, this seems wrong to me. But since this should work out to the
> right thing on both gnome-shell and unity, and fixes a bug on the latter,

Which bug do you refer to? The visual glitch with the huge icon? That's a bug in gnome-shell, not unity!

> let's do it :-)
> 
> However the condition doesn't match what the comment describes?

My comment ("the shell shows *both* things") clearly matches my code ((shell_shows_this && shell_shows_that)) and not yours.

> + if (shell_shows_appmenu && shell_shows_menubar)
>   [set appmenu to NULL]
> 
> Shouldn't that be more like
> + if (!shell_shows_appmenu || shell_shows_menubar)
>   [set appmenu to NULL]

Let's have a bit of fun with line drawing chars :)

                 ║ shell-menubar │ no-shell-menubar
═════════════════╬═══════════════╪══════════════════
shell-appmenu    ║     unity     │       g-s-1
─────────────────╫───────────────┼──────────────────
no-shell-appmenu ║       ?       │       g-s-2

My patch disables the appmenu for the "unity" slot only.

Your patch disables the appmenu for "unity", "?" and "g-s-2" which (for me) is the nondefault setup of gnome-shell, you say it's the default, where the appmenu is present as the 4th window decoration button.

In other words, your suggestion addresses comment 2, but leaves the behavior of comment 0 unchanged.

Wait... I guess I messed up something... My mind is just about to explode...

No, I didn't mess it up 2 comments above.

What I missed is: The giant icon artifact in the "g-s-2" field of the table is present in Ubuntu Artful with g-t _before_ the GMenu port, but not with current HEAD (as the code currently disables appmenu for the bottom row of the table).

In other words, the GMenu port accidentally removed the 4th button, and as such, provided a workaround for that huge icon bug.

My patch brings back the 4th button, and with that brings back the huge icon bug by no longer disabling appmenu for the bottom row. Your patch doesn't.

(In reply to Egmont Koblinger from comment #6)
> (In reply to Christian Persch from comment #5)
> 
> > Yes, we should. If the user sets the pref for putting the appmenu in the
> > window decorations (which is the *default* even), that must work.
> 
> Might be GNOME's or Fedora's default, not Ubuntu's :-) 

org.gnome.desktop.wm.preferences.gschema.xml:
    <key name="button-layout" type="s">
      <default>'appmenu:close'</default>

> It shows the appmenu
> at the top-left of the desktop by default, it takes a gnome-tweak-tool
> change to place it in the window title/headerbar as the 4th button.
> 
> > > (Mind you, this happens to be a good workaround against bug 787470 /
> > > https://bugs.launchpad.net/ubuntu/+source/gnome-terminal/+bug/1724250.)
> > 
> > Well, this is an icon theme bug, so we really shouldn't work around it like
> > this :-)
> 
> I'd rather say gnome-shell/mutter bug than icon theme bug, 

I remember a gtk+ dev saying it being an icon theme bug in one of the bugs about this (in gtk, I think) but couldn't find it again.

> > > There's been a couple of reports to Ubuntu that the terminal's menubar has
> > > two Terminal entries, and duplicates some of the features. 
> > 
> > Yes, that's also what happens if you set both the menubar and appmenu
> > properties of a GtkApplication, the shell doesn't show the app menu, and you
> > app window has show-menubar = TRUE.
> 
> Is this a desired user experience though? Do you have examples of apps doing
> this? The appmenu and the menubar right next to each other, as if they were
> one menu, doesn't make too much sense to me.

Don't know about desired 'user experience', but the code does this by design in gtk+/gtk/gtkapplicationwindow.c. Presumably that was as a result of some sort of discussion with the UI people?

> > > That's because the first one is the appmenu, the rest is the menubar.
> > > Towards the user, there's no distinction between them. Also I can't recall
> > > any other app doing this.
> > 
> > That's probably because few other apps uses GtkApplication::menubar, where
> > this will happen automatically, as above.
> 
> So do you mean it's okay and we should leave it this way, with the two
> Terminal menus?

No, I'm just describing the situation.
 
> > let's do it :-)
> > 
> > However the condition doesn't match what the comment describes?
> 
> My comment ("the shell shows *both* things") clearly matches my code
> ((shell_shows_this && shell_shows_that)) and not yours.
> 
> > + if (shell_shows_appmenu && shell_shows_menubar)
> >   [set appmenu to NULL]
> > 
> > Shouldn't that be more like
> > + if (!shell_shows_appmenu || shell_shows_menubar)
> >   [set appmenu to NULL]
> 
> Let's have a bit of fun with line drawing chars :)
> 
>                  ║ shell-menubar │ no-shell-menubar
> ═════════════════╬═══════════════╪══════════════════
> shell-appmenu    ║     unity     │       g-s-1
> ─────────────────╫───────────────┼──────────────────
> no-shell-appmenu ║       ?       │       g-s-2
> 
> My patch disables the appmenu for the "unity" slot only.
> 
> Your patch disables the appmenu for "unity", "?" and "g-s-2" which (for me)
> is the nondefault setup of gnome-shell, you say it's the default, where the
> appmenu is present as the 4th window decoration button.
> 
> In other words, your suggestion addresses comment 2, but leaves the behavior
> of comment 0 unchanged.

Now *I* am confused :-) Does newer gnome-shell actually set gtk-shell-shows-app-menu to FALSE?

Also I just found gtk_application_prefers_app_menu() which is supposed to make exactly this decision, so

+ if (!gtk_application_prefers_app_menu(...)) [ set appmenu to NULL ]
?

(In reply to Egmont Koblinger from comment #8)
> No, I didn't mess it up 2 comments above.
> 
> What I missed is: The giant icon artifact in the "g-s-2" field of the table
> is present in Ubuntu Artful with g-t _before_ the GMenu port, but not with
> current HEAD (as the code currently disables appmenu for the bottom row of
> the table).
> 
> In other words, the GMenu port accidentally removed the 4th button, and as
> such, provided a workaround for that huge icon bug.
> 
> My patch brings back the 4th button, and with that brings back the huge icon
> bug by no longer disabling appmenu for the bottom row. Your patch doesn't.

!gtk_application_prefers_app_menu() is equivalent to (!show_app_menu || show_menubar) which is my version of the patch...

So let's go with your patch instead, or just

+ if (shows_menubar) 
  [ set appmenu to NULL ]

dropping the shows_appmenu condition alltogether?

> I remember a gtk+ dev saying it being an icon theme bug in one of the bugs
> about this (in gtk, I think) but couldn't find it again.

Let me know please if you find it, it would be great to let humanity-theme devs aware of this, or at least know what others have figured out about it.

I didn't see anything obviously wrong in the theme, I mean icon are placed under the directory of the corresponding size, and index.theme also looks good wrt sizes.

> Now *I* am confused :-) Does newer gnome-shell actually set
> gtk-shell-shows-app-menu to FALSE?

Whatever's shipped by Artful (3.26.2):

Appmenu is by default shown at the top-left of the entire desktop. shell-shows-appmenu is TRUE. ("g-s-1" in the table.)

gnome-tweak-tool -> Top Bar, disable Application Menu. Log out & in.

A 4th window decoration button appears next to Close, Minimize and Maximize. This pops up the appmenu, which is hence no longer at the top-left of the entire desktop. shell-shows-appmenu is now FALSE. ("g-s-2" in the table.)

> So let's go with your patch instead, or just
> 
> + if (shows_menubar) 
>   [ set appmenu to NULL ]
> 
> dropping the shows_appmenu condition alltogether?

This only differs from my patch in the "?" case, so it's hard to tell :-) Maybe the shell shows the menubar but someone else shows the appmenu, so why not have it there. My thinking is still, according to the comment, that the only case when the appmenu doesn't make sense _to me_ (and is more confusing than useful) is when the appmenu and the menubar appear right next to each other, i.e. both are shown by the shell.

Let me know please if you have any doubts about this patch, we should probably play safe then and defer to 3-29, or only modify the "unity" case as per your suggestion, I wouldn't mind that much.

I'm also thinking about upgrading to Bionic tomorrow during the day to see if it changes anything in the game (e.g. perhaps fixes the huge icon bug).

Let's go with your patch then, we can always revisit later.

(In reply to Christian Persch from comment #13)
> Let's go with your patch then, we can always revisit later.

Okay, submitting now. (I ended up not upgrading to Bionic.)

> > > > (Mind you, this happens to be a good workaround against bug 787470 /
> > > > https://bugs.launchpad.net/ubuntu/+source/gnome-terminal/+bug/1724250.)
> > > 
> > > Well, this is an icon theme bug, so we really shouldn't work around it like
> > > this :-)
> > 
> > I'd rather say gnome-shell/mutter bug than icon theme bug, 
> 
> I remember a gtk+ dev saying it being an icon theme bug in one of the bugs
> about this (in gtk, I think) but couldn't find it again.

This is a mutter bug, I've tracked it down and have a fix (https://bugs.launchpad.net/ubuntu/+source/mutter/+bug/1718238).

Now if only I could file an upstream bug against mutter...

(The bug is triggered by another weird issue though where I suspect a GTK+ bug, but could maaaaybeeeee be an icon theme bug although I don't think so.)

(In reply to Egmont Koblinger from comment #15)
> Now if only I could file an upstream bug against mutter...
> 

You can do it here: https://gitlab.gnome.org/GNOME/mutter/issues

Thanks Piotr (I found that but had problems logging in, it's resolved now).

Filed https://gitlab.gnome.org/GNOME/mutter/issues/23.

Great to hear that. I see you have a patch, so you could open a merge request (equivalent to GitHub’s pull request).

Would that help? :)

I intentionally did not do that. I don't fancy that concept. I've never done a PR / MR, and don't feel like starting it now.

I wish developers over there to look at my change, I wish they improve it according to their taste, I wish they consider adding tests, geez I haven't even run the tests, they should run them, fix if fail, add new ones if they want to. I truly wouldn't want to write a changelog matching their existing practices. And I don't want the additional burden, cloning, uploading, maintaining something that is mine and not theirs until they merge it, and then come back to it to remove my fork (I guess that's how it goes). It's not for Mutter, it's my general feeling about contributing to other projects. I think I've done my fair share, now it's their turn to finish it from here. :)

(In reply to Egmont Koblinger from comment #19)
> Would that help? :)
> 

Yes, in general it’s much easier for the maintainer, but I’m sure a patch is also welcome.

(In reply to Piotr Drąg from comment #20)

> Yes, in general it’s much easier for the maintainer

That's obvious, my assumption is that for the whole ecosystem of developers cross-contributing to each other, it's significantly more effective if the final bits (code style, wording, comments, tests, changelog etc.) are done by those familiar with the code, rather than the occasional contributors. E.g. I'd much rather write VTE/gnome-terminal changelogs for others who send fixes to us, than to my contributions to other projects. YMMV.