Created attachment 139531 [details] [review]
Add shell_texture_cache_load_themed

Utility function to load a texture for a themed icon from a
string name.

Created attachment 139532 [details] [review]
Add distinction between cached apps versus not to ShellAppSystem

What was in the cache by default is things in applications.menu
and settings.menu.  We want the ability to also load arbitrary
.desktop files not included in those menus.  So rename
shell_app_system_lookup_app to shell_app_system_lookup_app_cached,
and add a new function which will explicitly look for (and cache
if found) the desktop file.

This patch depends on a new function in gnome-menus:
gmenu_tree_entry_new_from_id

Created attachment 139533 [details] [review]
Add shell_global_show_uri

Utility function to display a URI.

Created attachment 139534 [details] [review]
Add shell-util.[ch]; functions ported from gnome-panel for URIs

These two functions are useful for the Places implementation to
get name/icon for a desktop-related URI such as Documents or
Downloads.

Created attachment 139535 [details] [review]
Add Places

Implement the Places mockup.  This commit adds a link to Home,
Network, Connect to server, and the GTK+ bookmarks.

Created attachment 139958 [details] [review]
Add shell_global_show_uri

Utility function to display a URI.

Comment on attachment 139532 [details] [review]
Add distinction between cached apps versus not to ShellAppSystem

Going to do this one in a different and ultimately better way.

Created attachment 139959 [details] [review]
app system hacking

Created attachment 140023 [details] [review]
ShellAppSystem: Support loading a .desktop file directly

Previously, ShellAppSystem only loaded (and cached) the set of
.desktop files from applications.menu and settings.menu, using
the gnome-menus library.  The ShellAppInfo structure was
a "hidden typedef" for GMenuTreeEntry.

But we need to support loading an arbitrary .desktop file.  Thus,
refactor the ShellAppInfo into a real struct, with a refcount,
and allow it to point to either a GMenuTreeEntry or a GKeyFile.

Created attachment 140024 [details] [review]
Add Places

Implement the Places mockup.  This commit adds a link to Home,
Network, Connect to server, and the GTK+ bookmarks.

