Merge lp:~ted/indicator-session/removing-me into lp:indicator-session/0.1

[Ted Gould]

[Ted Gould (ted)]

This branch started with just wanting to ensure that there isn't a menu
item for the currently logged in user. But, then there's more.

Ended up restructuring the code slightly so that we could also detect to
see if the guest user is logged in. This involved making count and
users variables not global which allowed for simplifying the callbacks
for user added and removed.

[David Barth (dbarth)]

Hmm, it feels you should avoid going into the loop if the user_count is wrong, to avoid some pathological cases. I'd prefer to cut processing as soon as possible, and check for the guest account explicitly.

review: Needs Information

[Ted Gould (ted)]

On Tue, 2010-03-02 at 20:31 +0000, David Barth wrote:
> Hmm, it feels you should avoid going into the loop if the
> user_count is wrong, to avoid some pathological cases. I'd
> prefer to cut processing as soon as possible, and check for
> the guest account explicitly.

To find the guest account we're going to have to look through the
entries until we find it, which is the same thing as when we find the
guest account we discover the issue, and break. I'm not sure that it'd
be any faster to do in another loop.

[David Barth (dbarth)]

Ok, this is a dynamic account... But that loop means that it's going to be very costly on system connected to directories with thousands of entries.

I think you should exploit that fact that a guest account is a system one, ie with an ID below LAST_SYSTEM_UID. That's in /etc/adduser.conf.

Not sure if that changes a lot, but as 'guest' is already hardcoded, either hardcoding to < 999, or sourcing the conf file to get the environment variable.

review: Needs Fixing

[Ted Gould (ted)]

On Tue, 2010-03-02 at 23:12 +0000, David Barth wrote:
> Ok, this is a dynamic account... But that loop means that
> it's going to be very costly on system connected to directories
> with thousands of entries.
>
> I think you should exploit that fact that a guest account is
> a system one, ie with an ID below LAST_SYSTEM_UID. That's in
> /etc/adduser.conf.
>
> Not sure if that changes a lot, but as 'guest' is already
> hardcoded, either hardcoding to < 999, or sourcing the conf
> file to get the environment variable.

But, we're getting all the names from GDM. I don't think that we can
request a subset of that list. I think the only way we could do it is
to specially request information on the guest account from ConsoleKit
and avoid GDM all together.

The other option would be to not show if the guest is logged in when
we're in "overflow mode."

I guess my feeling is that we're talking about thousands of string
comparisons. Which, on a modern processor is calculated in ms...

But, if you want, we can go with option A. I think it'd be the best,
just talk to ConsoleKit directly about the guest account.

[David Barth (dbarth)]

The issue is network access with a directory, not so much the number of entries. Enumerating accounts may be extremely slow. But I think it's more of gdm's role here, and so the cache should already be populated anyway.

Go ahead with the patch.

review: Approve

=== modified file 'src/session-service.c'
--- src/session-service.c	2010-02-23 22:55:45 +0000
+++ src/session-service.c	2010-03-02 20:05:22 +0000
@@ -68,9 +68,6 @@
 static DbusmenuMenuitem  *lock_menuitem = NULL;
 static DbusmenuMenuitem  *switch_menuitem = NULL;
 
-static gint   count;
-static GList *users;
-
 static DbusmenuMenuitem * root_menuitem = NULL;
 static GMainLoop * mainloop = NULL;
 static DBusGProxy * up_main_proxy = NULL;
@@ -415,6 +412,7 @@
                UsersServiceDbus *service)
 {
   DbusmenuMenuitem *mi = NULL;
+  DbusmenuMenuitem * guest_mi = NULL;
   GList *u;
   UserData *user;
   gboolean can_activate;
@@ -444,12 +442,12 @@
 
       if (check_guest_session ())
         {
-          mi = dbusmenu_menuitem_new ();
-		  dbusmenu_menuitem_property_set (mi, DBUSMENU_MENUITEM_PROP_TYPE, USER_ITEM_TYPE);
-          dbusmenu_menuitem_property_set (mi, USER_ITEM_PROP_NAME, _("Guest Session"));
-          dbusmenu_menuitem_property_set_bool (mi, USER_ITEM_PROP_LOGGED_IN, FALSE);
-          dbusmenu_menuitem_child_append (root, mi);
-          g_signal_connect (G_OBJECT (mi), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK (activate_guest_session), NULL);
+          guest_mi = dbusmenu_menuitem_new ();
+		  dbusmenu_menuitem_property_set (guest_mi, DBUSMENU_MENUITEM_PROP_TYPE, USER_ITEM_TYPE);
+          dbusmenu_menuitem_property_set (guest_mi, USER_ITEM_PROP_NAME, _("Guest Session"));
+          dbusmenu_menuitem_property_set_bool (guest_mi, USER_ITEM_PROP_LOGGED_IN, FALSE);
+          dbusmenu_menuitem_child_append (root, guest_mi);
+          g_signal_connect (G_OBJECT (guest_mi), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK (activate_guest_session), NULL);
         }
 
       if (check_new_session ())
@@ -472,39 +470,49 @@
             }
         }
 
