Created attachment 266142 [details] [review]
Add gtk_application_prefers_traditional_menus()

Applications can call this to determine if they should preferentially
show a 'traditional' menu layout.  This will be %TRUE on desktop
environments that do not have an application menu like the one in
gnome-shell.  It is %TRUE on Windows and Mac OS.

Applications are completely free to totally ignore this API -- it is
only provided as a hint to help applications that may be interested in
supporting non-GNOME platforms with a more native 'look and feel'.

Created attachment 266143 [details] [review]
Add gtk_application_load_resources()

If the application calls this (ideally from its 'startup' function) then
GtkApplication will look for resources under /org/app/id/application/
containing things such as the action descriptions and menubar layout of
the application.

Also add gtk_application_get_menu_by_id() to allow looking up any GMenu
defined in the 'menus.ui' resource by its id.  This is useful for plugin
systems that want to add extension points to the menus of applications.

In the event that the application has provided a 'traditional-menus.ui'
resource and gtk_application_prefers_traditional_menus() is %TRUE then
the 'traditional-menus.ui' will be used instead of 'menus.ui'.

This patch is incomplete, but it also shows the intent to load the
action description xml as well.

Created attachment 266144 [details] [review]
Use gtk_application_load_resources()

Use GtkApplication's facilities to automatically manage loading our menu
layouts.  Remove our own 'traditional menus' detection code.


Here's a first attempt at an example patch showing a clear improvement
when applied to the current state of gedit (which is an application that
wants to present alternate menu layouts when not running under
gnome-shell).

Review of attachment 266143 [details] [review]:

::: gtk/gtkapplication.c
@@ +1599,3 @@
+     * an alternative menus .ui file.
+     */
+    if (gtk_application_prefers_traditional_menus (application))

In the comment to the previous patch you said 'applications can totally ignore this api'. So that was a lie - you use it internally, so no matter what the app does, it is going to be used.

@@ +1645,3 @@
+    return NULL;
+
+  object = gtk_builder_get_object (application->priv->menus_builder, id);

It seems weirdly inconsistent to me to hardwire resource paths, but then expect the user to provide an id for poking into the resource.

I'm interested in using resources for action descriptions. I'm not interested in enshrining the notion that apps should have traditional menus as well as modern menus.

Review of attachment 266143 [details] [review]:

Neat!

::: gtk/gtkapplication.c
@@ +1568,3 @@
+  gchar *basepath;
+  gint i;
+

g_return_if_fail() is missing.

