Created attachment 145574 [details] [review]
Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage

Created attachment 145618 [details] [review]
Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage

There were 3 really independent classes inside ShellAppMonitor
previously.  The simplest thing was exporting a startup-notification
API, which is now split off into a small ShellStartupNotification
class.

The two bigger parts were mapping windows to applications, and
recording application usage statistics.  The latter part
(now called ShellAppUsage) is much more naturally built on top of
the former (now called ShellWindowTracker).

ShellWindow tracker also gains a focus-app property, which is
what most things in the shell UI are interested in (instead of
window focus).

ShellAppSystem moves to exporting ShellApp from more of its
public API, rather than ShellAppInfo.  ShellAppSystem also
ensures that ShellApp instances are unique by holding
a hash on the ids.

ShellApp's private API is split off into a shell-app-private.h,
so shell-app.h can be included in shell-app-system.h.

Favorites handling is removed from ShellAppSystem, now inside
appDisplay.js.

Port all of the JavaScript for these changes.

Review of attachment 145618 [details] [review]:

The reorg makes it hard to tell what's new and what's just moved, so I ended up reviewing all of it...

::: js/ui/appDisplay.js
@@ +32,3 @@
 const MAX_ITEMS = 30;
 
+function AppFavorites() {

possibly belongs in its own file, since several other files are using it too

@@ +73,3 @@
+    },
+
+    getFavorites: function() {

both callers of this would be simplified if it just returned the hash instead of arrayifying it

@@ +93,3 @@
+        ids.push(appId);
+        this._gconf.set_string_list(this.FAVORITE_APPS_KEY, ids);
+        this._favorites[appId] = app;

i think the last line is redundant, since _onFavsChanged() should get called because of the previous line

@@ +905,3 @@
 
+    _getFavorites: function() {
+

leftover?

::: js/ui/appIcon.js
@@ +412,3 @@
         if (windows.length > 0)
             this._appendSeparator();
+        this._toggleFavoriteMenuItem = this._appendMenuItem(null, this._source.isFavorite ? _("Remove from Favorites")

the change from "this._isFavorite" to "this._source.isFavorite" seems like it should break it, since you don't set AppIcon.isFavorite. Possibly a half-implemented change?

::: src/shell-app-system.c
@@ +585,1 @@
 shell_app_system_lookup_cached_app (ShellAppSystem *self, const char *id)

the name is wrong now, since it returns not-already-cached apps too

::: src/shell-app-usage.c
@@ +37,3 @@
+ * This time tracking is implemented by watching for focus notifications,
+ * and computing a time delta between them.  Also we self the
+ * GNOME Session "StatusChanged" signal which by default is emitted after 5

s/self/watch/ ? (global search and replace gone bad?)

@@ +48,3 @@
+#define USAGE_CLEAN_DAYS 7 /* If after 7 days we haven't seen an app, purge it */
+
+#define SNAPSHOTS_PER_CLEAN_SECONDS ((USAGE_CLEAN_DAYS * 24 * 60 * 60) / MIN_SNAPSHOT_APP_SECONDS) /* One week */

i can't figure out what this is supposed to be, but fortunately it's not used anywhere

@@ +96,3 @@
+  guint initially_seen_sequence;
+
+  GSList *previously_running;

This is read in when restoring from a save file, but doesn't seem to be used for anything

@@ +108,3 @@
+
+/* Represents an application record for a given context */
+struct AppUsage

ShellAppUsage vs AppUsage is a bit confusing

@@ +119,3 @@
+
+  /* Transient data */
+  guint initially_seen_sequence; /* Arbitrary ordered integer for when we first saw

does not seem to be used any more (either ShellAppUsage's or AppUsage's)

@@ +146,3 @@
+
+static long
+get_time (void)

this is just "time(NULL)"

@@ +153,3 @@
+}
+
+static void shell_app_usage_class_init(ShellAppUsageClass *klass)

needs newline and space

@@ +161,3 @@
+
+static void
+destroy_usage (AppUsage *usage)

unnecessary, could just use g_free

(or alternatively, make AppUsage use the slice allocator)

@@ +222,3 @@
+
+static void
+usage_iterator_init (ShellAppUsage *self,

there is a lot of extra work going on here to support "contexts", which we don't even support...

@@ +371,3 @@
+  gboolean idle;
+
+  idle = (status >= 3);

would be nice to #define that

@@ +383,3 @@
+        g_object_unref (self->watched_app);
+      /* Resync the active window, it may have changed while
+       * we were idle and ignoring focus changes.

this comment seems wrong since it's in the "becoming idle" codepath

@@ +389,3 @@
+
+      /* The GNOME Session signal we watch is 5 minutes, but that's a long
+       * time for this purpose.  Instead, just add a base 30 seconds.

could we just check the user-time of the focus window?

@@ +430,3 @@
+  self->enable_monitoring = FALSE;
+
+  self->app_usages_for_context = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,

you already created this hash table above

@@ +579,3 @@
+  g_date_set_time_t (date, time (NULL));
+  g_date_subtract_days (date, USAGE_CLEAN_DAYS);
+  date_days = g_date_get_julian (date);

er? I do not think that means what you think it does. (Or, well, whoever wrote the code originally.)

@@ +586,3 @@
+    {
+      if ((usage->score < SCORE_MIN) &&
+          (usage->last_seen < date_days))

Since this compares a time_t to a Julian day number, it will be FALSE for any date after late January 1970, so presumably we're currently never cleaning out the list.

GDate doesn't seem to have a time_t accessor. But we don't need to use GDate to subtract a week anyway. Just subtract 24*60*60*7.

@@ +618,3 @@
+  ret = g_data_output_stream_put_string (stream, elt, NULL, error);
+  g_free (elt);
+  if (!ret) goto out;

newline

@@ +621,3 @@
+
+  ret = write_escaped (stream, str, error);
+  if (!ret) goto out;

newline

@@ +685,3 @@
+      return FALSE;
+    }
+  buffered_output = g_buffered_output_stream_new (G_OUTPUT_STREAM(output));

space before last "(" (and likewise 2 lines down)

(also i'm not sure there's any reason to interpose a GBufferedOutputStream here...)

@@ +953,3 @@
+      gconf_value_free (value);
+    }
+  else /* Schema is not present, set default value by hand to avoid getting FALSE */

should remove this special-casing at some point, since the schema should always be present now

::: src/shell-startup-notification.c
@@ +62,3 @@
+  GdkDisplay *display;
+
+  /* FIXME: should we create as many monitors as there are GdkScreens? */

we hardcode a 1-screen assumption in a bunch of places

@@ +148,3 @@
+  themed = (GThemedIcon*)g_themed_icon_new (icon_name);
+  texture = shell_texture_cache_load_gicon (shell_texture_cache_get_default (),
+                                            G_ICON (themed), size);

sillycasting. Just make themed a GIcon.

::: src/shell-window-tracker.c
@@ +37,3 @@
+
+/* Title patterns to detect apps that don't set WM class as needed.
+ * Format: pseudo/wanted WM class, title regex pattern, NULL (for GRegex) */

app id, not WM class

@@ +110,3 @@
+}
+
+static void shell_window_tracker_class_init(ShellWindowTrackerClass *klass)

newline, space

@@ +149,3 @@
+  int i;
+
+  g_object_get (window, "title", &title, NULL);

can use meta_window_get_title() and save a strdup/free

@@ +169,3 @@
+            {
+              g_free (title);
+              /* Set a pseudo WM class, handled like true ones */

s/WM class/app id/

@@ +180,3 @@
+
+/**
+ * get_cleaned_wmclass_for_window:

Really, this is "get_app_id_from_wm_class" isn't it? The docs make it look like we're just converting it to some entirely arbitrary form.

@@ +196,3 @@
+    return NULL;
+
+  cleaned_wmclass = g_utf8_strdown (wmclass, -1);

the XGetClassHint man page says "If the data returned by the server is in the Latin Portable Character Encoding, then the returned strings are in the Host Portable Character Encoding" which I assume means it's the locale charset, which might not be UTF-8?

(mutter doesn't appear to do any conversions on it itself)

@@ +206,3 @@
+
+/**
+ * window_is_tracked:

further explanation of the tracked vs interesting distinction might be useful. (It's not clear to me why you'd want to track some un-interesting windows, but not others.)

@@ +387,3 @@
+
+static void
+on_transient_window_title_changed (MetaWindow      *window,

should we disconnect from notify::title here?

@@ +478,3 @@
+  _shell_app_remove_window (app, window);
+
+  if (shell_app_get_n_windows (app) == 0)

is there a potential problem here if app==tracker->focus_app (particularly given that focus_app gets changed from an idle handler and so might lag the destruction of the ShellApp?)

(In reply to comment #3)
> 
> is there a potential problem here if app==tracker->focus_app (particularly
> given that focus_app gets changed from an idle handler and so might lag the
> destruction of the ShellApp?)

We hold a reference to the focus app, so I don't see how it would be a problem.  Though this does bring up that we weren't unreffing it in the focus change hander...fixed.

Created attachment 145639 [details] [review]
Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage

The two parts were mapping windows to applications, and
recording application usage statistics.  The latter part
(now called ShellAppUsage) is much more naturally built on top of
the former (now called ShellWindowTracker).

ShellWindowTracker retains the startup-notification handling.

ShellWindowTracker also gains a focus-app property, which is
what most things in the shell UI are interested in (instead of
window focus).

ShellAppSystem moves to exporting ShellApp from more of its
public API, rather than ShellAppInfo.  ShellAppSystem also
ensures that ShellApp instances are unique by holding
a hash on the ids.

ShellApp's private API is split off into a shell-app-private.h,
so shell-app.h can be included in shell-app-system.h.

Favorites handling is removed from ShellAppSystem, now inside
appFavorites.js.

Port all of the JavaScript for these changes.

Created attachment 145642 [details] [review]
Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage

The two parts were mapping windows to applications, and
recording application usage statistics.  The latter part
(now called ShellAppUsage) is much more naturally built on top of
the former (now called ShellWindowTracker).

ShellWindowTracker retains the startup-notification handling.

ShellWindowTracker also gains a focus-app property, which is
what most things in the shell UI are interested in (instead of
window focus).

ShellAppSystem moves to exporting ShellApp from more of its
public API, rather than ShellAppInfo.  ShellAppSystem also
ensures that ShellApp instances are unique by holding
a hash on the ids.

ShellApp's private API is split off into a shell-app-private.h,
so shell-app.h can be included in shell-app-system.h.

Favorites handling is removed from ShellAppSystem, now inside
appFavorites.js.

Port all of the JavaScript for these changes.

I moved startup notification back into ShellWindowTracker, because I realized that I'm going to need it in the future to handle application tracking through SN.

Also I have a local fix for the now-spurious AppDisplay include in appIcon.js.

i'm just going to check out the bits I'd mentioned before and assume everything else is unchanged.

> ::: js/ui/appIcon.js
> @@ +412,3 @@
>          if (windows.length > 0)
>              this._appendSeparator();
> +        this._toggleFavoriteMenuItem = this._appendMenuItem(null,
> this._source.isFavorite ? _("Remove from Favorites")
> 
> the change from "this._isFavorite" to "this._source.isFavorite" seems like it
> should break it, since you don't set AppIcon.isFavorite. Possibly a
> half-implemented change?

The updated patch computes "isFavorite" separately in _init and _onItemActivate, which means that if something else modifies the favorites while the menu is popped up, then the menu item could end up doing the opposite of what its label says. (Yeah, unlikely.)

> @@ +96,3 @@
> +  guint initially_seen_sequence;
> +
> +  GSList *previously_running;
> 
> This is read in when restoring from a save file, but doesn't seem to be used
> for anything
> 
> @@ +119,3 @@
> +
> +  /* Transient data */
> +  guint initially_seen_sequence; /* Arbitrary ordered integer for when we
> first saw
> 
> does not seem to be used any more (either ShellAppUsage's or AppUsage's)
> 
> @@ +146,3 @@
> +
> +static long
> +get_time (void)
> 
> this is just "time(NULL)"
> 
> @@ +153,3 @@
> +}
> +
> +static void shell_app_usage_class_init(ShellAppUsageClass *klass)
> 
> needs newline and space

none of these were addressed. I realize none of them are related to what you're doing in this patch, but then, you did address some of the other comments on this file which also weren't related, so...

(In reply to comment #3)
> 
> i think the last line is redundant, since _onFavsChanged() should get called
> because of the previous line

Probably, but it seems better to leave in, because this way if any later code in the same callback calls getFavorites it will get the right data (unlikely in this case, but I think it's right in general)

> 
> This is read in when restoring from a save file, but doesn't seem to be used
> for anything

I was using it in https://bugzilla.gnome.org/show_bug.cgi?id=594802 and plan to re-resurrect it when I get a cahnce to work on that again.

> @@ +119,3 @@
> +
> +  /* Transient data */
> +  guint initially_seen_sequence; /* Arbitrary ordered integer for when we
> first saw
> 
> does not seem to be used any more (either ShellAppUsage's or AppUsage's)

True...I guess it's been mostly obsoleted by window user time and stable sequence.  Just axed it from my local patch.
 
> @@ +146,3 @@
> +
> +static long
> +get_time (void)
> 
> this is just "time(NULL)"

Not that non-POSIX gnome-shell is a sane thing, but I typically try to use GLib API where available.

> there is a lot of extra work going on here to support "contexts", which we
> don't even support...

Yeah, but the old code sort-of supported it and I thought it woudl be best to not regress there.

> could we just check the user-time of the focus window?

Instead of self->watch_start_time?  Hmm...that would be cleaner if the session status was shorter than 30s I guess.  But what we really need here is some reliable idle indicator on a known timeout, and I didn't want to dive into the X API to do it.  Something for later.
 
> (also i'm not sure there's any reason to interpose a GBufferedOutputStream
> here...)

General best practice?  Though it would have been nice had gio provided buffered by default, with an unbuffered option.
 
> @@ +953,3 @@
> +      gconf_value_free (value);
> +    }
> +  else /* Schema is not present, set default value by hand to avoid getting
> FALSE */
> 
> should remove this special-casing at some point, since the schema should always
> be present now

When we switch to dconf =)

> the XGetClassHint man page says "If the data returned by the server is in the
> Latin Portable Character Encoding, then the returned strings are in the Host
> Portable Character Encoding" which I assume means it's the locale charset,
> which might not be UTF-8?
> 
> (mutter doesn't appear to do any conversions on it itself)

Decided to just assume ascii.

(In reply to comment #8)
> i'm just going to check out the bits I'd mentioned before and assume everything
> else is unchanged.
> 
> > ::: js/ui/appIcon.js
> > @@ +412,3 @@
> >          if (windows.length > 0)
> >              this._appendSeparator();
> > +        this._toggleFavoriteMenuItem = this._appendMenuItem(null,
> > this._source.isFavorite ? _("Remove from Favorites")
> > 
> > the change from "this._isFavorite" to "this._source.isFavorite" seems like it
> > should break it, since you don't set AppIcon.isFavorite. Possibly a
> > half-implemented change?
> 
> The updated patch computes "isFavorite" separately in _init and
> _onItemActivate, which means that if something else modifies the favorites
> while the menu is popped up, then the menu item could end up doing the opposite
> of what its label says. (Yeah, unlikely.)

I thought about that but decided it'd be best fixed as part of some sort of GtkAction-like system.

Created attachment 145800 [details] [review]
Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage

The two parts were mapping windows to applications, and
recording application usage statistics.  The latter part
(now called ShellAppUsage) is much more naturally built on top of
the former (now called ShellWindowTracker).

ShellWindowTracker retains the startup-notification handling.

ShellWindowTracker also gains a focus-app property, which is
what most things in the shell UI are interested in (instead of
window focus).

ShellAppSystem moves to exporting ShellApp from more of its
public API, rather than ShellAppInfo.  ShellAppSystem also
ensures that ShellApp instances are unique by holding
a hash on the ids.

ShellApp's private API is split off into a shell-app-private.h,
so shell-app.h can be included in shell-app-system.h.

Favorites handling is removed from ShellAppSystem, now inside
appFavorites.js.

Port all of the JavaScript for these changes.

Attachment 145800 [details] pushed as e941e80 - Split ShellAppMonitor into ShellWindowTracker, ShellAppUsage