Note this series no longer depends on the gnome-menus patch (though I'd still like to see that at some point).

Created attachment 140028 [details] [review]
ShellAppSystem: Support loading a .desktop file directly

Previously, ShellAppSystem only loaded (and cached) the set of
.desktop files from applications.menu and settings.menu, using
the gnome-menus library.  The ShellAppInfo structure was
a "hidden typedef" for GMenuTreeEntry.

But we need to support loading an arbitrary .desktop file.  Thus,
refactor the ShellAppInfo into a real struct, with a refcount,
and allow it to point to either a GMenuTreeEntry or a GKeyFile.

(In reply to comment #1)
> Created an attachment (id=139531) [edit]
> Add shell_texture_cache_load_themed
> 
> Utility function to load a texture for a themed icon from a
> string name.

Looks fine. Maybe shell_texture_cache_load_icon_name() rather than shell_texture_cache_load_themed()? THe use if a "themed GIcon" seems like an internal detail - what you you are doing doing is looking up an icon name in the current icon theme. (The lookup is the same as gtk_image_new_from_icon_name(), etc.)

(In reply to comment #4)
> Created an attachment (id=139534) [edit]
> Add shell-util.[ch]; functions ported from gnome-panel for URIs
> 
> These two functions are useful for the Places implementation to
> get name/icon for a desktop-related URI such as Documents or
> Downloads.
 
+ This code adapted under the GPLv2 from:

Suggest saying "The code in this file"
Suggest saying "GPLv2+" if that's the case.
Suggest specific reference to file name gnome-panel/gnome-panel/panel-util.c took me a little while to track that down.

I verify that you successfully cut-and-pasted the code from gnome-panel.c :-) 

Would it be better to make shell_util_get_icon_for_uri() return GIcon? Then you'd either have:

 GICon => load GIcon
 or name => GIcon => load GIcon

And avoid:

 GIcon => name => GIcon => load GIcon

shell_util_get_icon_name_from_g_icon() is a bit horrible.

Maybe something less generic then shell-util.[ch] - I don't think we'd want to mix other random util functions into the same file - it seems useful to keep a single file with just the cut-and-paste code.

(In reply to comment #6)
> Created an attachment (id=139958) [edit]
> Add shell_global_show_uri
> 
> Utility function to display a URI.

Isn't this just:

 Gio.app_info_launch_default_for_uri(uri, Main.create_launch_context());

[ But without the handling of the desktop in Main.create_launch_context() ]

(In reply to comment #10)
> Created an attachment (id=140024) [edit]
> Add Places
> 
> Implement the Places mockup.  This commit adds a link to Home,
> Network, Connect to server, and the GTK+ bookmarks.

+const Big = imports.gi.Big;
+const Clutter = imports.gi.Clutter;
+const Pango = imports.gi.Pango;
+const GLib = imports.gi.GLib;
+const Gio = imports.gi.Gio;
+const Gtk = imports.gi.Gtk;
+const Tidy = imports.gi.Tidy;
+const Shell = imports.gi.Shell;
+const Lang = imports.lang;
+const Signals = imports.signals;
+const Mainloop = imports.mainloop;

You don't use Gtk, Tidy, Mainloop. (Would be nice to have some tools to find unused imports, functions, etc.)

+const DEFAULT_PADDING = 4;
+const PLACES_PADDING = 8;

See naming discussions elsewhere :-)

+            Main.overlay.hide();
+            try {
+                Shell.Global.get().show_uri(homeUri, Clutter.get_current_event_time());
+            } catch (e) {
+                // FIXME do something better with error
+                log(e);
+            }

All the other places in this function you hook up things that throw without the try/catch. Not sure there's much point in the try if it's just going to log the error anyways - that will happen without the backtrace anyways.

Trying it out, looks like a nice start! If you have something non-existent in your ~/.gtk-bookmarks, then it dies with:

    JS ERROR: !!!     stack = 'Error("Argument 'name' (type utf8) may not be null")@:0
("Argument 'name' (type utf8) may not be null")@gjs_throw:0
@:0
()@/home/otaylor/gnome-shell/source/gnome-shell/js/ui/places.js:119

Because Shell.util_get_icon_for_uri() is returning null, I patched it locally to do:

 if (icon == null)
   icon = "gtk-file"

But such items actually should be omitted entirely, and that's what gnome-panel does.

(In reply to comment #12)
> Created an attachment (id=140028) [edit]
> ShellAppSystem: Support loading a .desktop file directly
> 
> Previously, ShellAppSystem only loaded (and cached) the set of
> .desktop files from applications.menu and settings.menu, using
> the gnome-menus library.  The ShellAppInfo structure was
> a "hidden typedef" for GMenuTreeEntry.
> 
> But we need to support loading an arbitrary .desktop file.  Thus,
> refactor the ShellAppInfo into a real struct, with a refcount,
> and allow it to point to either a GMenuTreeEntry or a GKeyFile.

Generally looks looks good to commit, makes sense.

+  switch (info->type)
+  {

+  default:
+    g_assert_not_reached ();

I've gotten out of the habit of using default statements with assertions, I'd rather have the compiler tell me when I missed an enum value.

Slight indentation breakage

+  g_slice_free1 (sizeof (ShellAppInfo), info);

g_slice_free (ShellAppInfo, info);

+  if (!keyfile)
+    {
+      g_free (full_path);
+      return NULL;
+    }

Should you check for an erorr occuring instead and also free the key file? (Probably not good form to check *error != NULL, though Javascript will always pass in a non-zero *error)

+  switch (info->type)
+  {
+    case SHELL_APP_INFO_TYPE_ENTRY:
+      return g_strdup (gmenu_tree_entry_get_desktop_file_path ((GMenuTreeEntry*)info->entry));
+    case SHELL_APP_INFO_TYPE_DESKTOP_FILE:
+      return NULL;
+  }

Shouldn't this return info->keyfile_path in the second case?

-  iconname = gmenu_tree_entry_get_icon ((GMenuTreeEntry*)info);
+  switch (info->type)
+  {
+    case SHELL_APP_INFO_TYPE_ENTRY:
+      iconname = g_strdup (gmenu_tree_entry_get_icon ((GMenuTreeEntry*)info->entry));
+      break;

Looks like it leaks iconname

-      g_object_set (ret, "width", size, "height", size, NULL);
+      g_object_set (ret, "opacity", 0, "width", size, "height", size, NULL);

Leakage from something unrelated?

-shell_app_info_get_desktop_file_path
-  filename = gmenu_tree_entry_get_desktop_file_path ((GMenuTreeEntry*) info);
+  switch (info->type)
+  {
+    case SHELL_APP_INFO_TYPE_ENTRY:
+      filename = gmenu_tree_entry_get_desktop_file_path ((GMenuTreeEntry*) info->entry);
+      break;
+    case SHELL_APP_INFO_TYPE_DESKTOP_FILE:
+      filename = info->keyfile_path;
+      break;
+    default:
+      g_assert_not_reached ();
+      break;
+  }

Could call shell_app_info_get_desktop_file_path()? (with the above change)

(In reply to comment #17)

> Generally looks looks good to commit, makes sense.
> 
> +  switch (info->type)
> +  {
> 
> +  default:
> +    g_assert_not_reached ();
> 
> I've gotten out of the habit of using default statements with assertions, I'd
> rather have the compiler tell me when I missed an enum value.

I agree...looks like best practice is to omit the default:, but handle it after the end of the switch.

> +  if (!keyfile)
> +    {
> +      g_free (full_path);
> +      return NULL;
> +    }
> 
> Should you check for an erorr occuring instead and also free the key file?
> (Probably not good form to check *error != NULL, though Javascript will always
> pass in a non-zero *error)

Ah, good catch, thanks.

> -      g_object_set (ret, "width", size, "height", size, NULL);
> +      g_object_set (ret, "opacity", 0, "width", size, "height", size, NULL);
> 
> Leakage from something unrelated?

It's right, just uncommented in the git commit.  We want the "failed to load" textures to be 0 opacity.

(In reply to comment #14)

> Would it be better to make shell_util_get_icon_for_uri() return GIcon? Then
> you'd either have:
> 
>  GICon => load GIcon
>  or name => GIcon => load GIcon
> 
> And avoid:
> 
>  GIcon => name => GIcon => load GIcon
> 
> shell_util_get_icon_name_from_g_icon() is a bit horrible.

Hah, wow, yes it is.  I didn't really study the code originally admittedly.  Anyways, it's gone now and definitely a good cleanup.

> Maybe something less generic then shell-util.[ch] - I don't think we'd want to
> mix other random util functions into the same file - it seems useful to keep a
> single file with just the cut-and-paste code.

I named it shell-uri-util.[ch]. 

(In reply to comment #16)
> 
> +const DEFAULT_PADDING = 4;
> +const PLACES_PADDING = 8;
> 
> See naming discussions elsewhere :-)

Ok well for now I just reverted to "4".  I could make up names for these things but...we'll have to grep for spacing: and padding: eventually and fix this up for CSS.  Maybe two global constants for padding or spacing to use say between an icon and a name, etc?

> 
> But such items actually should be omitted entirely, and that's what gnome-panel
> does.

Yes, done.  I also made it handle non-existent ~/.gtk-bookmarks, and monitor it for changes.