Created attachment 145901 [details] [review]
Refactored places.js so it could be added to the search panel, and added it to the search panel. From the GNOME Boston summit

Comment on attachment 145901 [details] [review]
Refactored places.js so it could be added to the search panel, and added it to the search panel. From the GNOME Boston summit

>From: JP St. Pierre <jstpierre@jstpierre-fedora.(none)>

Need to fix that. See http://live.gnome.org/Git/Help/AuthorEmail

>- * 
>+ *

Don't mix whitespace-fixing changes with code changes. If your editor insists on fixing the whitespace, then you should submit a separate commit with just whitespace fixes first, then another patch with the code changes on top of that.

(I'm not familiar with the existing places code so I'll let someone else review that.)

Created attachment 145969 [details] [review]
Refactored places.js so it could be added to the search panel, and added it to the search panel. From the GNOME Boston summit

arrgh... uploaded the wrong one again

Created attachment 145973 [details] [review]
Refactored places.js so it could be added to the search panel, and added it to the search panel. From the GNOME Boston summit

Review of attachment 145973 [details] [review]:

The commit message needs to be "subject" <blank line> "why and details".
http://mail.gnome.org/archives/gnome-shell-list/2009-February/msg00010.html has a general introduction to git with a link to http://lists.cairographics.org/archives/cairo/2008-September/015092.html that details the commit message style.

So in your case it can be:
-----------------------------------------------------------------------------------
Make places results available in search

Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from Places to PlacesManager.
-----------------------------------------------------------------------------------

The high level comment is that I think you should also factor out the code that maintains place info (name, iconFactory, onActivate) into PlaceInfo and use that in PlaceDisplay and PlaceDisplayItem. Otherwise it's weird how you are using some information components from PlaceDisplay that also has display components in PlaceDisplayItem. I also propose you rename some classes as following:
PlacesManager (good name!)
PlaceDisplay -> DashPlaceDisplayItem (similar to DashDocDisplayItem)
Places -> DashPlaceDisplay (similar to DashDocDisplay)
PlaceDisplayItem (good name!)
PlacesSearchDisplay -> PlaceDisplay
PlaceInfo (new)

Then you can also rename places.js to placeDisplay.js

This wasn't introduced by your changes, but a lot of the code has extra spaces between the function name and parenthesis. Could remove these since you are moving the code around or fix this up as part of the separate commit. (My typical approach is to fix spacing issues in the blocks of code I'm modifying for the patch, but to leave the parts not related to my changes to be a separate fix.)

::: js/ui/places.js
@@ +125,3 @@
         this._reloadBookmarks();
+
+        this._updateDesktopMenuVisibility();

Is there a reason you moved these two lines to the bottom of the function?

