Created attachment 138257 [details] [review]
Major rework of window monitoring to be application-based

As as told you the other day on IRC, could you preserve the per-activity (now "desktop context") stats stored in hash tables? I'll then improve it as soon as I can.

Other than that, I really like the design improvement!

Do we have a design for the desktop contexts?

I've written a short design here some time ago:
http://live.gnome.org/GnomeShell/DesktopContexts
And there's bug 579747.

My branch was somewhat working some time ago with a few hacks, but I've not updated it since your commits to app monitor (though I've local merges I'll finish). I guess the main issue is to find how we should integrate them so that they don't make the standard shell more complex - but we need a basic support for them in the core, among which the app monitor IMHO.

To do this though there are details.  My main question is, is an application on multiple contexts running once or twice from the perspective of the system?  The main problematic case here I see is windowed "container" apps, basically the browser and the terminal.  These are the two apps that I think people often want to "context-ize", but I don't see how you'd do it sanely without actually changing the apps, since we don't have control by default of the lifecycle of the app's windows.

I could change the monitor to store not just the last workspace an app was on, but all workspaces.  Would that be sufficient enough to implement a higher level framework on top?

Actually, maybe we should bite the bullet and switch to GMarkup so we don't keep breaking format compatibility.

If you know how the code should be changed concretely now let me know.  Before we took a context id as an integer, but the core just always used 0 when scanning the apps.

I think we need assistance from Jon to figure out the confusing maze of D-BUS interfaces (gnome-session is also involved somehow), but it's pretty clear from the source code that the SessionIdleChanged signal doesn't exist on org.gnome.ScreenSaver.

It should be noted that the way that idle was tracked was *intentionally* not the quite same concept of idle you'd want for, say, presence in an IM client or when to activate the screensaver - the comment:

     /* If no activity, see how long ago it was and count ourselves idle 
      * if it's been a short while. We keep this idle really short,
      * so it can be "more aggressive" about idle detection than
      * a screensaver would be.
      */

So if you flip to something and stop interacting with it, we only count 2 minutes of activity, rather than 10 or 15 minutes of activity. The whole app tracking thing is pretty approximate, so it might be fine to count 10 or 15 minutes, but it would be good to know what we are tracking. If the user has the screensaver disabled, or set to activate in hours, we don't want to count that full time to the current application. (Which is why I looked into gnome-screensaver and noticed that that signal didn't exist.) 

Some other comments in a brief look through the source code:

 * I don't think using 'restacked' to handle focus notification is sufficiently clean; yeah, it probably works OK, but we certainly have the ability to do better.

 * I'm not sure time accounting in correct. In the sequence:

 T1) Start using app A
 T2) Go idle
 T3) Stop being idle
 T4) Switch to app B

Don't you just account app A as T4-T3, not (T2-T2)+(T4-T3)

> To do this though there are details.  My main question is, is an application on
> multiple contexts running once or twice from the perspective of the system? 
> The main problematic case here I see is windowed "container" apps, basically
> the browser and the terminal.  These are the two apps that I think people often
> want to "context-ize", but I don't see how you'd do it sanely without actually
> changing the apps, since we don't have control by default of the lifecycle of
> the app's windows.
Yeah, using these two apps as examples is a good approach IMO. I think there will be a need at some point for the apps themselves to be aware of desktop contexts and work accordingly (e.g. by using different profiles in the terminal, or separate histories for web pages).  My idea is to develop the required framework for that, but we can't rely on apps doing so. Thus, we should make desktop contexts work by default, and allows applications to improve it by managing it themselves when they want to.

That's the broad theoretical plan. Now we need to discuss details. AFAIK single-instance apps are becoming the standard, and they can show several documents in tabs or separate windows (Firefox, GEdit, GNOME Terminal...). So we can consider that apps should be running only once, and create different windows for different desktop contexts.

With session management ATM, we're already able to save e.g. the pages a Firefox window is showing, and to restore them on the right workspace. The problem is, we can't ask Firefox not to show a given window when it's restarted, so that we can save a context and reopen it when the user wants. That's quite complex to handle - and I'm not sure it would work if we used separate instances of an app. Any ideas? Would that require refactoring the session management protocol?

> I could change the monitor to store not just the last workspace an app was on,
> but all workspaces.  Would that be sufficient enough to implement a higher
> level framework on top?
I don't really understand what you mean. When are you saving the last workspace an app was on? Why wouldn't you remember all the workspaces it had a window on? Even disregarding the desktop context issue, you have to consider all workspaces, aren't you?

