GNOME Bugzilla – Bug 570899
App monitoring module
Last modified: 2009-05-02 10:20:59 UTC
code using desktop-data-model parts to track application usage thoughout sessions and return most popular apps
Created attachment 128190 [details] [review] initial commit of app monitoring module
Since it's not really a patch but a relatively large file, you may want to look at it from Gitorious. For the above patch: http://www.gitorious.org/projects/gnome-shell/repos/mainline/commits/5ef16f7c6881b56521b2d9ab1db99106538a511c And for the files: http://www.gitorious.org/projects/gnome-shell/repos/mainline/blobs/app-monitor/src/shell-app-monitor.c (and the same for shell-app-monitor.h)
Instead of > * @returns a GSList of gchar*. g_free() yourself when no longer needed. You should now do: * Return: (element-type utf8) (transfer full): List of application wm_classes See http://live.gnome.org/GObjectIntrospection/Annotations > 215 int event_base, error_base; Declaration after statement. I think we're still trying to stay pre-C99 style. We should try to remove the 5s idle polling if we can. Since we're now in the WM process, I wonder if we're getting sufficient events by default that we can implement this without polling? Actually, now that I think about this a bit more there's a gnome-screensaver idle signal (SessionPowerManagementIdleChanged) I don't know why this code wasn't using it. > g_warning ("Could not load applications usage data: %s.\n", error->message); We probably shouldn't warn if you don't have data yet (i.e. on first startup). Could just drop the warning entirely; the only realistic error is ENOENT anyways. > 701 /* Only free the lists, we reuse directly their elements > 702 * FIXME: no longer true */ This is just g_slist_foreach (g_free), no? That's my first pass...I just realized I'm reading the diff instead of the file so switching to that.
Instead of > > > * @returns a GSList of gchar*. g_free() yourself when no longer needed. > > You should now do: > > * Return: (element-type utf8) (transfer full): List of application wm_classes > > See http://live.gnome.org/GObjectIntrospection/Annotations > I'll read that, here I was still naively copying syntax used internally by GTK, maybe mixing it with Doxygen's by the way... ;-) > 215 int event_base, error_base; > > Declaration after statement. I think we're still trying to stay pre-C99 style. > You mean that variables should be declared at the beginning of the block? Yes, that's a remnant of a copy/paste that helped to divide old app-monitor code form Hippo's at the beginning. I'll fix that. > We should try to remove the 5s idle polling if we can. Since we're now in the > WM process, I wonder if we're getting sufficient events by default that we can > implement this without polling? Actually, now that I think about this a bit > more there's a gnome-screensaver idle signal > (SessionPowerManagementIdleChanged) I don't know why this code wasn't using it. > That's your realm! Should i get this message from g-ss using D-Bus? > g_warning ("Could not load applications usage data: %s.\n", error->message); > > We probably shouldn't warn if you don't have data yet (i.e. on first startup). > Could just drop the warning entirely; the only realistic error is ENOENT > anyways. > That was mainly intended for malformed files, but maybe that's a useless case (who would modify manually that data?). So we would just skip it anyway, and only warn on saving if it's not possible? > 701 /* Only free the lists, we reuse directly their elements > > 702 * FIXME: no longer true */ > > This is just g_slist_foreach (g_free), no? > That comment that here at a time I was thinking of not strdup()ing the contents of the list, which was quite stupid given the memory problems it created for a few chars gained. I missed it when committing but obviously g_slist_foreach (g_free) will do. I'll commit that soon, but feel free to make more comments.
(Sorry for the big quotation mess above, no idea what happened to my message...)
The code is now basically working and is currently used by appDisplay to show most popular applications. Give it a try: http://www.gitorious.org/projects/gnome-shell/repos/mainline
I think I've come to a state in which the code can be merged when you want to. I'll only fix minor bugs if I find some, so you can review it.
I get no apps at all with your branch...
Hey, looking through the code, it looks really good. But of course, I managed to find a few things :-) Mot of this is details, style issues, and style suggestions rather than major problems. ====== PKG_CHECK_MODULES(LIBGNOMEUI, libgnomeui-2.0) +PKG_CHECK_MODULES(XSCRNSAVER, xscrnsaver) Should add both of these to MUTTER_PLUGIN instead - it's supposed to be one PKG_CHECK_MODULES per build target, not per library. + this._appMonitor.connect('popularity-changed', this._redisplay); This won't work as desired - the callback needs to be Lang.bind(this, this._redisplay) + // Ask for more apps than needed in case of problems + var apps = this._appMonitor.get_apps ("default", + this._maxItems + 5); Should describe what these 'problems' might be. Also, I think I'd like to see the 5 as a constant rather than inline. + let appId = apps[i].toLowerCase () + ".desktop"; I think the C code should do the lowercasing - the uppercase initial is really a pecularity of the WM_CLASS property. $(LIBGNOMEUI_LIBS) \ + -lXss \ libbig-1.0.la \ This would be better as $(XSCRNSAVER_LIBS), but if you move to MUTTER_PLUGIN as described above +/* This file includes modified code from the Hippo module of the + * desktop-data-model project in the functions collecting application usage data. + * Written by Owen Taylor, originally licensed under LGPL 2.1. + */ - There's no "Hippo module" - Hippo was just our namespace prefix we used everywhere. Say "From desktop-data-engine/engine-dbus/hippo-application-monitor.c" - Needs to include the information "Copyright Red Hat, Inc. 2006-2008" +/* Above this level, we divide all scores to reduce them. + * This allows the popularity order to change as new apps are used more often. + * With this value, an app goes from bottom to top of the + * popularity list in 50 hours of use */ +#define SCORE_MAX SAVE_APPS_TIMEOUT/3600*50 This looks fishy - A/B*C == (A/B)*C, so you have (1/50) hour not 50 hours. Also any time you have a complicated #define like this you should enclose it in parentheses. +/* Title patterns to detect apps that don't set WM class as needed. + * Format: pseudo/wanted WM class, title glob pattern */ +#define TITLE_PATTERNS { \ + {"ooo-writer", "* - OpenOffice.org Writer"}, \ + {"ooo-calc", "* - OpenOffice.org Calc"}, \ + {"ooo-impress", "* - OpenOffice.org Impress"}, \ + {"ooo-draw", "* - OpenOffice.org Draw"}, \ + {"ooo-base", "* - OpenOffice.org Base"}, \ + {"ooo-math", "* - OpenOffice.org Math"}, \ + {NULL, NULL} }; I wouldn't use a #define here, it's pretty confusing. (Note that you ended up with a double ;; - some compilers don't like that.) I'd probably define a static global variable instead. + gfloat score; /* Based on the number of times we'e seen the app and normalised */ In almost all cases, I'd use double rather than float. double is the native numeric type - float is only really useful if you need to pack a lot of floating point data into a small space. +static void +shell_app_monitor_finalize (GObject *object); + +static void +on_monitor_apps_changed (GFileMonitor *monitor, GFile *file, GFile *other_file, + GFileMonitorEvent event_type, gpointer user_data); Here and throughout - Prototypes should have the type on the same line as the function name - Function arguments should be lined up one per line, both in prototypes and in definitions. static void on_monitor_apps_changed (GFileMonitor *monitor, GFile *file, GFile *other_file, GFileMonitorEvent event_type, gpointer user_data); Also, I'd recommend ordering the file as: - Struture typedefs - Structure definitions - Forward definitions of functions (function prototypes) - Global data - Code - signals[CHANGED] = - g_signal_new ("changed", + signals[APPS_CHANGED] = + g_signal_new ("apps-changed", SHELL_TYPE_APP_MONITOR, G_SIGNAL_RUN_LAST, - G_STRUCT_OFFSET (ShellAppMonitorClass, changed), + G_STRUCT_OFFSET (ShellAppMonitorClass, apps_changed), NULL, NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + + signals[POPULARITY_CHANGED] = + g_signal_new ("popularity-changed", + SHELL_TYPE_APP_MONITOR, + G_SIGNAL_RUN_LAST, + 0, + NULL, NULL, + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0); +} These should be consistent about whether they use a class member for the default handler or not - the old one did, your new signal doesn't. I think not having it is slightly better (we aren't going to subclass from C, and thats's the only point of having the default handler.) But mostly it needs to be consistent. +/* Little callback to destroy lists inside hash table */ +static void destroy_popularity (gpointer key, gpointer value, gpointer user_data) +{ + GSList *list = (GSList *) value; In C, you don't need a cast when going from gpointer (void *) to any other type. I generally like using l as a list iteration variable. + AppPopularity *app_pop; Generally, avoid random abbreviations, just call the variable app_popularity + while (list) + { + app_pop = (AppPopularity *) list->data; A little clearer better to declare the variable inside the scope + g_free (app_pop->wm_class); + g_free (app_pop); + list = list->next; + } + g_slist_free (list); You've already walked list to NULL :-) So, in summary: GSList *l; for (l = data; l; l = l->next) { AppPopularity *app_popularity = l->data; g_free (app_popularity->wm_class); g_free (app_popularity); } g_slist_free (data); - g_signal_connect (monitor, "changed", G_CALLBACK (on_monitor_changed), self); + g_signal_connect (monitor, "apps-changed", + G_CALLBACK (on_monitor_apps_changed), self); I don't think you meant to change this connection. (Doesn't it give a critical warning?) + g_warning + ("Screensaver extension not found on X display, can't detect user idleness"); We don't have a fixed 80 column limit. Prefer long lines to bad breaks. + self->popularities = g_hash_table_new_full (g_str_hash, g_str_equal, + (GDestroyNotify) g_free, + /* Free manually in finalize () */ + (GDestroyNotify) NULL); You should comment *why* you don't use the GDestroyNotify not just that you don't. (That, because you are replacing elements and you don't want the destroy notify to free the old list) + g_object_get (shell_global_get(), "configdir", &shell_config_dir, NULL), + path = g_build_filename (shell_config_dir, DATA_FILENAME, NULL); You leak shell_config_dir. + if (g_hash_table_size(self->popularities)) + self->upload_apps_burst_count = 0; I never use if (x) when I mean (x != 0) numerically. if (g_hash_table_size(self->popularities) > 0) I think is clearer. + g_list_foreach (self->desktop_dir_monitors, (GFunc) g_object_unref, NULL); + g_list_free (self->desktop_dir_monitors); + save_to_file (self); Would recommend calling save_to_file before you start freeing things. (Also, would recommend saving in a timeout - getting finalize called at shutdown will be at best unreliable.) + * @number: how many applications are requested. Note that the actual + * list size may be less, or NULL if not enough applications are registered. Would recommend "max-count" perhaps rather than 'number' + list = g_slist_append (list, strdup (app_pop->wm_class)); + popularity = popularity->next; + } + return list; Generally best practice is to g_slist_prepend and then return g_slist_reverse(list) - traversing to the end each time is O(n^2). (Doesn't really matter here, but a good habit to follow uniformly.) + /* Find the currently focused window by looking at the _NET_ACTIVE_WINDOW property + * on all the screens of the display. + */ + for (i = 0; i < n_screens; i++) + { + GdkScreen *screen = gdk_display_get_screen (monitor->display, i); + GdkWindow *root = gdk_screen_get_root_window (screen); + + if (!gdk_x11_screen_supports_net_wm_hint (screen, net_active_window_gdk)) + continue; + + XGetWindowProperty (xdisplay, GDK_DRAWABLE_XID (root), + net_active_window_x, + 0, 1, False, XA_WINDOW, + &type, &format, &n_items, &bytes_after, &data); + if (type == XA_WINDOW) + { + active_window = *(Window *) data; + XFree (data); + break; + } + } Hmm, a MutterPlugin is per GdkScreen, not per GdkDisplay. On the other hand, I can't see running a separate GnomeShell instance on multiple screens of the same display. Let's leave it like this for now. Nobody uses multiple screens anyways :-) + /* This is a check for Nautilus, which sets the instance to this + * value for the desktop window; we do this rather than check for + * the more general _NET_WM_WINDOW_TYPE_DESKTOP to avoid having + * to do another XGetProperty on every iteration. We generally + * don't want to count the desktop being focused as app + * usage because it frequently can be a false-positive of an + * empty workspace. Was meant to be "on an empty workspace" (existing typo in the desktop-data-engine comment) +void +update_app_info (ShellAppMonitor *monitor) +{ + char *wm_class; + char *title; + char *rev_title; + GHashTable *app_active_times = NULL; /* GTime spent per activity */ + GTimeVal now; + GSList *temp_list; + static GSList *title_patterns = NULL; + static gchar *patterns[][2] = TITLE_PATTERNS; I don't like multi-dimensional arrays. I don't like keeping a parallel GSList and array that you have to keep in sync. I would suggest: struct { const char *pattern; const char *app_id; GPattern *pattern; } = { { "Foo", "Bar", NULL }, [...] } The NULLs aren't actually necessary, but they make things clearer. + g_pattern_spec_new (patterns[i][1])); Would suggest using GRegex rather than GPattern - GRegex is more well tested and general - GPattern is somewhat of an internal detail of the GTK+ theme system that was split out. + g_get_current_time (&now); + g_hash_table_insert (app_active_times, + g_strdup (monitor->current_activity), + GINT_TO_POINTER (now.tv_sec)); Can't use you the get_time() function? I would suggest using g_hash_table_replace() here - g_hash_table_insert() has weird behavior when overwriting an existing key/value pair (it uses the old key and the new value, and frees the new key and the old value. WHich ends up working here, but is unexpected and likely to lead to future bugs.) + /* 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, since it + * simply results in keeping bubbles up; we want to do this if people + * just look away for a minute, really. It can be "more aggressive" + * about idle detection than a screensaver would be. + */ This comment makes little sense in this context. "bubbles" here refer to the Mugshot notification popups. Needs to be rewritten though the idea that it can be more aggressive than a screensaver stands. +/* + * Returns lists of application names we've seen the user interacting with + * within the last 'in_last_seconds' seconds and in the specified activity. + * Free the names in the GSLists with g_free(), the lists themselves with + * g_slist_free(). There are not plural lists any more here. +static void +get_active_apps (ShellAppMonitor * monitor, + int in_last_seconds, + GSList ** wm_classes, + gchar ** activity) Is the activity return value needed? Can't the caller just use monitor->activity ? + if (monitor->upload_apps_burst_count > 0) + { + period = SAVE_APPS_BURST_TIMEOUT; + monitor->upload_apps_burst_count--; + + if (monitor->upload_apps_burst_count == 0) + { + g_source_remove (SAVE_APPS_BURST_TIMEOUT); + g_timeout_add_seconds (SAVE_APPS_TIMEOUT, on_save_apps_timeout, monitor); + } + } It looks to me that if you start out with burst_count == 0 (as you do when you have existing data), you'll stick in "burst mode" + GSList *wm_classes = NULL; + gchar *activity; + + get_active_apps (monitor, period, &wm_classes, &activity); Leaks activity. +/* Used to find an app item from its wm_class, when non empty */ +static gint +popularity_find_app (gconstpointer list_data, gconstpointer user_data) +{ + AppPopularity *list_pop = (AppPopularity *) list_data; + AppPopularity *user_pop = (AppPopularity *) user_data; + if (g_str_equal (list_pop->wm_class, user_pop->wm_class)) + return 0; + else + return 1; +} I would generally only use g_str_equal as an argument to g_hash_table_new() - use strcmp() for checking for equality in normal code. (There is special cased GCC optimization, etc.) Here you can just do: return strcmp (list_pop->wm_class, user_pop->wm_class) +/* Used to sort highest scores at the top */ +static gint +popularity_sort_apps (gconstpointer data1, gconstpointer data2) +{ + AppPopularity * pop1 = (AppPopularity *) data1; + AppPopularity * pop2 = (AppPopularity *) data2; ^ Extra space, as well as earlier comment about abbreviating popularity to pop. +/* Limit the score to a certain level so that most popular apps can change */ +static void +normalise_popularity (GSList *list) American English: "normalize" + /* Highest score since list is sorted */ + app_pop = (AppPopularity *) (list->data); + /* Limiting score allows new apps to catch up (see SCORE_MAX definition) */ + if (app_pop->score > SCORE_MAX) + while (list) + { + app_pop = (AppPopularity *) (list->data); + app_pop->score /= 2; + list = list->next; + } So, the algorithm is roughly speaking: - Every hour add one to the score of each app that was used within the last hour - When one app reaches 50, divide all scores by 2 ? I certainly don't have as good an intuitive feel for this algorithm as the online-desktop algorithm that I described on IRC earlier, but we can see how it works out. Can you add a block comment to the top of the file describing how the score is computed? +/* Save apps data internally to lists, merging if necessary */ +static void +save_active_apps (ShellAppMonitor *monitor, int collection_period, + GSList * wm_classes, gchar *activity) +{ + AppPopularity *app_pop; + AppPopularity temp; /* We only set/use two fields here */ + GDate *date; + guint32 date_days; + GSList *popularity; I would all this list 'popularities' not 'popularity' + GSList *item; + + g_assert (wm_classes); I don't like this assert - what happens if the user has used nothing? "No active apps" seems perfectly valid to me. I see that you guard it in the caller, but I don't see any reason it *should* be guarded in the caller. I see you rely on wm_classes != NULL below, I don't seee any reason you *should* rely on that :-) + popularity = g_hash_table_lookup (monitor->popularities, activity); + date = g_date_new (); + g_date_set_time_t (date, time (NULL)); + date_days = g_date_get_julian (date); + if (!popularity) /* Just create the list using provided information */ + { + do + { + app_pop = g_new (AppPopularity, 1); + app_pop->last_seen = date_days; + app_pop->score = 1; + /* Copy data from the old list */ + if (wm_classes) + app_pop->wm_class = g_strdup((gchar *) wm_classes->data); + else + app_pop->wm_class = ""; + popularity = g_slist_prepend (popularity, app_pop); + } + while ( (wm_classes = g_slist_next(wm_classes)) ); - Use an 'l' temporary instead of mutating the value passed in, changing values passed in can result in bugs, as we saw earlier - Just use standard for (l = wm_classes; l; l = l->next) - Don't use g_[s]list_next, they just obscure the linked-list nature + else /* Merge with old data */ + { + do + { + if (wm_classes) + temp.wm_class = (gchar *) wm_classes->data; + else + temp.wm_class = ""; Don't see how wm_classes could be null here or below. + popularity = g_slist_sort (popularity, (GCompareFunc) &popularity_sort_apps); The & is extraneous (though harmless) here. I would advise against casting to GCompareFunc, since it will disguise problems if you get your signature wrong ... just make the function signature match GCompareFunc. +/* Clean up apps we see rarely. + * The logic behind this is that if an app was seen less than 10 times + * and not seen for a week, it can probably be forgotten about. + * This should much reduce the size of the list and avoid 'pollution'. */ SCORE_MIN is 5 actually. How does this interact with the normalize step? Say I had an app that I was using quite a bit and had a score of say 20. Then I don't use it for a while. Firefox is going to chug right up at a score of 5-6 a day. So, every 5 days I'm going to normalize. (50 => 25) That means that in 2-3 weeks the app will be pruned. Anyways, should be described in the requested bock comment as well. + while (list) + { + next = list->next; + app_pop = (AppPopularity *) (list->data); + if ((app_pop->score < SCORE_MIN) && (app_pop->last_seen < date_days)) + head = g_slist_remove (head, list); + list = next; + } Hmm a bit inefficient but should be fine, too bad we don't have g_slist_remove_foreach() (bug 538584). +/* Save app data lists to file */ +static void +save_to_file (ShellAppMonitor *monitor) +{ + GHashTableIter iter; + gchar *activity; + GSList *popularity; + AppPopularity *app_pop; + GFileOutputStream *output; + GDataOutputStream *data_output; + GError *error = NULL; + gchar *line; + gchar score_buf[G_ASCII_DTOSTR_BUF_SIZE]; + + /* Parent directory is already created by shell-global */ + output = g_file_replace (monitor->configfile, NULL, FALSE, G_FILE_CREATE_NONE, NULL, &error); + if (!output) + { + g_warning ("Could not save applications usage data: %s.\n", error->message); g_warning() doesn't require a trailing \n + g_error_free (error); + return; + } + data_output = g_data_output_stream_new (G_OUTPUT_STREAM(output)); ^ Missing ' ' (there are some more around in the file) + g_object_unref (output); + + g_hash_table_iter_init (&iter, monitor->popularities); + while (g_hash_table_iter_next (&iter, (gpointer *) &activity, (gpointer *) &popularity) + && popularity) + { + /* Clean once in a while */ + popularity = clean_popularity (popularity); + popularity = g_slist_sort (popularity, (GCompareFunc) &popularity_sort_apps); Doesn't this break your hash table if either of these modifies the head of your popularity list? In general, I think it's probably not a good idea to have the 'save' function have side effects other than saving. + line = g_strdup_printf ("%s,%s,%I32u\n", app_pop->wm_class, + score_buf, app_pop->last_seen); Hmm, I don't even know what %I32u does. %u should be fine, right? + g_data_output_stream_put_string (data_output, line, NULL, NULL); Should check for errors on this as well. + g_free (line); + } + while ( (popularity = g_slist_next(popularity)) ); + } + g_output_stream_close (G_OUTPUT_STREAM(data_output), NULL, &error); + if (error) + { + g_warning ("Could not save applications usage data: %s.\n", error->message); + g_error_free (error); + return; + } You leak data_output on error. + g_object_unref (data_output); + return ; ^ Stray ' '. But you don't need a return at the end of the function. +} + +/* Load data about apps usage from file */ +static void +restore_from_file (ShellAppMonitor *monitor) +{ + gchar *activity = NULL; + GSList *popularity; + AppPopularity *app_pop; + GFileInputStream *input; + GDataInputStream *data_input; + GError *error = NULL; + gchar *line; + gchar **info; + + input = g_file_read (monitor->configfile, NULL, &error); + if (error) /* Fail silently, error message would be useless (warn on saving) */ Ignoring @G_IO_ERROR_NOT_FOUND makes sense to me, but other errors probably represent bugs, and better to have warnings then obscure problems that are hard to track down? + while ( (line = g_data_input_stream_read_line (data_input, NULL, NULL, &error)) ) + { + if (error) + goto error; line should be null if error is set. I'd probably write: while (TRUE) { line = ... if (line == NULL) break To avoid having the assignment inside the conditional. + if (line && g_str_equal (line, "--")) /* Line starts a new activity */ ^^^^ How could line be null? + { + g_free (line); + line = g_data_input_stream_read_line (data_input, NULL, NULL, &error); + if (line && !g_str_equal (line, "")) + { + if (activity) /* Save previous activity, in right order */ + { + popularity = g_slist_reverse (popularity); + g_hash_table_insert (monitor->popularities, activity, popularity); + } + activity = line; + } + else if (error) + goto error; Leaks line and activity. + else /* Unexpected error, go out */ Better to say something like "-- at end of file" - you know what the issue is, why be vague and say "unexpected error"? + { + g_free (line); + g_input_stream_close (G_INPUT_STREAM(data_input), NULL, NULL); + g_object_unref (data_input); + return; + } + } + /* Line is about an app. + * If no activity was provided yet, just skip */ + else if (activity && line && !g_str_equal (line, "")) + { + int i; + + info = g_strsplit (line, ",", 3); Do you want the limit here - that will make "a,b,c,d" Split as "a" "b" "c,d", which doens't seem more likely to be right than splitting to 4 fields. + if (info[0] && info [1] && info[2]) /* Skip on wrong syntax */ + { + app_pop = g_new (AppPopularity, 1); + app_pop->wm_class = g_strdup(info[0]); + app_pop->score = g_ascii_strtod (info[1], NULL); + app_pop->last_seen = (guint32) strtoul (info[2], NULL, 10); + popularity = g_slist_prepend (popularity, app_pop); + } + + for (i = 0; i == 2; g_free (info[++i])); + g_free (info); Use g_strfreev() + if (activity) /* Save last activity, in the right order */ + { + popularity = g_slist_reverse (popularity); + g_hash_table_insert (monitor->popularities, activity, popularity); + } + if (!error) + return; Leaks data_input. + error: /* For GIO errors, fail silently too */ + g_error_free (error); + g_input_stream_close (G_INPUT_STREAM(data_input), NULL, NULL); + g_object_unref (data_input); + return; +/* Get the most popular applications for a given activity */ +GSList * +shell_app_monitor_get_apps (ShellAppMonitor *monitor, const gchar *activity, gint number); Same line breaking problems as for prototypes inthe C file. + /* Ensure config dir exists for later use */ + global->configdir = g_build_filename (g_get_user_config_dir (), "gnome-shell", + NULL); + conf_dir = g_file_new_for_path (global->configdir); + g_file_make_directory (conf_dir, NULL, NULL); + g_object_unref (conf_dir); } Hmm, configdir is distinctly weird for this, but I guess there is no ~/.local/var
Hey, looking through the code, it looks really good. But of course, I managed > to find a few things :-) Mot of this is details, style issues, and style > suggestions rather than major problems. > Thanks for doing this boring exercice so consciously! I'm glad to know you find it nice. Let's fix what need to be before we forget about them and don't want to dive into it again. I've fixed all you listed, here are the parts I wanted to comment. + // Ask for more apps than needed in case of problems > + var apps = this._appMonitor.get_apps ("default", > + this._maxItems + 5); > > Should describe what these 'problems' might be. Also, I think I'd like to see > the 5 as a constant rather than inline. > Actually, using a coefficient makes more sense here. I'm using 1.5 now, I don't think we need a constant for it since that's really a random number which does not matter because we choose it much too high to avoid any problem. I added the comment to explain that these problems are with .desktop file not matching ID. +/* Above this level, we divide all scores to reduce them. > + * This allows the popularity order to change as new apps are used more often. > + * With this value, an app goes from bottom to top of the > + * popularity list in 50 hours of use */ > +#define SCORE_MAX SAVE_APPS_TIMEOUT/3600*50 > > This looks fishy - A/B*C == (A/B)*C, so you have (1/50) hour not 50 hours. Also > any time you have a complicated #define like this you should enclose it in > parentheses. > I've switched to using g_timeout_add_seconds(), so SAVE_APPS_TIMEOUT is now 3600. That means SCORE_MAX=(3600/3600)*50=50, which is reached in 50 hours (thus the way of writing it). I'm not sure this behavior (and the whoe algorithm, see below) is perfect, but it should work for now, and I'd like to test it in real world to see what can be best. I've added parentheses. Also, I'd recommend ordering the file as: > > - Struture typedefs > - Structure definitions > - Forward definitions of functions (function prototypes) > - Global data > - Code > What do you mean by "global data"? title_patterns? I'd rather put it with the #ifdefs, it's easier to find. Else I've applied that scheme. - signals[CHANGED] = > - g_signal_new ("changed", > + signals[APPS_CHANGED] = > + g_signal_new ("apps-changed", > SHELL_TYPE_APP_MONITOR, > G_SIGNAL_RUN_LAST, > - G_STRUCT_OFFSET (ShellAppMonitorClass, changed), > + G_STRUCT_OFFSET (ShellAppMonitorClass, apps_changed), > NULL, NULL, > g_cclosure_marshal_VOID__VOID, > G_TYPE_NONE, 0); > + > + signals[POPULARITY_CHANGED] = > + g_signal_new ("popularity-changed", > + SHELL_TYPE_APP_MONITOR, > + G_SIGNAL_RUN_LAST, > + 0, > + NULL, NULL, > + g_cclosure_marshal_VOID__VOID, > + G_TYPE_NONE, 0); > +} > > These should be consistent about whether they use a class member for the > default handler or not - the old one did, your new signal doesn't. I think not > having it is slightly better (we aren't going to subclass from C, and thats's > the only point of having the default handler.) But mostly it needs to be > consistent. > I did not understand what this offest was, so I omitted it. ;-) I remove the other one then. - g_signal_connect (monitor, "changed", G_CALLBACK (on_monitor_changed), > self); > + g_signal_connect (monitor, "apps-changed", > + G_CALLBACK (on_monitor_apps_changed), self); > > I don't think you meant to change this connection. (Doesn't it give a critical > warning?) > No I didn't, and yes it did... ;-) > + if (g_hash_table_size(self->popularities)) > + self->upload_apps_burst_count = 0; > > I never use if (x) when I mean (x != 0) numerically. > > if (g_hash_table_size(self->popularities) > 0) > > I think is clearer. > > + g_list_foreach (self->desktop_dir_monitors, (GFunc) g_object_unref, NULL); > + g_list_free (self->desktop_dir_monitors); > + save_to_file (self); > > Would recommend calling save_to_file before you start freeing things. (Also, > would recommend saving in a timeout - getting finalize called at shutdown will > be at best unreliable.) > Removing it, saving is already done with timeout. This line is a remnant from early experiments. Hmm, a MutterPlugin is per GdkScreen, not per GdkDisplay. On the other hand, I > can't see running a separate GnomeShell instance on multiple screens of the > same display. Let's leave it like this for now. Nobody uses multiple screens > anyways :-) > No developer does, so let's forget it. But one day or another we'll need to support it. I would suggest using g_hash_table_replace() here - g_hash_table_insert() has > weird behavior when overwriting an existing key/value pair (it uses the old key > and the new value, and frees the new key and the old value. WHich ends up > working here, but is unexpected and likely to lead to future bugs.) > I know that behavior is strange, but I don't think that can be a problem: freeing now or later doesn't change much. Anyway, it should be marked as deprecated... Fixed. +static void > +get_active_apps (ShellAppMonitor * monitor, > + int in_last_seconds, > + GSList ** wm_classes, > + gchar ** activity) > > Is the activity return value needed? Can't the caller just use > monitor->activity ? > That's an open issue. For now activites are useless so I'd keep it as is, but in the future that's going to be much more complex. monitor->activity should be removed in favor of a timetable, because activities can switch quite quickly if they're associated with workspaces. Because of this, I'd like to pass either a timestamp or an activity to functions to avoid strange effects. +/* Save apps data internally to lists, merging if necessary */ > +static void > +save_active_apps (ShellAppMonitor *monitor, int collection_period, > + GSList * wm_classes, gchar *activity) > +{ > + AppPopularity *app_pop; > + AppPopularity temp; /* We only set/use two fields here */ > + GDate *date; > + guint32 date_days; > + GSList *popularity; > > I would all this list 'popularities' not 'popularity' > I used "popularities" to call the hash table containing popularity list for each activity. So we have: app_popularity -> by app popularity -> by activity popularites -> the top-level table That can be confusing, but it's the best I've found - and at least I've used it consistently over the file. + GSList *item; > + > + g_assert (wm_classes); > > I don't like this assert - what happens if the user has used nothing? "No > active apps" seems perfectly valid to me. I see that you guard it in the > caller, but I don't see any reason it *should* be guarded in the caller. I see > you rely on wm_classes != NULL below, I don't seee any reason you *should* rely > on that :-) > That was another piece of code remaining from early test phase. Obviously here it's stupid, the only time we call this function, we check for wm_classes before. I'd have found it a nice message on crash: "The desktop shell has just found you had not used any application in the last in_last_seconds seconds. Crashing." :-) + else /* Merge with old data */ > + { > + do > + { > + if (wm_classes) > + temp.wm_class = (gchar *) wm_classes->data; > + else > + temp.wm_class = ""; > > Don't see how wm_classes could be null here or below. > True. That was from the time we had app_ids too, and that one of them could be null. + g_object_unref (output); > + > + g_hash_table_iter_init (&iter, monitor->popularities); > + while (g_hash_table_iter_next (&iter, (gpointer *) &activity, (gpointer *) > &popularity) > + && popularity) > + { > + /* Clean once in a while */ > + popularity = clean_popularity (popularity); > + popularity = g_slist_sort (popularity, (GCompareFunc) > &popularity_sort_apps); > > Doesn't this break your hash table if either of these modifies the head of your > popularity list? In general, I think it's probably not a good idea to have the > 'save' function have side effects other than saving. > That was definitely breaking things, I don't know how that worked despite this. I've moved those to restore_from_file. The rationale was that doing things on startup is generally bad because it slows boot process. Here we're dealing with milliseconds, so it's not really a problem. I've added a call to it in save_active_apps(), because on systems that hibernate, ideally we should almost never restart. + line = g_strdup_printf ("%s,%s,%I32u\n", app_pop->wm_class, > + score_buf, app_pop->last_seen); > > Hmm, I don't even know what %I32u does. %u should be fine, right? > Yes, it's just that last_seen is technically a guint32. But I found out that all around the GLib, guint32 were used as guint, so we can treat them as such. Strange detail. Concerning error reporting, I've added a check so that saving errors are reported once, and no more if the code is always the same. I think this makes sense because logs are filled every hour if something goes wrong. + /* Ensure config dir exists for later use */ > + global->configdir = g_build_filename (g_get_user_config_dir (), > "gnome-shell", > + NULL); > + conf_dir = g_file_new_for_path (global->configdir); > + g_file_make_directory (conf_dir, NULL, NULL); > + g_object_unref (conf_dir); > } > > Hmm, configdir is distinctly weird for this, but I guess there is no > ~/.local/var > Yes, that's not very clear to me either. I thought .cache could be good, but normally cache is only for data you can recover automatically, only with machine work. Here we don't want to lose this data. So the best is config - session state is already stored in .gnome/session, and I guess gnome-shell will go there too at some point. About XDG base dirs, I think it could be nice to check in XDG_CONFIG_DIRS for a system-wide file. That would allow admins to preseed the stats for their users to see the rights apps first. And then we would immediately save to our current file. Is that how things should work? And, hell, I've got to the end of this list! Cool! ;-)
// We still need to determine what events other than search can trigger // a change in the set of applications that are being shown while the // user in in the overlay mode, however let's redisplay just in case. - me._redisplay(false); - }); + this._redisplay(); Is the removal of the parameter a mismerge? There's also a conflict marker in appDisplay.js on your branch. + // Ask for more apps than needed in case of problems, + // i.e. failing to match .desktop file with WM class Maybe just be more verbose: "Ask or more app than we need, since the list of recently used apps might contain an app we don't have a desktop file for". Or something like that. + this._maxItems * 1.5); Probably clearer to explicitly round to an integer. > > This looks fishy - A/B*C == (A/B)*C, so you have (1/50) hour not 50 hours. > > Also any time you have a complicated #define like this you should enclose > > it in parentheses. > > I've switched to using g_timeout_add_seconds(), so SAVE_APPS_TIMEOUT is > now 3600. That means SCORE_MAX=(3600/3600)*50=50, which is reached in 50 > hours (thus the way f writing it). I'm not sure this behavior (and the whole > algorithm, see below) is perfect, but it should work for now, and I'd like to > test it in real world to see what can be best. I've added parentheses. And if SAVE_APP_TIMEOUT was 1800, the score would increase twice as fast, but your computation of SCORE_MAX would halve? So, maybe what you meant was (3600*50)/SAVE_APPS_TIMEOUT? (Which is only coincidentally the same because of SAVE_APPS_TIMEOUT is 3600 at the moment.) > What do you mean by "global data"? title_patterns? I'd rather put it with > the #ifdefs, it's easier to find. Else I've applied that scheme. Yes, that was the particular example. Putting it with the #defines is OK too. +static struct title_pattern +{ You don't need the struct name here and should omit it. (If you were including a name, our style would be _TitlePattern) - g_signal_connect (monitor, "changed", G_CALLBACK (on_monitor_changed), self); - self->priv->desktop_dir_monitors - = g_list_prepend (self->priv->desktop_dir_monitors, - monitor); + g_signal_connect (self, "apps-changed", + G_CALLBACK (on_monitor_apps_changed), self); + self->desktop_dir_monitors = g_list_prepend (self->desktop_dir_monitors, + monitor); No, no, no :-) Just leave it it was. It was a different changed signal. +GSList *shell_app_monitor_get_apps (ShellAppMonitor *monitor, + const gchar *activity, + gint max_count) +{ Miscommunication here. There's a difference between prototypes (function forward declaration) which are formatted as above, and function definitions which are formatted as: === GSList * shell_app_monitor_get_apps (ShellAppMonitor *monitor, const gchar *activity, gint max_count) { === + for (l = list; l; l = l->next) + { + app_popularity = (AppPopularity *) l->data; + g_free (app_popularity->wm_class); + g_free (app_popularity); + } {} need to be in two spaces further here. > Yes, it's just that last_seen is technically a guint32. But I found out > that all around the GLib, guint32 were used as guint, so we can treat them > as such. Strange detail. guint will always be at least 32 bits. If you are worried about running on a cray with 64-bit integers ;-) then I'd just make last_seen a guint. > About XDG base dirs, I think it could be nice to check in XDG_CONFIG_DIRS > for a system-wide file. That would allow admins to preseed the stats for > their users to see the rights apps first. And then we would immediately > save to our current file. Is that how things should work? I don't think asking sysadmins create a fake usages file is going to be an obvious way to handle things. We haven't discussed what to do before there is a useful amount of date. My sense is that we should have the apps list be: A) <Apps user has explicitly pinned> B) <Users most frequent apps removing the pinned ones> C) <Fixed list of apps removing the ones from the most frequent list and pinned apps> And maybe only have the "most frequent apps" part after we've finished the initial burst. A) and C) are what a sysadmin might want to configure and lock down, so the natural place for them is GConf, which is the GNOME config system. We aren't yet handling dealing with GConf, but if that's the eventual goal, then looking in XDG_DATA_DIRS doens't seem like a useful intermediate step.
Here I am, amid rather busy times I've found a few hours to fix these bugs. // We still need to determine what events other than search can > trigger > // a change in the set of applications that are being shown while > the > // user in in the overlay mode, however let's redisplay just in > case. > - me._redisplay(false); > - }); > + this._redisplay(); > > Is the removal of the parameter a mismerge? There's also a conflict marker in > appDisplay.js on your branch. The 'false' parameters seem to have been missing for a very long time on my branch. I'm not sure they have not been introduced since then. Anyway, now they're back. The conflict marker is fixed too (always test before committing !)... + // Ask for more apps than needed in case of problems, > + // i.e. failing to match .desktop file with WM class > > Maybe just be more verbose: "Ask or more app than we need, since the list of > recently used apps might contain an app we don't have a desktop file for". Or > something like that. Done. + this._maxItems * 1.5); > > Probably clearer to explicitly round to an integer. Done, and I'm now using a MAX_ITEMS constant which is now set to 30, because this._maxItems has become this._maxItemsPerPage, so I needed another count. Maybe that would deserve a GConf option at some point. > > This looks fishy - A/B*C == (A/B)*C, so you have (1/50) hour not 50 hours. > > > Also any time you have a complicated #define like this you should enclose > > > it in parentheses. > > > > I've switched to using g_timeout_add_seconds(), so SAVE_APPS_TIMEOUT is > > now 3600. That means SCORE_MAX=(3600/3600)*50=50, which is reached in 50 > > hours (thus the way f writing it). I'm not sure this behavior (and the whole > > algorithm, see below) is perfect, but it should work for now, and I'd like to > > test it in real world to see what can be best. I've added parentheses. > > And if SAVE_APP_TIMEOUT was 1800, the score would increase twice as fast, but > your computation of SCORE_MAX would halve? > > So, maybe what you meant was (3600*50)/SAVE_APPS_TIMEOUT? (Which is only > coincidentally the same because of SAVE_APPS_TIMEOUT is 3600 at the moment.) You got it! Funny to see how I was confused... Else we would have got weird results when tweaking those values. > What do you mean by "global data"? title_patterns? I'd rather put it with > > the #ifdefs, it's easier to find. Else I've applied that scheme. > > Yes, that was the particular example. Putting it with the #defines is OK too. > > +static struct title_pattern > +{ > > You don't need the struct name here and should omit it. (If you were including > a name, our style would be _TitlePattern) That must have come from my first attempts to get this complex declaration work. ;-) > - g_signal_connect (monitor, "changed", G_CALLBACK (on_monitor_changed), > self); > - self->priv->desktop_dir_monitors > - = g_list_prepend (self->priv->desktop_dir_monitors, > - monitor); > + g_signal_connect (self, "apps-changed", > + G_CALLBACK (on_monitor_apps_changed), self); > + self->desktop_dir_monitors = g_list_prepend (self->desktop_dir_monitors, > + monitor); > > No, no, no :-) Just leave it it was. It was a different changed signal. This time I think YOU're wrong! Actually, I've renamed the original "changed" signal to "apps-changed", to make it more consistent with the "popularity-changed" signal. I fixed these lines because I had a runtime warning about that missing signal. +GSList *shell_app_monitor_get_apps (ShellAppMonitor *monitor, > + const gchar *activity, > + gint max_count) > +{ > > Miscommunication here. There's a difference between prototypes (function > forward declaration) which are formatted as above, and function definitions > which are formatted as: > > === > GSList * > shell_app_monitor_get_apps (ShellAppMonitor *monitor, > const gchar *activity, > gint max_count) > { > === > > + for (l = list; l; l = l->next) > + { > + app_popularity = (AppPopularity *) l->data; > + g_free (app_popularity->wm_class); > + g_free (app_popularity); > + } > > {} need to be in two spaces further here. Indentation fixed. > > Yes, it's just that last_seen is technically a guint32. But I found out > > that all around the GLib, guint32 were used as guint, so we can treat them > > as such. Strange detail. > > guint will always be at least 32 bits. If you are worried about running on a > cray with 64-bit integers ;-) then I'd just make last_seen a guint. last_seen is obtained from a GTime, so it has to be pseudo-converted all the time. The easiest way is to "convert" it when saving and loading. > > About XDG base dirs, I think it could be nice to check in XDG_CONFIG_DIRS > > for a system-wide file. That would allow admins to preseed the stats for > > their users to see the rights apps first. And then we would immediately > > save to our current file. Is that how things should work? > > I don't think asking sysadmins create a fake usages file is going to be an > obvious way to handle things. We haven't discussed what to do before there is a > useful amount of date. My sense is that we should have the apps list be: > > A) <Apps user has explicitly pinned> > B) <Users most frequent apps removing the pinned ones> > C) <Fixed list of apps removing the ones from the most frequent list and > pinned apps> > > And maybe only have the "most frequent apps" part after we've finished the > initial burst. > > A) and C) are what a sysadmin might want to configure and lock down, so the > natural place for them is GConf, which is the GNOME config system. We aren't > yet handling dealing with GConf, but if that's the eventual goal, then looking > in XDG_DATA_DIRS doens't seem like a useful intermediate step. OK, I was just wondering before we consider it as done. I've added some code so that the list is filled with default applications if AppMonitor does not provide enough for that. While I agree with your proposal, I think I would be nice to let admins set the default rather than tweaking A). This makes more sense since admins want to present new users with likely-to-be-used apps, but these should be overridden by the user's actual work. If we consider these as "explicitly pinned", he'll need to remove them by hand. The best way to do this is to get the list of default apps from a config file instead of hardcoding them, which is kind of ugly. Hope everything works now!
I see two major parts to this bug: 1) Landing the infrastructure for application tracking 2) Using it in the UI What I'd like to do for 1) is make it a separate class (or alternatively, we could rename the current ShellAppMonitor to e.g. ShellMenuSystem; I think I'll do this now). Milan what do you think about consolidating/rebasing your patches? For example squashing some of the "minor fixes" commits together. Even better if possible is to separate them into layers, though I'm not sure what a clean division would be. Possibly something like * Data structures for monitoring, and record into memory * State saving support * API We should discuss the UI too, but it's probably better to do it on the mailing list via mockups.
1) About the infrastructure, renaming it is not a problem for me, just pick up the name you want... But IMHO ShellMenuSystem is not accurate, it should have something to do with app usage tracking/monitoring, stats... 2) For now, recent apps appear at the top of the list, then come default apps. That's pretty basic but should work well. I guess we need a few features like setting an app to a specific permanent position, and removing an unwanted app from the list. That's all I can think of... About the git history: sure, many small commits are kind of ugly. It can make sense to merge them into 3 commits as you say. Or even only two, since I'm not sure it makes sense to split the commits about the main sources files; the UI changes that affect appDisplay.js should obviously be separated. Is there a nice way to merge commits together with GIT?
About renaming current ShellAppMonitor: I understand what you meant now! :-p Since I had not looked at the new menu code, I did not understand that you talked about the current *master* file. I've split the code, moving menu part to shell-menu-system.{c,h} and ShellMenuSystem, as you said. New features in app-monitor: - GConf option to disable app monitoring. I'm not really sure the code I added to install schemas is working, but it should and is quite simple. - activity IDs are now int, that's what I'm using in the activity branch I'll upload soon. - moved config files to ~/.gnome2/shell/ I've noticed a strange line in code taken from Hippo: > g_source_remove (SAVE_APPS_BURST_TIMEOUT); which should remove a timer added with > g_timeout_add_seconds (SAVE_APPS_BURST_TIMEOUT, on_save_apps_timeout, self); Shoudn't we remove it using the return value of the function? Other than that, I think the code is ready to be squashed into cleaner commits, if we agree on an ideal plan (shell-app-monitor.{c,h}, shell-global.c, autotools files in one part, appDisplay.js in another?).
Yeah, that line is definitely wrong. Let me figure out this strange build problem and then I'll land a rename and we can go from there.
I've committed the rename, do you think you could rebase again? As for the split I think at least two (C+autoconf, js), but we may want more. For example I think it would be a good idea to split out the infrastructure to add GConf as a separate commit.
No problem with the rebase, I'll do this some time soon (hope that's not too tricky...). I can split the GConf code apart, yes. Anyway, it's two commits that I can merge together, keeping the result separate. I don't know if splitting the file saving/restoring parts is worth the pain, since it's the very same file, and no new dependency/feature. About the line > g_source_remove (SAVE_APPS_BURST_TIMEOUT); I'll fix it using the timer ID. Another little concern: the timer poll_for_idleness is called every 5 sec. I don't think this interval was chosen scientifically, so we may question it: maybe 10 sec would be sufficient, considering that we only merge data (to change an app's score or not) every hour. Or even choose a much longer delay? I don't like the idea of something constantly running and waking up CPU, limiting it to the minimum would be nice.
OK, I've rebased all that commits into 3 parts, and pushed it to a new branch called app-monitor-new: http://www.gitorious.org/projects/gnome-shell/repos/mainline/logs/app-monitor-new (hope that will be visible soon). I can still rebase-edit them to fix a few things, such as the timer question I raised above, or anything you may find.
Another point, in appDisplay.js:235: > this._appsStale = true; Did you actually mean to integrate this line into the callback? Just to be sure.
(In reply to comment #20) > Another point, in appDisplay.js:235: > > this._appsStale = true; > Did you actually mean to integrate this line into the callback? Just to be > sure. Yep, that line has been there for a while according to annotate, I believe it's right. On your rebased tree: generally looks great! Might be good to split out the #define SHELL_GCONF_DIR "/desktop/gnome/shell" and marshaller file into an earlier "infrastructure" commit. Other than that let's get this in - I forget, do you have a GNOME account?
I've split the global configuration dir code into a first "infrastructure" commit. The *GConf* dir is now in the GConf commit, it's better this way. The marshallers were wrong, I don't need them anymore. And additionally, I'm now a git-rebase guru! No, I don't have a GNOME account, but I'm sure somebody can do it for me. Git makes it really nice to work this way. Everything is force-updated on the Gitorious -new branch.
I've merged these commits now (hopefully in the right way). Let's open new bugs for futher enhancements. Thanks!
Thanks! I hope you'll like it!
Oops! I realize I've forgotten that gconf_client_get_default() increased the ref count on the client, and needed a g_object_unref when done.That's obviously not a bug deal I practice, but still. I can provide a simple patch soon, but maybe it could be good that somebody double checks that (short) GConf code, since I may have done more significant mistakes... http://git.gnome.org/cgit/gnome-shell/commit/?id=216db2bb12ff4179cdd5f10e72755703f0ff25b4