@@ +130,3 @@
+    },
+
+    _updateDevices : function() {

This was part of the original code, but the spacing is off in this function. Might as well fix it.

@@ +131,3 @@
+
+    _updateDevices : function() {
+        this._mounts = [];

Since this is a class variable it should be first defined in _init().

@@ +169,3 @@
+        }
+        this.emit('mounts-changed');
+        this.emit('places-changed');

Should be 'mounts-updated' and 'places-updated'. At least that's the signals you connect to. Either '-changed' or '-updated' is fine, but you need to pick one :).

@@ +174,3 @@
     _reloadBookmarks: function() {
 
+        this._bookmarks = [];

Since this is a class variable it should be first defined in _init().

@@ +201,3 @@
         }
 
+        for (i = 0; i < bookmarksOrder.length; i++) {

Needs 'let' otherwise there is a warning.

@@ +224,3 @@
 
+        this.emit('bookmarks-changed');
+        this.emit('places-changed');

Should be 'bookmarks-updated' and 'places-updated'.

@@ +229,3 @@
+    _updateDesktopMenuVisibility: function() {
+        let gconf = Shell.GConf.get_default();
+        this._desktopIsHome = gconf.get_boolean(DESKTOP_IS_HOME_KEY);

Since this is a class variable it should be first defined in _init(). You can set it to false there just before calling _updateDesktopMenuVisibility()

Also isDesktopHome would be a more conventional name for it.

@@ +376,3 @@
+
+    _updateDefaults: function() {
+        this._actionsBox.get_children().forEach(function (el) { el.destroy(); });

Can you not inline the function body? And also you should use 'child' instead of 'el'. So:
        this._actionsBox.get_children().forEach(function (child) {
            child.destroy(); 
        });

Same for the code in _updateMounts() and _updateBookmarks().

@@ +407,3 @@
+    __proto__: GenericDisplay.GenericDisplayItem.prototype,
+
+    _init : function(placeDisplay, id) {

You are not using 'id' anymore.

@@ +411,3 @@
+        this._placeDisplay = placeDisplay;
+
+        this._setItemInfo(placeDisplay.name, "");

Use '' instead of "". At some point, we started using "" only for strings that need to be translated.

@@ +414,3 @@
+    },
+
+    getId: function() {

Does not look like this is used. Would need call _id variable id to indicate that it is a public variable if it were useful.

@@ +422,3 @@
+    // Opens an application represented by this display item.
+    launch : function() {
+        this._placeDisplay._onActivate();

Need to rename _onActivate() into onActivate() since now used outside the class and needs to be part of the PlaceInfo anyway.

@@ +429,3 @@
+    // Returns an icon for the item.
+    _createIcon : function() {
+        return this._placeDisplay._iconFactory(GenericDisplay.ITEM_DISPLAY_ICON_SIZE);

_iconFactory should now be iconFactory and needs to be part of the PlaceInfo anyway.

@@ +434,3 @@
+    // Returns a preview icon for the item.
+    _createPreviewIcon : function() {
+        return this._placeDisplay._iconFactory(GenericDisplay.ITEM_DISPLAY_ICON_SIZE);

Should pass in GenericDisplay.PREVIEW_ICON_SIZE

@@ +446,3 @@
+}
+
+PlacesSearchDisplay.prototype = {

Needs to implement _compareItems() from GenericDisplay, otherwise get a "Not implemented" error. Just sorting the results alphabetically should be fine.

@@ +452,3 @@
+        GenericDisplay.GenericDisplay.prototype._init.call(this);
+        this._stale = true;
+        Main.placesManager.connect('places-updated', Lang.bind(this, function (e) {

I think the introduction of a shadow signal 'places-updated' is confusing; I think it's cleaner to just connect to all three signals here too and have a single function called _setCacheStale() that sets this._stale = false;

Alternatively, you can add a comment the first time you emit both signals in _updateDevices() saying that Places cares about which particular type of places got updated, while PlacesSearchDisplay only cares that places got updated, so we have both signals for the specific type and for places in general. You can then add "See comment in _updateDevices() as to why we emit two signals" to both _reloadBookmarks() and _updateDesktopMenuVisibility().

@@ +461,3 @@
+        if (!this._stale)
+            return true;
+        this._allItems = Main.placesManager.getAllPlaces();

this._allItems should be a map, not an array. So perhaps you need these ids after all. Since there is no good way to get unique ids for the various types of places, I think just creating a map with sequential numeric ids should be fine for now. I don't think we depend on ids being the same for the same item between cache refreshes anywhere.

@@ +466,3 @@
+    },
+
+    // Sets the list of the displayed items based on how recently they were last visited.

This comment doesn't work here. Should be
// Sets the list of the displayed items ordering them alphabetically.

@@ +473,3 @@
+            this._matchedItems[id] = 1;
+            this._matchedItemKeys.push(id);
+        }

Should call this._matchedItemKeys.sort(Lang.bind(this, this._compareItems)); here

@@ +478,3 @@
+    // Checks if the item info can be a match for the search string by checking
+    // the name of the document. Item info is expected to be GtkRecentInfo.
+    // Returns a boolean flag indicating if itemInfo is a match.

Wrong comment. Should be:
// Checks if the item info can be a match for the search string by checking
// the name of the place. Item info is expected to be PlaceInfo.
// Returns a boolean flag indicating if itemInfo is a match.

@@ +487,3 @@
+            return true;
+        // TODO: we can also check doc URIs, so that
+        // if you search for a directory name, we display recent files from it

TODO not applicable

Created attachment 146129 [details] [review]
Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.

Created attachment 146143 [details] [review]
Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.

Created attachment 146144 [details] [review]
Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.

Review of attachment 146144 [details] [review]:

Looks very good overall. All comments are pretty minor.

I think you missed the suggested subject line for the commit message in my previous comment. I thought it should be:
------------------------------------------------------------------------------
Make places results available in search

Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.
-------------------------------------------------------------------------------

You can now also add:

Introduce PlaceInfo class that contains information about the place and can be used by classes that display it in different ways. Rename classes so that their names are consistent with corresponding classes in appDisplay.js and docDisplay.js

::: js/ui/dash.js
@@ +14,3 @@
 const AppDisplay = imports.ui.appDisplay;
 const DocDisplay = imports.ui.docDisplay;
+const Places = imports.ui.placeDisplay;

It'd be more consistent to rename Places to PlaceDisplay here.

@@ +80,3 @@
  * if the provided index is incremented by an increment and the array
  * is wrapped in if necessary.
+ *

There seem to be a lot of whitespace fixes mixed in in this file. Probably better to do them as a separate commit.

::: js/ui/main.js
@@ +17,3 @@
 const Overview = imports.ui.overview;
 const Panel = imports.ui.panel;
+const Places = imports.ui.placeDisplay;

This should be PlaceDisplay.

::: js/ui/places.js
@@ +25,3 @@
 /**
+ * Represents a place object, which is most normally a bookmark entry,
+ * a mount/volume, or a "special" place like the Home Folder, Computer, and Network.

"special" doesn't need quotes here.

@@ +35,3 @@
 }
 
+PlaceInfo._nextId = 0;

I think it is problematic to be increasing this count indefinitely. We re-create the PlaceInfo for each place when a set of places it belongs to changes, so the id changes and keeping it in PlaceInfo doesn't give us any continuous tracking of the same place.
You should just be able to use the array index as an id when you populate _allItems in _refreshCache() in PlaceDisplay.

@@ +157,3 @@
+        let drives = this._volumeMonitor.get_connected_drives();
+        for (let i = 0; i < drives.length; i++) {
+                let volumes = drives[i].get_volumes();

Can you fix up the indentation in this function?

@@ +191,3 @@
+
+        this.emit('mounts-updated');
+        this.emit('places-updated');

Can you add a comment explaining why two signals are emitted here?
(e.i. that DashPlaceDisplay cares about which particular type of places got updated, while PlaceDisplay only cares that places got updated, so we have both signals for the specific type and for places in general). You can then add "See comment in _updateDevices() as to why we emit two signals" to
both _reloadBookmarks() and _updateDesktopMenuVisibility().

@@ +349,3 @@
+};
+
+Signals.addSignalMethods(PlaceDisplay.prototype);

This is not necessary here.

@@ +387,3 @@
+        Main.placesManager.connect('defaults-updated', Lang.bind(this, this._updateDefaults));
+        Main.placesManager.connect('mounts-updated', Lang.bind(this, this._updateBookmarks));
+        Main.placesManager.connect('bookmarks-updated', Lang.bind(this, this._updateMounts));

The two callback functions are mixed-up.

@@ +395,3 @@
+
+    _updateDefaults: function() {
+        let places = Main.placesManager.getDefaultPlaces();

Don't you need to destroy each child in the _actionsBox first too?

@@ +438,3 @@
+    },
+
+    getId: function() {

Not used.

@@ +502,3 @@
+            this._matchedItems[id] = 1;
+            this._matchedItemKeys.push(id);
+        }

Should call this._matchedItemKeys.sort(Lang.bind(this, this._compareItems)); OR have a comment why we are not ordering the items in any particular way here.

Created attachment 146666 [details] [review]
Make places results available in search

Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.

Created attachment 146836 [details] [review]
Make places results available in search

Places is one of the dash sections and it should be included in search results.

Factor out the code for getting and updating the information about places from
Places to PlacesManager.

Comment on attachment 146836 [details] [review]
Make places results available in search

Great job on the patch! I committed it with these minor changes.

1) Added this extra sentence to the commit message:

Introduce PlaceInfo class that contains information about the place and can be
used by classes that display it in different ways. Rename classes so that their
names are consistent with corresponding classes in appDisplay.js and
docDisplay.js

2) Updated the author name to "JP St. Pierre <jstpierre@mecheye.net>" from "magcius <jstpierre@mecheye.net>". We normally use full names in the commit messages. Hope that's ok with you. You should probably update your local git preferences.

3) Removed unrelated whitespace changes, removed an unused setup to handle signals, added the id field to PlaceInfo, and commented / made more obvious the usage of the array index as an id for PlaceInfo. Here is the diff with my changes:

diff --git a/js/ui/dash.js b/js/ui/dash.js
index 823a15d..da4f8e8 100644
--- a/js/ui/dash.js
+++ b/js/ui/dash.js
@@ -66,7 +66,7 @@ const PLACES = "places";
  * Returns the index in an array of a given length that is obtained
  * if the provided index is incremented by an increment and the array
  * is wrapped in if necessary.
- *
+ * 
  * index: prior index, expects 0 <= index < length
  * increment: the change in index, expects abs(increment) <= length
  * length: the length of the array
@@ -318,7 +318,7 @@ SearchEntry.prototype = {
             else
                 this.entry.text = '';
 
-            // Return true to stop the signal emission, so that this.actor doesn't get
+            // Return true to stop the signal emission, so that this.actor doesn't get 
             // the button-press-event and re-highlight itself.
             return true;
         }));
@@ -341,7 +341,7 @@ SearchEntry.prototype = {
     _resetTextState: function (searchEntryClicked) {
         let text = this.getText();
         this._iconBox.remove_all();
-        // We highlight the search box if the user starts typing in it
+        // We highlight the search box if the user starts typing in it 
         // or just clicks in it to indicate that the search is active.
         if (text != '' || searchEntryClicked) {
             if (!searchEntryClicked)
diff --git a/js/ui/placeDisplay.js b/js/ui/placeDisplay.js
index f996f14..acee53e 100644
--- a/js/ui/placeDisplay.js
+++ b/js/ui/placeDisplay.js
@@ -39,6 +39,7 @@ PlaceInfo.prototype = {
         this.name = name;
         this.iconFactory = iconFactory;
         this.launch = launch;
+        this.id = null;
     }
 }
 
@@ -353,8 +354,6 @@ DashPlaceDisplayItem.prototype = {
     }
 };
 
-Signals.addSignalMethods(PlaceDisplay.prototype);
-
 function DashPlaceDisplay() {
     this._init();
 }
@@ -493,8 +492,17 @@ PlaceDisplay.prototype = {
         this._allItems = {};
         let array = Main.placesManager.getAllPlaces();
         for (let i = 0; i < array.length; i ++) {
-            array[i].id = i;
-            this._allItems[i] = array[i];
+            // We are using an array id as placeInfo id because placeInfo doesn't have any
+            // other information piece that can be used as a unique id. There are different
+            // types of placeInfo, such as devices and directories that would result in differently
+            // structured ids. Also the home directory can show up in both the default places and in
+            // bookmarks which means its URI can't be used as a unique id. (This does mean it can
+            // appear twice in search results, though that doesn't happen at the moment because we
+            // name it "Home Folder" in default places and it's named with the user's system name
+            // if it appears as a bookmark.)
+            let placeInfo = array[i];
+            placeInfo.id = i;
+            this._allItems[i] = placeInfo;
         }
         this._stale = false;
         return false;

One more change: I first rebased the patch to latest in master by removing one line with the whitespace change in dash.js that did not apply (and did not belong :).