Code review comment for lp:~bratsche/indicator-session/users-service

Revision history for this message
Ted Gould (ted) wrote :

      * In users-service.c I don't see where count is updated. I think
        it should be incremented/decremented on user added/removed.
      * There should be icons on the users if they are set (probably
        should go in another branch/bug)
      * There doesn't seem to be any sorting of the user list. They
        should be sorted alphabetical by first name.
      * There are six signals defined in the Class structure but only 4
        created on class init.
      * I don't think that you need a call to
        dbus_g_object_type_install_info, that should only be needed if
        this object is being shared over DBus (server) but I think it's
        only a client.
      * Confirm with Robert, but I'm pretty sure the excludes list is
        already being applied on the GDM side. We shouldn't need to
        apply it on this side as well. That way there's only one copy
        of the list.
      * I think that dbus_g_connection_register_g_object can be dropped
        as well. Which I think is the only reason you're using the
        session bus, so I think that can be dropped.
      * Oh, now realizing more of the issue. Makefile.am shouldn't even
        be generating user-service-server.h. Drop that file.
      * I don't think that dbus_proxy_system and dbus_proxy_session are
        needed as we're not asking DBus itself for any information.
        We're just using it to communicate. We'd only need those
        proxies to query information about the bus itself.
      * Can session_compare be replaced with g_strcmp0? It seems about
        the same.
      * I was looking at "CanActivateSessions" call to ConsoleKit. Do
        you think we should check this earlier and make the menu items
        sensitive or insensitive based on this?
      * It looks like you're always getting the information on all the
        users. I think that we really don't care if it's over our max,
        so can we just not bother getting all that data if we're in
        overflow?
      * The guest user needs to be handled as well. If there is a guest
        already logged in selecting the guest on the menu shouldn't
        start a new session, but should go to the guest that is already
        running.

  review needs-fixing

review: Needs Fixing

« Back to merge proposal