> Actually, maybe we should bite the bullet and switch to GMarkup so we don't
> keep breaking format compatibility.
Not sure that's worth the pain. What do we want to store, now or in the future? The file applications_usage was meant to save stats, and only those. Maybe if we want to store more information, we'd better use another file.

> If you know how the code should be changed concretely now let me know.  Before
> we took a context id as an integer, but the core just always used 0 when
> scanning the apps.
Yeah, that was the way I found to prepare the code waiting for further developments. For now, regarding only the stats issue, I don't really need changing anything. If you just keep the design I have used initially (one usage list per desktop context, all lists stored in a hash table using the context id), I'll be happy. The concrete changes I need to do are not very complex, it's mainly updating the code to detect when a workspace is active. I can update them and commit; do you prefer I do that before you commit your changes, or after?


I hope I've been clear enough, I know this is a little confusing. There are two issues here: the broad one about apps definition and management, and the concrete one about storing per-context stats. Let's try not to mix them too much. ;-)

Created attachment 139648 [details] [review]
Bug 588343 - Major rework of window monitoring to be application-based

The previous application monitoring code was originally designed
to be based on WM_CLASS, which was then resolved on a server.
We have that resolution code locally now, so instead
of saving WM_CLASS data, save application IDs.

Also, inside the WM we have a much better
infrastructure for tracking windows.  In particular, rather
than polling, we can just watch for focus notification on
the display, and window add/remove.

Instead of polling XScreensaver, use DBus to watch org.gnome.Session
which already has an idle time watch.

Now there is no polling at all inside the monitor.