-      if (count > MINIMUM_USERS && count < MAXIMUM_USERS)
-        {
-          if (users != NULL)
-            {
-              GList *l = NULL;
-
-              for (l = users; l != NULL; l = l->next)
-                {
-                  users = g_list_delete_link (users, l);
-                }
-
-              users = NULL;
-            }
-
-          users = users_service_dbus_get_user_list (service);
-
-          users = g_list_sort (users, (GCompareFunc)compare_users_by_username);
-
-          for (u = users; u != NULL; u = g_list_next (u))
-            {
-              user = u->data;
-
-              user->service = service;
-
-              mi = dbusmenu_menuitem_new ();
-              dbusmenu_menuitem_property_set (mi, DBUSMENU_MENUITEM_PROP_TYPE, USER_ITEM_TYPE);
-              dbusmenu_menuitem_property_set (mi, USER_ITEM_PROP_NAME, user->real_name);
-			  dbusmenu_menuitem_property_set_bool (mi, USER_ITEM_PROP_LOGGED_IN, user->sessions != NULL);
-              dbusmenu_menuitem_child_append (root, mi);
-              g_signal_connect (G_OBJECT (mi), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK (activate_user_session), user);
-            }
-        }
-    }
+		GList * users = NULL;
+		users = users_service_dbus_get_user_list (service);
+		guint user_count = g_list_length(users);
+
+		if (user_count > MINIMUM_USERS && user_count < MAXIMUM_USERS) {
+			users = g_list_sort (users, (GCompareFunc)compare_users_by_username);
+		}
+
+		for (u = users; u != NULL; u = g_list_next (u)) {
+			user = u->data;
+			user->service = service;
+
+			if (user->uid == getuid()) {
+				/* Hide me from the list */
+				continue;
+			}
+
+			if (g_strcmp0(user->user_name, "guest") == 0) {
+				/* Check to see if the guest has sessions and so therefore should
+				   get a check mark. */
+				if (user->sessions != NULL) {
+					dbusmenu_menuitem_property_set_bool (guest_mi, USER_ITEM_PROP_LOGGED_IN, TRUE);
+				}
+				/* If we're showing user accounts, keep going through the list */
+				if (user_count > MINIMUM_USERS && user_count < MAXIMUM_USERS) {
+					continue;
+				}
+				/* If not, we can stop here */
+				break;
+			}
+
+			if (user_count > MINIMUM_USERS && user_count < MAXIMUM_USERS) {
+				mi = dbusmenu_menuitem_new ();
+				dbusmenu_menuitem_property_set (mi, DBUSMENU_MENUITEM_PROP_TYPE, USER_ITEM_TYPE);
+				dbusmenu_menuitem_property_set (mi, USER_ITEM_PROP_NAME, user->real_name);
+				dbusmenu_menuitem_property_set_bool (mi, USER_ITEM_PROP_LOGGED_IN, user->sessions != NULL);
+				dbusmenu_menuitem_child_append (root, mi);
+				g_signal_connect (G_OBJECT (mi), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK (activate_user_session), user);
+			}
+		}
+
+		g_list_free(users);
+	}
 
 	DbusmenuMenuitem * separator = dbusmenu_menuitem_new();
 	dbusmenu_menuitem_property_set(separator, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
@@ -562,42 +570,13 @@
 /* Signal called when a user is added.  It updates the count and
    rebuilds the menu */
 static void
-user_added (UsersServiceDbus *service,
-            UserData         *user,
-            gpointer          user_data)
-{
-  DbusmenuMenuitem *root = (DbusmenuMenuitem *)user_data;
-
-  count++;
-
-  rebuild_items (root, service);
-}
-
-/* Signal called when a user is deleted.  It updates the count and
-   rebuilds the menu */
-static void
-user_removed (UsersServiceDbus *service,
-              UserData         *user,
-              gpointer          user_data)
-{
-  DbusmenuMenuitem *root = (DbusmenuMenuitem *)user_data;
-
-  count--;
-
-  rebuild_items (root, service);
-}
-
-/* Wrapper around rebuild_items that is used on the first call
-   so that we can initialize the count variable. */
-static void
-create_items (DbusmenuMenuitem *root,
-              UsersServiceDbus *service)
-{
-  g_return_if_fail (IS_USERS_SERVICE_DBUS (service));
-
-  count = users_service_dbus_get_user_count (service);
-
-  rebuild_items (root, service);
+user_change (UsersServiceDbus *service,
+             UserData         *user,
+             gpointer          user_data)
+{
+	DbusmenuMenuitem *root = (DbusmenuMenuitem *)user_data;
+	rebuild_items (root, service);
+	return;
 }
 
 /* When the service interface starts to shutdown, we
@@ -638,15 +617,15 @@
 
     dbus_interface = g_object_new (USERS_SERVICE_DBUS_TYPE, NULL);
 
-    create_items (root_menuitem, dbus_interface);
+    rebuild_items (root_menuitem, dbus_interface);
 
     g_signal_connect (G_OBJECT (dbus_interface),
                       "user-added",
-                      G_CALLBACK (user_added),
+                      G_CALLBACK (user_change),
                       root_menuitem);
     g_signal_connect (G_OBJECT (dbus_interface),
                       "user-removed",
-                      G_CALLBACK (user_removed),
+                      G_CALLBACK (user_change),
                       root_menuitem);
 
 	setup_up();