@@ +1587,3 @@
+    if (bytes)
+      {
+        //gtk_application_load_action_descriptions (application, bytes);

Why is this block in this patch when it's not used yet?

@@ +1641,3 @@
+{
+  GObject *object;
+

g_return_if_fail()s are missing.

Hardcoding the resource paths from the app ID means that you can't use this in apps that have a variable app ID (ie gnome-terminal-server).

(In reply to comment #4)
> In the comment to the previous patch you said 'applications can totally ignore
> this api'. So that was a lie - you use it internally, so no matter what the app
> does, it is going to be used.

This is still true.  That check will do nothing at all unless the application has also gone out of its way to add a 'tradition-menus.ui' resource.  I'd consider that a pretty clear opt-in on the application author's part.

> @@ +1645,3 @@
> +    return NULL;
> +
> +  object = gtk_builder_get_object (application->priv->menus_builder, id);
> 
> It seems weirdly inconsistent to me to hardwire resource paths, but then expect
> the user to provide an id for poking into the resource.

The intention here is to use this for things like plugin systems that sprinkle various named 'extension points' around the menus of the application which are locations that plugins can add menu items (like under the tools menu, etc).  This is what gedit is doing, for example.  Right now they manage that by adding an 'id' *attribute* to their GMenuModel and iterating to find that.  This let's them use a hash lookup on the builder instead.

(In reply to comment #7)
> Hardcoding the resource paths from the app ID means that you can't use this in
> apps that have a variable app ID (ie gnome-terminal-server).

That's a very good point.  I didn't think about this.  Hrmph...

(In reply to comment #5)
> I'm interested in using resources for action descriptions. I'm not interested
> in enshrining the notion that apps should have traditional menus as well as
> modern menus.

Well, apps _do_ care. I'm not using gtk+ to write a gnome-shell only application. We already have special code in place right now to deal with app v.s. win menus where we need to manually merge an app menu into a win menu when not running under gnome shell.

For gedit, I would love to see a straightforward way to have:

1) app menu in gnome shell, win menu in gear button
2) app menu + win menu in gear button (not running in shell)
3) global menu when running on OS X

I'm not a big fan of the automatic magic to fetch resources that may or may not be there, and that I have to know to name them exactly "traditional-menus.ui". This kind of magic glue may seem nice when you write software, but it's really hard to read and understand what happens underneath. Why can't we be explicit about it?

(In reply to comment #7)
> Hardcoding the resource paths from the app ID means that you can't use this in
> apps that have a variable app ID (ie gnome-terminal-server).

My top-two ideas for dealing with this:

 - _load_resources_from_path() or an extra arg for load_resources() that is usually NULL

 - some evil hack the stores the 'original app id' that you set when you constructed the application and tries to always use that

I actually sort of like the evil hack because it seems like it would always be "the right thing" and uh... we should DTRT.

(In reply to comment #9)
> (In reply to comment #5)
> > I'm interested in using resources for action descriptions. I'm not interested
> > in enshrining the notion that apps should have traditional menus as well as
> > modern menus.
> 
> Well, apps _do_ care. I'm not using gtk+ to write a gnome-shell only
> application. We already have special code in place right now to deal with app
> v.s. win menus where we need to manually merge an app menu into a win menu when
> not running under gnome shell.

Its alright for apps to care, and to expect some support from the toolkit for doing this. We do support this already. I'm just not happy with hard-coding magic names for menus.xml and traditional-menus.xml, when these things are not that canonical. It seems we agree on that.

Maybe a bit of background about where I'm coming from:

After some research into the topic I've come to the conclusion that by and large we want to support three different type of menu layout:

 - GNOME 3 style

 - Mac OS style

 - "traditional" style

GNOME 3 style tends to have these attributes:

 - app menu is set by the application

 - typically no menubar, favouring a gear menu instead

Mac OS style tends to have these attributes:

 - app menu is _not_ set by the application (see bug 720552 for why)

 - a normal menubar is provided

 - the menubar is the traditional File/Edit/View menu with the exception that
   there are no items for "About", "Preferences" and "Quit" because they are
   already in the application menu

Traditional style tends to have these attributes:

 - app menu is _not_ set by application

 - a normal menubar is provided

 - the menubar is the traditional File/Edit/View menu


This means that we really have something like 2.1 different cases: the only difference between "mac os" and "traditional" is that "About" "Preferences" and "Quit" should not appear among the items in the normal menubar on Mac OS.  That's what the soon-to-appear hidden-when='macos' is about.

So I want to get into a place where we recommend to apps to do this in order to have menus handled properly on all platforms:

startup() {
  ...

  if (app.prefers_traditional_menubar()) {
    menubar = create_new_file_edit_view_style_menu();
    set_menubar (menubar);
  } else {
    appmenu = create_menu_for_shell();
    set_app_menu (appmenu);
  }

and also use this to (manually) control if they show a gear menu or not.

Of course, apps are free to ignore 'prefers_traditional_menubar()' and either always present a GNOME-style UI or always present a traditional style UI.

I hope what I've said so far is mostly uncontroversial.


Now the rest: for some time I've wanted GApplication to load various resources from well-known path to simplify the number of expected tasks that applications must do in startup() and also to give a future GNOME IDE some "well known names" that it can use when creating resources with graphical editors.

From this standpoint I think it makes a good deal of sense to have hard-coded well-known names for things like the application's list of action descriptions or the app menu and menubar.

So that's "menus.ui".

The deal with "traditional-menus.ui" is simply so that applications can have the if(prefers_traditional) check above done for them automatically, at least as far as gtk_application_load_resources() is concerned.

I agree that it's a bit gross but I think that having hard-coded names here is valuable for the reasons I stated above, and I think that we're going to have to cross this bridge eventually.  I want to start considering other things as well -- a timely example that I'm rolling over in my head right now is the ability to list your GOptionContext configuration via a resource (although I'll probably end up going against that idea).  We've also talked about a general metadata file -- see bug 688989.

In some ways I guess this bug has a lot of overlap with bug 688989 but I've come to think that one-big-file is a bad approach.  In particular, having the action descriptions in the same file format as the menus is something that I've experimented with and am strongly against.

So that's my thoughts more or less... I'd love to hear some suggestions for how to do this in a way that people are more comfortable with, but I think that what I've recommended here is actually not that bad.  It's also completely optional: if you don't call this function then nothing happens.

Created attachment 279618 [details] [review]
Add gtk_application_prefers_app_menu()

Rebased, updated (version tags) and renamed the API to avoid the awkward
"traditional" wording.  Also: added docs.

Review of attachment 279618 [details] [review]:

It would be good to update one of our example applications to show the 'ideal' portable behavior, using this new API.

::: gtk/gtkapplication-dbus.c
@@ +439,3 @@
+       */
+      result = show_app_menu && !show_menubar;
+      decided = TRUE;

I don't understand hwo show_menubar factors into this here. And if you want to do it this way, shouldn't there be a prefers_global_menubar equivalent ?

::: gtk/gtkapplicationimpl.c
@@ +159,3 @@
+gtk_application_impl_prefers_app_menu (GtkApplicationImpl *impl)
+{
+  return GTK_APPLICATION_IMPL_GET_CLASS (impl)->prefers_app_menu (impl);

I guess you want to do an if here ? You don't implement prefers_app_menu in all subclasses...

Review of attachment 279618 [details] [review]:

I'll take a look at updating bloatpad.

::: gtk/gtkapplication-dbus.c
@@ +439,3 @@
+       */
+      result = show_app_menu && !show_menubar;
+      decided = TRUE;

more or less: show_menubar is TRUE on both Unity and Mac OS, where we prefer not to have a separate app menu.  See the "research" in comment 12.

Also: in both cases, the "global menubar" is the same as what you'd expect to find in a local (win32 style) menubar.

::: gtk/gtkapplicationimpl.c
@@ +159,3 @@
+gtk_application_impl_prefers_app_menu (GtkApplicationImpl *impl)
+{
+  return GTK_APPLICATION_IMPL_GET_CLASS (impl)->prefers_app_menu (impl);

I implement it (trivially) on the base class in order to avoid the conditional.

(In reply to comment #15)
> 
> I implement it (trivially) on the base class in order to avoid the conditional.

Oh, missed that. I guess this is fine, then.

Might be good to add a pointer to this new api from the GtkSettings::gtk-shell-shows-app-menu and GtkSettings::gtk-shell-shows-menubar docs.

Review of attachment 266143 [details] [review]:

Docs are missing here, obviously.

::: gtk/gtkapplication.c
@@ +1563,3 @@
 
+void
+gtk_application_load_resources (GtkApplication *application)

Maybe we can overcome the 'too late for 100% magic' concern by having an (optional) path here ?

gtk_application_load_resources (app, "/my/resource/path");

would load resources from that location, while

gtk_application_load_resources (app, NULL)

could use the default path.

Review of attachment 279618 [details] [review]:

.

Comment on attachment 279618 [details] [review]
Add gtk_application_prefers_app_menu()

Attachment 279618 [details] pushed as d3b34d3 - Add gtk_application_prefers_app_menu()

As discussed on IRC, I like the idea of removing some boiler plate code and have a declarative syntax to say what resources should be loaded, but I still do not like the magic-names approach.

From one side we try to make it easier to write a new gtk application but from the other we end up increasing the barrier of entry for the random contributor that wants to "fix a bug", by requiring to know a lot of insider information before understanding what's going on.
E.g. If I want to fix a menu item, I'll grep for the label, find that's in "menus.ui", then I'll grep for "menus.ui" to find where it is used and I'll not find anything.

I'd much rather have the IDE auto generate

g_object_new (MY_APP,
              "resource-path", "/my/app",
              "menu-resource", "menus.ui",
              "actions", "actions.ui",
              ...)

etc


or at least have a single conventional entry point, e.g. having a

<application>
  <menu ref="menus.ui">
  ....
</application>

map (this would be nicer if we had templates for, non-widgets)


or to avoid an extra "mapping-file" indirection, extend the gresource syntax

<gresources>
  <gresource prefix="/org/gnome/gedit/ui" application="org.gnome.gedit">
    <file role="application::menu" preprocess="xml-stripblanks">gedit-menu.ui</file>

Created attachment 279907 [details] [review]
GApplication: add an internal is_constructed flag

We will use this for some internal purposes... but for now we can use it
to make sure the user didn't forget to chain-up on constructed.

Created attachment 279908 [details] [review]
GApplication: add a "resource base path"

We don't use this for anything inside of GApplication yet, but Gtk is
about to start using it to find various bits of the application (such as
its menus, icons, etc.).

By default, we form the base path from the application ID to end up with
the familiar /org/example/app style.

Created attachment 279909 [details] [review]
bloatpad: move into private subdir

Move bloatpad to ./examples/bp/ so that we can start treating it as more of a
"normal" app instead of just jamming everything into a single .c file.

We don't use the name "bloatpad" for the directory in order not to
create 'git pull' pain with the probably-already-existing executable of
the same name.

Created attachment 279910 [details] [review]
bloatpad: use resources

Created attachment 279911 [details] [review]
GtkApplication: use resources for loading menus

Use the new ::resource-base-path property on #GApplication to attempt to
load the menu layout of the application.

We look first at gtk/menus-appmenu.ui or gtk/menus-traditional.ui
depending on the setting of gtk_application_prefers_app_menu().  Failing
that, we fall back to the common case of gtk/menus.ui (which should
always be given).  This provides a convenient way for application
authors to provide a different set of menus, depending on the desktop
environment they find themselves in.

As is the intention with other resources, if the resource base path is
unset, nothing will be loaded.  Additionally, if the expected files are not
found, it is not an error -- just nothing happens.

Created attachment 279912 [details] [review]
bloatpad: use Gtk's automated menu loading

We move our menus.ui file into Gtk's namespace so that it will get
picked up.  Accordingly, we no longer have to do any of the work for
ourselves...

Created attachment 279937 [details] [review]
GApplication: add a "resource base path"

Lars convinced me that the previous iteration of this patch was
sufficiently ugly that it needed to be redone.

This introduces the property as a non-construct property that has its
value set from the application ID during construction and can be freely
changed (with no further magic) thereafter.

Created attachment 279938 [details] [review]
gapplication tests: test resource base path

Run some cases to make sure resource base path is behaving as we expect
it to.

Review of attachment 279937 [details] [review]:

GApplication leaks resource_path.

::: gio/gapplication.c
@@ +1663,3 @@
+void
+g_application_set_resource_base_path (GApplication *application,
+                                 const gchar  *resource_path)

When can I call this function? In init() doesn't work because you assert() on that and in constructed() doesn't work because is_constructed is set before my constructed handler runs.

It looks like I can only set this by passing it to g_object_new(). That's fine with me, but we can make this function private then. Or am I missing something?

There's also some alignment issue here ;)

Review of attachment 279907 [details] [review]:

A bit ugly, but ok if we need it.

(In reply to comment #29)
> When can I call this function? In init() doesn't work because you assert() on
> that and in constructed() doesn't work because is_constructed is set before my
> constructed handler runs.

Take a look at the test cases for various ways that you can use this.

By and large I expect:

 - almost nobody will explicit set this

 - of those who do:

   - people who subclass will pass it to g_object_new()

   - people who call g(tk)_application_new() will use the setter

but it is also possible to use the setter from your ->constructed().

(In reply to comment #30)
> Review of attachment 279907 [details] [review]:
> 
> A bit ugly, but ok if we need it.

We actually don't need this anymore... It's still something that I would like to enforce, but less important now....

Review of attachment 279909 [details] [review]:

as said on irc: fine with me

Review of attachment 279910 [details] [review]:

ok

Review of attachment 279911 [details] [review]:

Whats missing here is documentation. The GtkApplication long description should have a paragraph about the new magic.

::: gtk/gtkapplication.c
@@ +511,3 @@
+
+  /* Load the menus */
+  {

Why the extra scope here ? seems a little odd

Review of attachment 279912 [details] [review]:

looks fine to me

(In reply to comment #34)
> Why the extra scope here ? seems a little odd

When we start doing things like parsing actions and such here, we will have more temporary local variables whose scope we want to limit to those sections -- you probably saw that in an earlier version of the patch.

This anticipates that we will be adding more to this function going forward -- we can avoid the re-indent this way.

Attachment 279909 [details] pushed as 4d8b2af - bloatpad: move into private subdir
Attachment 279910 [details] pushed as b40672f - bloatpad: use resources

Created attachment 280084 [details] [review]
GtkApplication: use resources for loading menus

Use the new ::resource-base-path property on #GApplication to attempt to
load the menu layout of the application.

We look first at gtk/menus-appmenu.ui or gtk/menus-traditional.ui
depending on the setting of gtk_application_prefers_app_menu().  Failing
that, we fall back to the common case of gtk/menus.ui (which should
always be given).  This provides a convenient way for application
authors to provide a different set of menus, depending on the desktop
environment they find themselves in.

As is the intention with other resources, if the resource base path is
unset, nothing will be loaded.  Additionally, if the expected files are not
found, it is not an error -- just nothing happens.

Review of attachment 280084 [details] [review]:

Looks like a great start for docs, thanks.

Created attachment 280085 [details] [review]
GtkAppliation: setup icon theme resource path

If we have a resource base path for the application, set up an icon
theme search path based on it (within the default icon theme).

Created attachment 280086 [details] [review]
GtkApplication: document icon path setup

Review of attachment 280085 [details] [review]:

looks good to me. should probably be mentioned in the 'magic' section of the docs, too

Review of attachment 280086 [details] [review]:

::: gtk/gtkapplication.c
@@ +88,3 @@
  * gtk_application_set_app_menu() and gtk_application_set_menubar().
  *
+ * Gtk will also automatically setup an icon search path for the default

In to docs, I say consistently 'GTK+'. It would probably by better (more precise, anyway) to say that GtkApplication sets it up in the startup() callback ?

Attachment 279937 [details] pushed as cea9de9 - GApplication: add a "resource base path"
Attachment 279938 [details] pushed as 5825055 - gapplication tests: test resource base path

Attachment 279912 [details] pushed as cc1af0f - bloatpad: use Gtk's automated menu loading
Attachment 280084 [details] pushed as 868ee07 - GtkApplication: use resources for loading menus
Attachment 280085 [details] pushed as 687a846 - GtkAppliation: setup icon theme resource path
Attachment 280086 [details] pushed as 4948516 - GtkApplication: document icon path setup

Comment on attachment 279907 [details] [review]
GApplication: add an internal is_constructed flag

We don't need this, so let's drop it for now.