Re activities; this patch re-adds an activity context, switches to XML for storage (since it's way saner if we want to add stuff later).  

Re owen's comements: We now use org.gnome.Session for monitoring, should fix time accounting for idle, and depends on the notify::focus-window property on MetaDisplay from bug 590320.

Also fixes some bugs, and cleans up the code a bit more.

Generally, I like this patch - it certainly cleans things up from having hash tables of integers, etc.

I think it's pretty silly to have all this code for "activities" which we have no exact idea what they are, no UI design for, no idea how they relate to workspaces. But since Colin has gone through all this work to implement handling them in a nice way, just two comments:

 - They cannot be called activities. That means something different in our world (and something sort of different again for Zeitgeist.) "context" would be fine.

 - Please use string IDs rather than integer IDs - if we are going to be future-proofing against an unknown future, we should pick something that's more general than an integer index. The default context can be "".

- * Copyright Red Hat, Inc. 2006-2008
+ * Copyright Red Hat, Inc. 2006-2009

That was a date for the original code, not the rewritten code.

+ * This time tracking is implemented by watching for restack notifications,

focus notifications.

+#define FOCUS_TIME_PER_SCORE_SECONDS 7 /* Need 7 continuous seconds of focus */

I'm very unclear as to why why divide the score by 7. We don't care about the absolute value of the score, do we?

+#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 */

Lot's of missing space around operators and needs a parentheses around the whole thing, but I'd really suggest deleting the whole "clean" thing. I don't see value in it.

+#define SAVE_APPS_TIMEOUT_SECONDS 5    /* 30 seconds */

Since when is 5 seconds 30 seconds? :-) (dont' think the comment is necessary - generally such comments are there for milliseconds)

+#define SCORE_MIN SCORE_MAX >> 3

Needs parentheses. And the compiler is quite capable of turning / 8 into >> 3 if it's appropriate. But again suggest deleting the whole "clean" thing.

+  /* <char *appid, AppUsage *> */
+  GHashTable *app_usages_for_activity;

Points to a hash table, not to a AppUsage *.

+  /* not saved */
+  guint window_count;

The comment /* not saved */ is wrong now. And if it wasn't wrong would definitely ask for the question "OK, it's not saved, but what is it". It wasn't obvious to me until oI read the rest of the code that it was the count of open windows for that application.

+  if (result == NULL)
+    g_printerr ("failed to look up wm class: %s\n", wmclass);

Failing to look up window class is not an error - shouldn't commit with this.

+  if (usage == NULL)
+    {
+      /* This could in theory happen if we lost the app tracking.
+       * In that case, time for a punt.
+       */
+      return;
+    }

"we lost the app tracking" is completely unclear. (It looks like usage would be NULL for any application that you can't find .desktop file for?)

In other places you seem to use  get_app_usage_from_window() without checking the result.

+  usage_count = elapsed/FOCUS_TIME_PER_SCORE_SECONDS;

Missing spaces around operator

+      g_printerr ("incrementing %s by %d\n", meta_window_get_wm_class (window), usage_count);

Leftover debug spew.

+  gulong curtime = get_time ();
+  guint curtime = get_time ();

get_time() returns a long, not a ulong or uint.

* What happens if the focus changes while the user is idle (from a window closing, etc.) I don't think you handle that correctly.

+          MetaWindow *window = (MetaWindow*)window_iter->data;

Don't need the cast there - I think it reads better without, though maybe not everybody agrees.

+on_session_status_changed (DBusGProxy *proxy, guint status, ShellAppMonitor *monitor)

Parameters should be line-broken.

   g_signal_connect (screen, "notify::n-workspaces",
                     G_CALLBACK (shell_app_monitor_on_n_workspaces_changed), self);
+  display = meta_screen_get_display (screen);
+  g_signal_connect (display, "notify::focus-window", G_CALLBACK (on_focus_window_changed), self);

Two almost adjacent and identical calls to g_signal_connect() should be line-broken identically.

+  DBusGConnection *sessionbus;

session_bus

+  if (self->save_id > 0)

!= 0 would be clearer. > 0 implies it could be negative for some reason.

+ * Returns the set of applications which currently have more than one open
+ * window in the given activity.

Believe that it's "at least one" not "more than one"

+  /* Compress notifications */
+  self->idle_focus_change_id = g_timeout_add (250, idle_handle_focus_change, self);

Unclear where the 250 comes from or why it wouldn't be just as good at 0. (Note, you don't actually want a literal g_idle_add() without specifying a priority anywhere within the shell, since a constantly repainting app can starve a literal g_idle_add())

+static gboolean
+write_quoted (GDataOutputStream   *stream,

Would be better as "write_escaped()" - write_quoted() sounds like it will include the quotes, which it doesn't.

I dont 'think you want a GDataOutputStream - you just want an OutputStream - DataOutputStream is for writing binary data. I *do* think you want your output stream to be created as a GBufferedOutputStream wrapper around the file, or writing in little bits is going to be inefficient.

+      g_ascii_dtostr (score_buf, sizeof (score_buf), usage->score);
+      if (!write_attribute_string (data_output, "score", score_buf, &error))
+        goto out;

For cleaniness, I'd just go ahead and add a write_attribute_double() rather than inlining this.

> but I'd really suggest deleting the whole "clean" thing. I don't see value in it.

Hmm...it seems useful to me not to forever save an app that you happened to run once.  Maybe not a big deal...I can remove the code if you think it's a good change.

> In other places you seem to use  get_app_usage_from_window() without checking the result.

Those are all protected earlier by either calls to get_app_for_window or checking whether we already have the app in window_to_app.

> Unclear where the 250 comes from or why it wouldn't be just as good at 0.

Well, on multiprocessor it wouldn't help you that much since both apps would go as fast as possible, here we will use a constant amount of resources in this situation even as CPU speed increases.

> you just want an OutputStream - DataOutputStream is for writing binary data

AFAICS OutputStream is purely binary; void *buffer and size, whereas DataOutputStream has a useful _write_string which takes NULL-terminated and has clear conversion semantics for bindings since char * = utf8 by default.

Also I forgot...

>  - They cannot be called activities. 

Grumble =)

Pushed with all of these changes, we can continue the discussion though.

(In reply to comment #12)
> >  - They cannot be called activities. 
> 
> Grumble =)
Actually, the patch I want to update and commit was also fixing that, it has waited for a long time in my branch.

We can use string IDs for the contexts, but the idea behind using integers was that you can rename them if you want (just like folders), and it's easier to deal with in the code. But if you prefer strings, I can adapt to that switch easily.

We'll have to discuss the main problem around desktop contexts now, which is that while it should be optional (plugin), it needs some support in the core. If it's only in the app monitor, that's not really an issue IMHO, but we'll have to find out if there are other place where support is needed too. I guess that can be handled much better when the code is in JS, with dynamically loaded plugins.

Thanks for the patch!

I wasn't thinking names for the strings but rather UUIDs