What would the search results for "Mike" show, theoretically? And how would we indicate to the user what action would be taken clicking on whatever is shown?

I'm curious to how it would be fleshed out.

Don't know. I'd show the contact, maybe with their avatar/image if they have one, and offer the different options in a different app. Or it would popup a vCard view of the person, filled in with the details we know, and offer the different methods of contact from there.

*** Bug 646137 has been marked as a duplicate of this bug. ***

My current thinking is that it would be result provided (in the user's model) by the Contacts app. Clicking on the result would display the Contact in the Contacts app. An open question is whether we want to at some point provide direct actions on the result - ie. initiate a chat, call, email etc. We'd need some design work for that.

(In reply to comment #4)
> My current thinking is that it would be result provided (in the user's model)
> by the Contacts app. Clicking on the result would display the Contact in the
> Contacts app. An open question is whether we want to at some point provide
> direct actions on the result - ie. initiate a chat, call, email etc. We'd need
> some design work for that.

I certainly want an easy way to initiate chat, etc. directly from the results. Note that this bug basically describes Morten Mjelva's Gnome GSoC project: http://live.gnome.org/mortenmj_GnomeShell_People

We'll also be discussing this in a bit of detail at next week's IM, Contacts, Social hackfest: https://live.gnome.org/Hackfests/IMContactsSocial2011

Sure feel free to discuss but for now that's the plan :)

Created attachment 193505 [details] [review]
This includes libfolks in our build system.

Created attachment 193506 [details] [review]
Contact search, backend

I had difficulties interacting with libfolks using javascript, so a lot of the internals of the contact search is implemented in C.

Created attachment 193507 [details] [review]
Contact search, frontend

This is the frontend. The visuals might need some work, but I'd like to move ahead and get this reviewed while I work on that.

Review of attachment 193506 [details] [review]:

Mostly good.

::: configure.ac
@@ +69,3 @@
 GJS_MIN_VERSION=1.29.15
 MUTTER_MIN_VERSION=3.0.0
+FOLKS_MIN_VERSION=0.5.2

This should be squashed in with the previous patch.

::: src/Makefile.am
@@ +104,3 @@
 	shell-app-usage.h		\
 	shell-arrow.h			\
+	shell-contact-system.h	\

Please align these.

::: src/shell-contact-system.c
@@ +1,1 @@
+/* This implements a complete suite for caching and searching contacts in the

You need to add a standard GPL header here.

@@ +34,3 @@
+  folks_individual_aggregator_prepare_finish (aggregator, res, &error);
+  if (error != NULL) {
+      /* Oh snap we're fucked */

Be nice. Also, this code path is dead, so just pass NULL instead of &error.

@@ +136,3 @@
+{
+    GSList *term_iter;
+    guint weight = 0;

Watch your indentation.

@@ +158,3 @@
+
+      /* Match on one or more IM addresses */
+      while (gee_iterator_next (im_addrs_iter))

Wouldn't this get exhausted after one term?

@@ +163,3 @@
+
+          p = strstr (addr, term);
+          if (p == addr) {

Style.

@@ +191,3 @@
+  else if (first->weight < second->weight)
+    {
+        return -1;

Watch your indentation.

@@ +196,3 @@
+    {
+      // TODO: Sort by alias (with strcmp) if weight is equal
+      return 0;

If you do this, use g_utf8_collate, not strcmp.

@@ +245,3 @@
+ * Returns: (transfer none): All individuals
+ */
+GeeMap *

Are we sure Gee* works seamlessly with gobject-introspection?

@@ +334,3 @@
+      weight = do_match (self, individual, normalized_terms);
+
+      if (weight != 0) {

Style.

@@ +337,3 @@
+          key = gee_map_iterator_get_key (iter);
+
+          result = g_new (ContactSearchResult, 1);

Use g_slice_new (and g_slice_free above).

@@ +365,3 @@
+    GSList *terms)
+{
+  return shell_contact_system_initial_search (self, terms);

You're going to have to finish this.

Review of attachment 193507 [details] [review]:

::: js/Makefile.am
@@ +21,3 @@
 	ui/boxpointer.js	\
 	ui/calendar.js		\
+	ui/contactDisplay.js \

Align me.

::: js/ui/contactDisplay.js
@@ +2,3 @@
+
+const Clutter = imports.gi.Clutter;
+const Folks = imports.gi.Folks

;

@@ +15,3 @@
+const PopupMenu = imports.ui.popupMenu;
+const Search = imports.ui.search;
+const SearchDisplay = imports.ui.searchDisplay;

Lots of unused imports here.

@@ +47,3 @@
+
+        let icon = new St.Icon({ gicon: this.individual.avatar,
+                                 icon_name: 'avatar-default',

Use either a gicon or an icon_name, not both.

@@ +55,3 @@
+                            y_align: St.Align.MIDDLE });
+
+        let details = new St.BoxLayout({ style: 'padding: 7px;',

Please use a CSS class name and modify the CSS file instead of using inline styles.

@@ +62,3 @@
+                               y_align: St.Align.MIDDLE });
+
+        let aliasText = this.individual['alias'] || "Unknown";

This needs to be translatable: _("Unknown")

@@ +81,3 @@
+
+        switch(presence)
+          {

Style.

@@ +84,3 @@
+          case Folks.PresenceType.AVAILABLE:
+            text = _("Available");
+            iconName = "user-available-symbolic";

Use single quotes for untranslated text.

@@ +141,3 @@
+ * serves as the source from which to retrieve contacts in shell.
+ */
+function ContactManager() {

This is quite a useless thing to have. Most of the methods don't do anything. The other two just unnecessarily wrap the ContactSystem.

@@ +190,3 @@
+        let themeNode = this.actor.get_theme_node();
+        this._spacing = themeNode.get_length('spacing');
+        this._item_size = themeNode.get_length('-contact-grid-item-size');

Instead of defining a separate CSS style name, what about using CSS classes?

    .contact-grid {
        -shell-grid-item-size: 12345px;
    }

@@ +213,3 @@
+        this.actor.connect('notify::width', Lang.bind(this, function() {
+            this._width = this.actor.width;
+            Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() {

A comment explaining why you do this would be nice.

@@ +242,3 @@
+                 'createIcon': function(size) {
+                         return contact.createIcon(size);
+                 },

Do not use a comma at the end of object initializers.

::: src/shell-contact-system.c
@@ +248,3 @@
 shell_contact_system_get_all (ShellContactSystem *self)
 {
+  GeeMap *individuals;

This should be squashed with the previous patch.

Review of attachment 193506 [details] [review]:

::: src/shell-contact-system.c
@@ +98,3 @@
+
+static char *
+normalize_and_casefold (const char *str)

We also have shell_util_normalize_and_casefold.

Created attachment 194852 [details] [review]
Forst Post-review patch

This should fix everything apart from the style related issues. I'll get on those later today.

Created attachment 194890 [details] [review]
Patch

Review of attachment 194890 [details] [review]:

Generally looks pretty close. The comments about the C code are details - stylistic and memory management, but there is some code organization that needs to be improved for contactDisplay.js

::: js/ui/contactDisplay.js
@@ +9,3 @@
+const Util = imports.misc.util;
+const IconGrid = imports.ui.iconGrid;
+const Main = imports.ui.main;

Unused import

@@ +38,3 @@
+                                 style_class: 'contact-icon' });
+        if (this.individual.avatar != null)
+            icon['gicon'] = this.individual.avatar;

unclear why you are using the [] syntax here instead of icon.gicon = this.individal.avatar as we would normally do

@@ +54,3 @@
+                               y_align: St.Align.MIDDLE });
+
+        let aliasText = this.individual['alias'] || _("Unknown");

again arbitrary use of [] syntax

@@ +99,3 @@
+
+        let box = new St.BoxLayout({ vertical: false,
+                                   style_class: 'contact-details-status' });

misalignment

@@ +150,3 @@
+            this._width = this.actor.width;
+            Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() {
+                this._tryAddResults();

Why are you duplicating all the code from GridSearchResults._init() (including this hack) here? - you need to extend GridSearchResults so that it's modifiable in the way you need - perhaps by passing extra arguments to _init, perhaps by adding virtual functions that you can override, rather than duplicating a big hunk of internals that will break if GridSearchResults is changed in any way. (That would probably be done as a separate patch, not squashed into this patch)

@@ +173,3 @@
+        let contact = new Contact(id);
+        if (!contact)
+            return null;

What's the reasoning for handling contact == NULL here? How would that happen? It doesn't look like the calling site in GridSearchResuls handles a null return.

@@ +201,3 @@
+
+    activateResult: function(id, params) {
+        let contact = new Contact(id);

This is sketchy - creating a bunch of actors, just to call a launch method that doesn't get used. If you need to do the same thing in both places, you could just create a module-level function - _launchContact(id) or something.

::: src/shell-contact-system.c
@@ +27,3 @@
+prepare_individual_aggregator_cb (GObject *obj,
+    GAsyncResult *res,
+    gpointer user_data)

You have all your function prototypes declarations and definitions. The type names and variable names need to be lined up. See, e.g., shell-gtk-embed.[ch] for apropriate examples.

@@ +60,3 @@
+  self->priv->aggregator = folks_individual_aggregator_new ();
+  folks_individual_aggregator_prepare (self->priv->aggregator,
+      prepare_individual_aggregator_cb, NULL);

Also, if you line break function arguments, indent them to match the opening ( of the function arguments.

@@ +89,3 @@
+
+  g_type_class_add_private (object_class, sizeof
+      (ShellContactSystemPrivate));

You also cannot liine break between a function name and '('. It looks like you might have run your code through some sort of automated tool that enforced a 80-column line limit. We don't follow any strict line length limit, though it's good to avoid lines that are very, very long or are unbalanced with the surrounding code. Automated tools generally don't do a good job at breaking lines in meaningful ways.

@@ +134,3 @@
+
+  char *casefolded_alias = normalize_and_casefold (
+      folks_alias_details_get_alias (FOLKS_ALIAS_DETAILS (individual)));

Leaked

@@ +136,3 @@
+      folks_alias_details_get_alias (FOLKS_ALIAS_DETAILS (individual)));
+
+  GeeMultiMap *im_addr_map = folks_im_details_get_im_addresses(FOLKS_IM_DETAILS (individual));

missing space

@@ +137,3 @@
+
+  GeeMultiMap *im_addr_map = folks_im_details_get_im_addresses(FOLKS_IM_DETAILS (individual));
+  GeeCollection *im_addrs = gee_multi_map_get_values (im_addr_map);

Leaked

@@ +153,3 @@
+
+      /* Match on one or more IM addresses */
+      im_addrs_iter = gee_iterable_iterator ((GeeIterable*) im_addrs);

Leaked.

@@ +167,3 @@
+          else if (p != NULL)
+            {
+            weight += IM_SUBSTRING_MATCH_WEIGHT;

Indentation problem

@@ +168,3 @@
+            {
+            weight += IM_SUBSTRING_MATCH_WEIGHT;
+            break;

This break is a bit odd - because it means that whether we get PREFIX_MATCH_WEIGHT or SUBSTRING_MATCH_WEIGHT can depend on the ordering of the addresses. Not a big deal in practice, but the code would make more sense I think if you had gboolean have_im_prefix_match and have_im_substring_match and set those in the iteration and then add a weight at the end.

@@ +185,3 @@
+  if (first->weight > second->weight)
+    {
+      return 1;

Would be more readable without the excess unneeded {}

@@ +198,3 @@
+
+static void
+free_results (gpointer data,

free_result, this only frees one of them

@@ +205,3 @@
+
+static GSList *
+sort_and_prepare_results (GSList *results)

Even for an internal funciton, it's worth marking that it does something unusual with the argument, I'd put a one-line comment:

+/* modifies and frees @results */
 static GSList *
 sort_and_prepare_results (GSList *results)

@@ +258,3 @@
+
+  individuals = folks_individual_aggregator_get_individuals (self->priv->aggregator);
+  g_return_val_if_fail (GEE_IS_MAP (individuals), NULL);

g_return_val_if_fail() should almost always be used to check that the caller of the function is passing correct arguments. Here you are doing some sort of internal assertion, use g_assert() for this if you are concerned about having something in your internal logic wrong that would cause hard to debug problems. Or omit the check if you can't think of any plausible reason why it would fail. So, the only g_return_if_fail I'd have here is the:'g_return_val_if_fail (SHELL_IS_CONTACT_SYSTEM (self), NULL);'

@@ +288,3 @@
+  g_return_val_if_fail (gee_map_has_key (individuals, key), NULL);
+
+  value = gee_map_get (individuals, key);

leaked (you probably just want to make this function transfer full)

@@ +289,3 @@
+
+  value = gee_map_get (individuals, key);
+  individual = (FolksIndividual *) value;

Generally, you should write casts like this using the GObject wrapper:

 individual = FOLKS_INDIVIDUAL (value);

that will automatically add a runtime check that catches problems, avoiding the need to add checks like the next one

@@ +337,3 @@
+  while (gee_map_iterator_next (iter))
+    {
+      individual = gee_map_iterator_get_value (iter);

leaked

::: src/shell-contact-system.h
@@ +39,3 @@
+
+FolksIndividual *shell_contact_system_get_individual (ShellContactSystem *self,
+    gchar *id);

Indentation problems here similar to that described for the .c file

Created attachment 195109 [details] [review]
Add a grid parameter to GridSearchResults

To avoid code duplication, I'm adding a parameter to GridSearchResults that lets us override the default grid.

Created attachment 195110 [details] [review]
Patch

Created attachment 195113 [details] [review]
Add a grid parameter to GridSearchResults

Created attachment 195114 [details] [review]
Patch

Review of attachment 195113 [details] [review]:

Looks fine

Review of attachment 195114 [details] [review]:

Looking very close, few things sneaked through and still need attention

::: src/shell-contact-system.c
@@ +76,3 @@
+{
+  self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
+      SHELL_TYPE_CONTACT_SYSTEM, ShellContactSystemPrivate);

Still misindentation here according to our standards

@@ +91,3 @@
+
+static char *
+normalize_and_casefold (const char *str)

You should use shell_util_normalize_and_casefold() (which doesn't have the leak you've introduced here). See commit c5de239e25b12

@@ +132,3 @@
+  guint weight = 0;
+
+  const char *alias = normalize_and_casefold (folks_alias_details_get_alias (FOLKS_ALIAS_DETAILS (individual)));

You didn't fix this leak

@@ +165,3 @@
+          p = strstr (addr, term);
+          if (p == addr)
+              have_im_prefix = TRUE;

indented too far (and the next branch too)

@@ +265,3 @@
+
+  individuals = folks_individual_aggregator_get_individuals (self->priv->aggregator);
+  g_return_val_if_fail (GEE_IS_MAP (individuals), NULL);

See previous comment:

g_return_val_if_fail() should almost always be used to check that the caller of the function is passing correct arguments. Here you are doing some sort of internal assertion, use g_assert() for this if you are concerned about having something in your internal logic wrong that would cause hard to debug problems. Or omit the check if you can't think of any plausible reason why it would fail. So, the only g_return_if_fail I'd have here is the:'g_return_val_if_fail (SHELL_IS_CONTACT_SYSTEM (self), NULL);'

@@ +291,3 @@
+  individuals = folks_individual_aggregator_get_individuals (self->priv->aggregator);
+
+  value = gee_map_get (individuals, key);

See previous comment:

leaked (you probably just want to make this function transfer full)

::: src/shell-contact-system.h
@@ +39,3 @@
+
+FolksIndividual *shell_contact_system_get_individual (ShellContactSystem *self,
+                                                      gchar *id);

Would rather have the parameter names lined up here too, though we aren't completely consistent about it.

Created attachment 195119 [details] [review]
Patch

Apparantly I had lost a commit somewhere, so a few of your comments from before weren't in my revised patch:. Sorry for the extra hassle.

Review of attachment 195119 [details] [review]:

Looks great, I'll go ahead and push these patches!