Created attachment 266155 [details] [review]
mainWindow: use place in search model

Use place instead of location in search model in order to keep
as much information about the search results as possible.

Created attachment 266156 [details] [review]
Add store for recent and favorite places

This patch adds the PlaceStore object that extends GtkListStore.
Recent places can be added to the store and be written to file using
the JSON format.

If adding a recent place exceeds the limit for recent places the
oldest recent place will be removed.

Created attachment 266157 [details] [review]
settings: add recent-places-limit

Add setting to limit the number of recent places to store.

Created attachment 266158 [details] [review]
mainWindow: add storing of recent places

Add recent places visited to the PlaceStore.

Created attachment 266159 [details] [review]
mainWindow: add search completion from PlaceStore

Use the places stored in PlaceStore for popup completion while
searching.

Created attachment 266160 [details] [review]
PlaceStore: add storing of favorites

Review of attachment 266155 [details] [review]:

ack

Review of attachment 266156 [details] [review]:

Looks good otherwise.

::: src/placeStore.js
@@ +66,3 @@
+
+    addRecent: function(place) {
+        if (!this._exists(place, PlaceType.RECENT)) {

I'd rather we:

if (this._exists(place, PlaceType.RECENT))
    return;

in here

@@ +75,3 @@
+                }), true);
+            }
+            this._addPlace(place, PlaceType.RECENT, +new Date());

what the '+' for?

@@ +82,3 @@
+
+    load: function() {
+        if (!GLib.file_test(this.filename, GLib.FileTest.EXISTS))

Shouldn't we create the file if it doesn't exist?

@@ +85,3 @@
+            return;
+
+        let jsonParser = new Json.Parser();

The best thing about JSON is that you can readily use it in Javascript so you don't need a parser here.

@@ +133,3 @@
+        jsonBuilder.begin_array();
+
+        this.foreach(function(model, path, iter) {

Same thing here

Review of attachment 266157 [details] [review]:

ack

Review of attachment 266158 [details] [review]:

::: src/mainWindow.js
@@ +92,3 @@
+    _initPlaces: function() {
+        let dir = GLib.get_user_data_dir();
+        let placeFile = dir + '/' + _PLACES_STORE_FILE;

I'd rather we put this in utils.js

@@ +93,3 @@
+        let dir = GLib.get_user_data_dir();
+        let placeFile = dir + '/' + _PLACES_STORE_FILE;
+        let recentLimit = Application.settings.get('recent-places-limit');

I think PlaceStore should itself load its settings and file.

@@ +105,3 @@
+        Mainloop.timeout_add(_PLACES_STORE_TIMEOUT, (function() {
+            // will write to file if dirty
+            this._placeStore.store();

I think this should be implicit with addRecent.

@@ +323,3 @@
         this._saveWindowGeometry();
 
+        // write any un saved recent and favorite places to file

un saved -> unsaved

Review of attachment 266156 [details] [review]:

After looking at the following patches, I would suggest:

* This class be agnostic to 'recent' and 'favorite' concepts. You'd want to rename addRecent to just 'add' or 'addPlace'.
* Add two subclasses for 'recent' and 'favorite that only make this generic class very specific. They can also the handling of settings and where to load the file from etc.

Review of attachment 266159 [details] [review]:

Looking good otherwise.

::: src/mainWindow.js
@@ +143,3 @@
+        }).bind(this));
+
+        this._searchEntry.set_completion(completion);

I'd rather we do as much of this in .ui file as possible.

Review of attachment 266159 [details] [review]:

Looking good otherwise.

::: src/mainWindow.js
@@ +143,3 @@
+        }).bind(this));
+
+        this._searchEntry.set_completion(completion);

I'd rather we do as much of this in .ui file as possible.

Review of attachment 266159 [details] [review]:

Looking good otherwise.

::: src/mainWindow.js
@@ +143,3 @@
+        }).bind(this));
+
+        this._searchEntry.set_completion(completion);

I'd rather we do as much of this in .ui file as possible.

(In reply to comment #11)
> Review of attachment 266156 [details] [review]:
> 
> After looking at the following patches, I would suggest:
> 
> * This class be agnostic to 'recent' and 'favorite' concepts. You'd want to
> rename addRecent to just 'add' or 'addPlace'.
> * Add two subclasses for 'recent' and 'favorite that only make this generic
> class very specific. They can also the handling of settings and where to load
> the file from etc.

Yikes, didn't think that we need both in the same store for the completion entry so NM this comment.

Review of attachment 266160 [details] [review]:

So we save recent and favorite in the same file?

Review of attachment 266156 [details] [review]:

::: src/placeStore.js
@@ +66,3 @@
+
+    addRecent: function(place) {
+        if (!this._exists(place, PlaceType.RECENT)) {

Sure, agreed.

@@ +75,3 @@
+                }), true);
+            }
+            this._addPlace(place, PlaceType.RECENT, +new Date());

Transforms the date to a number, calls the toNumber-thingie on it. I found it while googling how to get a Unix timestamp in javascript.
But maybe it's better to explicit. I do not know how canonical that expression is in JavaScript. Mattias, what say you?

@@ +82,3 @@
+
+    load: function() {
+        if (!GLib.file_test(this.filename, GLib.FileTest.EXISTS))

Not sure, it will be created by the store method. Just as long as the model gets initialized we are good.

@@ +85,3 @@
+            return;
+
+        let jsonParser = new Json.Parser();

Ok, will try that!

Review of attachment 266158 [details] [review]:

::: src/mainWindow.js
@@ +92,3 @@
+    _initPlaces: function() {
+        let dir = GLib.get_user_data_dir();
+        let placeFile = dir + '/' + _PLACES_STORE_FILE;

Sure.

@@ +93,3 @@
+        let dir = GLib.get_user_data_dir();
+        let placeFile = dir + '/' + _PLACES_STORE_FILE;
+        let recentLimit = Application.settings.get('recent-places-limit');

Ok!

@@ +105,3 @@
+        Mainloop.timeout_add(_PLACES_STORE_TIMEOUT, (function() {
+            // will write to file if dirty
+            this._placeStore.store();

Ok!

@@ +323,3 @@
         this._saveWindowGeometry();
 
+        // write any un saved recent and favorite places to file

Yep!

Review of attachment 266159 [details] [review]:

::: src/mainWindow.js
@@ +143,3 @@
+        }).bind(this));
+
+        this._searchEntry.set_completion(completion);

I'll see how much is possible to do from UI.

(In reply to comment #15)
> 
> So we save recent and favorite in the same file?

That was my plan, and just calling it maps-places. I don't really see the point atm to have two files. What do you think?

(In reply to comment #20)
> (In reply to comment #15)
> > 
> > So we save recent and favorite in the same file?
> 
> That was my plan, and just calling it maps-places. I don't really see the point
> atm to have two files. What do you think?

As long as they are separated from each other, its fine i guess.

Review of attachment 266156 [details] [review]:

::: src/placeStore.js
@@ +75,3 @@
+                }), true);
+            }
+            this._addPlace(place, PlaceType.RECENT, +new Date());

(new Date()).getTime() is a bit easier on the eyes. :)

@@ +85,3 @@
+            return;
+
+        let jsonParser = new Json.Parser();

The commands are JSON.parse and JSON.stringify btw.

Created attachment 266286 [details] [review]
Add store for recent and favorite places

This patch adds the PlaceStore object that extends GtkListStore.
Recent places can be added to the store and be written to file using
the JSON format.

If adding a recent place exceeds the limit for recent places the
oldest recent place will be removed.

Created attachment 266287 [details] [review]
mainWindow: add search completion from PlaceStore

Use the places stored in PlaceStore for popup completion while
searching.

Created attachment 266288 [details] [review]
PlaceStore: add storing of favorites

Created attachment 266289 [details] [review]
mainWindow: add storing of recent places

Add recent places visited to the PlaceStore.

Review of attachment 266286 [details] [review]:

Looks good otherwise.

::: src/placeStore.js
@@ +95,3 @@
+            let jsonArray = JSON.parse(buffer);
+            jsonArray.forEach((function(jsonObject) {
+                let name = jsonObject['name'];

I think this can be simply jsonObject.name . Same for accesses to props of this object.

@@ +131,3 @@
+
+            let jsonObject = {};
+            jsonObject['poi_type'] = place.place_type;

same here.

Review of attachment 266287 [details] [review]:

ack

::: src/mainWindow.js
@@ +113,3 @@
                                   this._searchPopup.hide.bind(this._searchPopup));
+
+        let completion = new Gtk.EntryCompletion({ model: this._placeStore,

I'm confident that you can do most of this from UI file using the new templates in gtk. However its fine for now. Could you please put a FIXME about it before pushing?

Review of attachment 266288 [details] [review]:

ack

Review of attachment 266289 [details] [review]:

::: src/mainWindow.js
@@ +314,3 @@
+        // write any unsaved recent and favorite places to file
+        try {
+            this._placeStore.store();

As i said before, I think this should be implicit in addRecent() call.

(In reply to comment #28)
> Review of attachment 266287 [details] [review]:
> 
> ack
> 
> ::: src/mainWindow.js
> @@ +113,3 @@
>                                   
> this._searchPopup.hide.bind(this._searchPopup));
> +
> +        let completion = new Gtk.EntryCompletion({ model: this._placeStore,
> 
> I'm confident that you can do most of this from UI file using the new templates
> in gtk. However its fine for now. Could you please put a FIXME about it before
> pushing?

Yeah, I think so too. I was reading about how to do it when Mattias IM'd me about wanting this before the release. Will add a fixme and open a bug about it later on.

Created attachment 266294 [details] [review]
mainWindow: add storing of recent places

Add recent places visited to the PlaceStore.

Review of attachment 266294 [details] [review]:

ack

Actually, we don't yet have any UI to save to favorites to I'll keep this open.

Created attachment 266348 [details] [review]
mainWindow: move init of completion to ui file

Review of attachment 266348 [details] [review]:

::: src/main-window.ui
@@ +23,3 @@
+      </object>
+      <attributes>
+        <attribute name="pixbuf">0</attribute>

So this is a bit of a magic number that points to PlaceStore.Columns.ICON. Do you think it's ok?

::: src/mainWindow.js
@@ +116,3 @@
 
+        this._searchCompletion.set_model(this._placeStore);
+        this._searchCompletion.set_text_column(PlaceStore.Columns.NAME);

This _could_ be done from ui file with a <property name="text_column">2</property>, as is done with the icon. What do you think?

Review of attachment 266348 [details] [review]:

::: src/main-window.ui
@@ +23,3 @@
+      </object>
+      <attributes>
+        <attribute name="pixbuf">0</attribute>

Sure, perfectly normal afaik. :) PlaceStore.Columns.ICON is 0 so I don't see it as any magic.

::: src/mainWindow.js
@@ +116,3 @@
 
+        this._searchCompletion.set_model(this._placeStore);
+        this._searchCompletion.set_text_column(PlaceStore.Columns.NAME);

Yes please.

Created attachment 266357 [details] [review]
mainWindow: move init of completion to ui file

Review of attachment 266357 [details] [review]:

Looks good otherwise

::: src/mainWindow.js
@@ +116,3 @@
 
+        this._searchCompletion.set_model(this._placeStore);
+        this._searchCompletion.connect('match-selected', (function(c, m, iter) {

you can connect signals from UI file as well, no?

(In reply to comment #39)
> Review of attachment 266357 [details] [review]:
> 
> Looks good otherwise
> 
> ::: src/mainWindow.js
> @@ +116,3 @@
> 
> +        this._searchCompletion.set_model(this._placeStore);
> +        this._searchCompletion.connect('match-selected', (function(c, m, iter)
> {
> 
> you can connect signals from UI file as well, no?

Yes, I think you can. Not sure how you specify a member function of mainWindow though. And I'm not sure we should. It is not really UI and we do not do it for any other signals.

You want me to try to do it?

(In reply to comment #40)
> (In reply to comment #39)
> > Review of attachment 266357 [details] [review] [details]:
> > 
> > Looks good otherwise
> > 
> > ::: src/mainWindow.js
> > @@ +116,3 @@
> > 
> > +        this._searchCompletion.set_model(this._placeStore);
> > +        this._searchCompletion.connect('match-selected', (function(c, m, iter)
> > {
> > 
> > you can connect signals from UI file as well, no?
> 
> Yes, I think you can. Not sure how you specify a member function of mainWindow
> though. And I'm not sure we should. It is not really UI and we do not do it for
> any other signals.
> 
> You want me to try to do it?

Nm, lets do that everwhere some day.

Review of attachment 266357 [details] [review]:

Pushed as 142b3c2fba19c61d94d8d61f59a3e646f2f9a416

Created attachment 266955 [details] [review]
mainWindow: add custom match func to completion

GtkEntryCompletion has changed. Now if you call set_text_column
or set the property text_column all manually added renderers will
be removed. Also when you add renderers manually the text renderer
added by set_text_column will be removed and text_column set to -1.

So in order to have custom renderers for the GtkEntryCompletion we
cannot set text_column and instead need to have our own match func.

Created attachment 266956 [details] [review]
mainWindow: add custom match func to completion

GtkEntryCompletion has changed. Now if you call set_text_column
or set the property text_column all manually added renderers will
be removed. Also when you add renderers manually the text renderer
added by set_text_column will be removed and text_column set to -1.

So in order to have custom renderers for the GtkEntryCompletion we
cannot set text_column and instead need to have our own match func.

Review of attachment 266956 [details] [review]:

looking good otherwise.

::: src/mainWindow.js
@@ +121,3 @@
         }).bind(this));
+
+        this._searchCompletion.set_match_func(function(c, key, iter) {

c => completion

@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

I think you want to use gtk_tree_model_get_path and gtk_tree_path_get_indices() here instead so you can compare integers rather than string and not have to normalize.

Not entirely sure though so just try it and if it doesn't work, we can use this code.

@@ +125,3 @@
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);
+
+            name = GLib.utf8_normalize (name, -1, GLib.NormalizeMode.ALL);

do we really need this?

Review of attachment 266956 [details] [review]:

::: src/mainWindow.js
@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

I do not understand want you mean by this? What integers should be compared?

@@ +125,3 @@
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);
+
+            name = GLib.utf8_normalize (name, -1, GLib.NormalizeMode.ALL);

Yeah, I think so.

The key is normalized from the GtkEntryCompletion code and we need to normalize the name as well.
For instance, without the 'ö' in Malmö, from the key, did not match the 'ö' in Malmö from the store.

And g_utf8_normalize said to me:
"[...] You should generally call g_utf8_normalize() before comparing two Unicode strings."

Review of attachment 266956 [details] [review]:

::: src/mainWindow.js
@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

gtk_tree_path_get_indices() == PlaceStore.Columns.NAME

@@ +125,3 @@
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);
+
+            name = GLib.utf8_normalize (name, -1, GLib.NormalizeMode.ALL);

ah ok

Review of attachment 266956 [details] [review]:

::: src/mainWindow.js
@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

I don't see how getting the path would help me avoid comparing the name in the model against the supplied key.

Created attachment 267037 [details] [review]
mainWindow: add custom match func to completion

GtkEntryCompletion has changed. Now if you call set_text_column
or set the property text_column all manually added renderers will
be removed. Also when you add renderers manually the text renderer
added by set_text_column will be removed and text_column set to -1.

So in order to have custom renderers for the GtkEntryCompletion we
cannot set text_column and instead need to have our own match func.

Review of attachment 266956 [details] [review]:

::: src/mainWindow.js
@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

I might be missing something here but as indicated in the previous comment, I imagine you'll be comparing column numbers then.

Review of attachment 266956 [details] [review]:

::: src/mainWindow.js
@@ +123,3 @@
+        this._searchCompletion.set_match_func(function(c, key, iter) {
+            let model = c.get_model();
+            let name = model.get_value(iter, PlaceStore.Columns.NAME);

Na, this function gets called foreach row in the liststore and the key in the argument is what we typed in the gtkentry. The job of the function is to return true if the key matches the row of the store.

So there are not column numbers to compare.

Review of attachment 267037 [details] [review]:

ack

Comment on attachment 267037 [details] [review]
mainWindow: add custom match func to completion

Attachment 267037 [details] pushed as 3029c7d - mainWindow: add custom match func to completion

Close this please. :)

(In reply to comment #54)
> Close this please. :)

Do we already have UI to mark favorites?

(In reply to comment #55)
> (In reply to comment #54)
> > Close this please. :)
> 
> Do we already have UI to mark favorites?

No,  sorry,  my mistake. :-)

I have created a patch to add a favorite place and view them as a list.
I haven't worked on the UI, but the basic functionality with minimalist UI is ready.

A favorite place can be added by click the heart shaped icon on the top. It will add the current user location to the favorites.

Besides it is a list icon which on click displays the list of user favorite locations. Clicking on the favorite will take user to the location, just like in search.

This is my first patch. So please give me as much suggestions as you can. :) 

I am pasting the patch here.

diff --git a/src/favorite-popup.ui b/src/favorite-popup.ui
new file mode 100644
index 0000000..1d67507
--- /dev/null
+++ b/src/favorite-popup.ui
@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <!-- interface-requires gtk+ 3.10 -->
+  <object class="GtkStack" id="stack">
+    <property name="visible">True</property>
+    <property name="can_focus">False</property>
+    <property name="transition-type">crossfade</property>
+    <child>
+      <object class="GtkScrolledWindow" id="scrolled-window">
+        <property name="visible">True</property>
+        <property name="can_focus">False</property>
+        <property name="hscrollbar_policy">never</property>
+        <property name="shadow_type">in</property>
+        <child>
+          <object class="GtkTreeView" id="treeview">
+            <property name="visible">True</property>
+            <property name="can_focus">False</property>
+            <property name="expand">True</property>
+            <property name="headers-visible">False</property>
+            <property name="hover-selection">True</property>
+            <child internal-child="selection">
+              <object class="GtkTreeSelection" id="treeview-selection"/>
+            </child>
+          </object>
+        </child>
+      </object>
+    </child>
+    <child>
+      <object class="GtkSpinner" id="spinner">
+        <property name="visible">True</property>
+        <property name="can_focus">False</property>
+        <property name="halign">center</property>
+        <property name="valign">center</property>
+        <property name="width_request">16</property>
+        <property name="height_request">16</property>
+      </object>
+    </child>
+  </object>
+</interface>
diff --git a/src/favoritePopup.js b/src/favoritePopup.js
new file mode 100644
index 0000000..fbcf17c
--- /dev/null
+++ b/src/favoritePopup.js
@@ -0,0 +1,143 @@
+/* -*- Mode: JS2; indent-tabs-mode: nil; js2-basic-offset: 4 -*- */
+/* vim: set et ts=4 sw=4: */
+/*
+ * GNOME Maps is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * GNOME Maps is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with GNOME Maps; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Author: Jonas Danielsson <jonas@threetimestwo.org>
+ */
+
+const Gtk = imports.gi.Gtk;
+
+const Lang = imports.lang;
+const Utils = imports.utils;
+
+const Columns = {
+    ICON: 0,
+    TEXT: 1
+};
+
+const FavoritePopup = new Lang.Class({
+    Name: 'FavoritePopup',
+    Extends: Gtk.Popover,
+
+    _init: function(relativeTo, numVisible) {
+        this._numVisible = numVisible;
+
+        let ui = Utils.getUIObject('favorite-popup', ['scrolled-window',
+                                                      'stack',
+                                                      'spinner',
+                                                      'treeview',]);
+
+        this._stack = ui.stack;
+        this._scrolledWindow = ui.scrolledWindow;
+        this._spinner = ui.spinner;
+        this._treeView = ui.treeview;
+
+        this._treeView.connect('button-press-event',
+                               this._onListButtonPress.bind(this));
+        this._initList();
+
+        this.height_request = this._cellHeight * this._numVisible;
+        this._scrolledWindow.set_min_content_height(this.height_request);
+
+        this.parent({ relative_to: relativeTo,
+                      width_request: 300,
+                      no_show_all: true,
+                      visible: true });
+
+        this.get_style_context().add_class('maps-popover');
+        this.add(this._stack);
+        this.hide();
+    },
+
+    _initList: function() {
+        let column = new Gtk.TreeViewColumn();
+
+        this._treeView.append_column(column);
+
+        let cell = new Gtk.CellRendererPixbuf({ xpad: 2 });
+        column.pack_start(cell, false);
+        column.add_attribute(cell, 'pixbuf', Columns.ICON);
+
+        cell = new Gtk.CellRendererText({ xpad: 8,
+                                          ypad: 8 });
+        column.pack_start(cell, true);
+        column.add_attribute(cell, 'markup', Columns.TEXT);
+
+        this._cellHeight = column.cell_get_size(null)[3];
+        this._cellHeight += cell.get_preferred_height(this._treeView)[0];
+    },
+
+    _onListButtonPress: function(widget, event) {
+        let path_valid, path;
+        let bool, coordX, coordY;
+
+        [bool, coordX, coordY] = event.get_coords();
+        [path_valid, path] = this._treeView.get_path_at_pos(coordX, coordY,
+                                                            null, null, null);
+        if (path_valid) {
+            let model = this.getModel();
+            let iter_valid, iter;
+
+            if (model === null)
+                return;
+
+            [iter_valid, iter] = model.get_iter(path);
+            if (!iter_valid)
+                return;
+
+            this.emit('selected', iter);
+        }
+    },
+
+    showSpinner: function() {
+        this._spinner.start();
+        this._stack.set_visible_child(this._spinner);
+
+        if (!this.get_visible())
+            this.show();
+    },
+
+    showResult: function() {
+        if (this._spinner.active)
+            this._spinner.stop();
+
+        this._stack.set_visible_child(this._scrolledWindow);
+
+        if (!this.get_visible())
+            this.show();
+    },
+
+    vfunc_show: function() {
+        this._treeView.columns_autosize();
+        this.parent();
+    },
+
+    vfunc_hide: function() {
+        if (this._spinner.active)
+            this._spinner.stop();
+
+        this.parent();
+    },
+
+    setModel: function(model) {
+        this._treeView.set_model(model);
+    },
+
+    getModel: function() {
+        return this._treeView.get_model();
+    },
+});
+Utils.addSignalMethods(FavoritePopup.prototype);
diff --git a/src/gnome-maps.data.gresource.xml b/src/gnome-maps.data.gresource.xml
index 6250d60..33ae8fc 100644
--- a/src/gnome-maps.data.gresource.xml
+++ b/src/gnome-maps.data.gresource.xml
@@ -5,6 +5,7 @@
     <file preprocess="xml-stripblanks">main-window.ui</file>
     <file preprocess="xml-stripblanks">zoom-control.ui</file>
     <file preprocess="xml-stripblanks">search-popup.ui</file>
+    <file preprocess="xml-stripblanks">favorite-popup.ui</file>
     <file preprocess="xml-stripblanks">context-menu.ui</file>
     <file alias="application.css">../data/gnome-maps.css</file>
     <file alias="zoom-in.png">../data/media/zoom-in.png</file>
diff --git a/src/gnome-maps.js.gresource.xml b/src/gnome-maps.js.gresource.xml
index cb9a57c..838e3ad 100644
--- a/src/gnome-maps.js.gresource.xml
+++ b/src/gnome-maps.js.gresource.xml
@@ -4,6 +4,7 @@
     <file>application.js</file>
     <file>config.js</file>
     <file>contextMenu.js</file>
+    <file>favoritePopup.js</file>
     <file>geoclue.js</file>
     <file>main.js</file>
     <file>mainWindow.js</file>
diff --git a/src/main-window.ui b/src/main-window.ui
index 5d6afc4..dd60c6b 100644
--- a/src/main-window.ui
+++ b/src/main-window.ui
@@ -77,6 +77,48 @@
           </packing>
         </child>
         <child>
+          <object class="GtkButton" id="mark-favorite-button">
+            <property name="visible">True</property>
+            <property name="can-focus">True</property>
+            <property name="valign">center</property>
+            <property name="action-name">win.mark-favorite</property>
+            <style>
+              <class name="image-button"/>
+            </style>
+            <child>
+              <object class="GtkImage" id="mark-favorite-button-image">
+                <property name="visible">True</property>
+                <property name="icon-size">1</property>
+                <property name="icon-name">emblem-favorite-symbolic</property>
+              </object>
+            </child>
+          </object>
+          <packing>
+            <property name="pack-type">end</property>
+          </packing>
+        </child>
+        <child>
+          <object class="GtkButton" id="list-favorite-button">
+            <property name="visible">True</property>
+            <property name="can-focus">True</property>
+            <property name="valign">center</property>
+            <property name="action-name">win.list-favorite</property>
+            <style>
+              <class name="image-button"/>
+            </style>
+            <child>
+              <object class="GtkImage" id="list-favorite-button-image">
+                <property name="visible">True</property>
+                <property name="icon-size">1</property>
+                <property name="icon-name">view-list-symbolic</property>
+              </object>
+            </child>
+          </object>
+          <packing>
+            <property name="pack-type">end</property>
+          </packing>
+        </child>
+        <child>
           <object class="GtkMenuButton" id="layer-menu-button">
             <property name="visible">True</property>
             <property name="can-focus">True</property>
diff --git a/src/mainWindow.js b/src/mainWindow.js
index 3f9fc96..bfcdc52 100644
--- a/src/mainWindow.js
+++ b/src/mainWindow.js
@@ -27,6 +27,7 @@ const GLib = imports.gi.GLib;
 const Gtk = imports.gi.Gtk;
 const GObject = imports.gi.GObject;
 const Champlain = imports.gi.Champlain;
+const Geocode = imports.gi.GeocodeGlib;
 
 const Lang = imports.lang;
 const Mainloop = imports.mainloop;
@@ -34,6 +35,7 @@ const Mainloop = imports.mainloop;
 const Application = imports.application;
 const MapView = imports.mapView;
 const SearchPopup = imports.searchPopup;
+const FavoritePopup = imports.favoritePopup;
 const ContextMenu = imports.contextMenu;
 const PlaceStore = imports.placeStore;
 const Utils = imports.utils;
@@ -47,6 +49,7 @@ const _WINDOW_MIN_WIDTH = 600;
 const _WINDOW_MIN_HEIGHT = 500;
 
 const _PLACE_ICON_SIZE = 20;
+const _NUM_FAVORITES = 10;
 
 const SearchResults = {
     COL_ICON:         0,
@@ -54,6 +57,12 @@ const SearchResults = {
     COL_PLACE:        2
 };
 
+const FavoriteList = {
+    COL_ICON:        0,
+    COL_DESCRIPTION: 1,
+    COL_PLACE:       2
+};
+
 const MainWindow = new Lang.Class({
     Name: 'MainWindow',
 
@@ -62,7 +71,10 @@ const MainWindow = new Lang.Class({
         let ui = Utils.getUIObject('main-window', [ 'app-window',
                                                     'window-content',
                                                     'search-entry',
-                                                    'search-completion']);
+                                                    'search-completion',
+                                                    'list-favorite-button']);
+
+        this._favoriteButton = ui.listFavoriteButton;
         this._searchEntry = ui.searchEntry;
         this._searchCompletion = ui.searchCompletion;
         this.window = ui.appWindow;
@@ -76,6 +88,7 @@ const MainWindow = new Lang.Class({
         this._contextMenu = new ContextMenu.ContextMenu(this.mapView);
 
         this._initPlaces();
+        this._initFavorites();
         this._initSearchWidgets();
         this._initActions();
         this._initSignals();
@@ -96,6 +109,97 @@ const MainWindow = new Lang.Class({
         }
     },
 
+    _initFavorites: function(){
+
+        Utils.initActions(this.window, [
+            {
+                properties: { name: 'mark-favorite' },
+                signalHandlers: { activate: this._onMarkFavoriteActivate }
+            }, {
+                properties: { name: 'list-favorite' },
+                signalHandlers: { activate: this._onListFavoriteActivate }
+            }
+        ], this);
+
+        this._favoritePopup = new FavoritePopup.FavoritePopup(this._favoriteButton, _NUM_FAVORITES);
+
+        let model = new Gtk.ListStore();
+        model.set_column_types([GdkPixbuf.Pixbuf,
+                                GObject.TYPE_STRING,
+                                GObject.TYPE_OBJECT]);
+
+        this._favoritePopup.setModel(model);
+
+        this._favoritePopup.connect('selected',
+                                  this._onListFavoriteSelected.bind(this));
+        this.mapView.view.connect('button-press-event',
+                                  this._favoritePopup.hide.bind(this._favoritePopup));
+
+    },
+
+    _onMarkFavoriteActivate: function(){
+        if (!this.mapView.geoclue.location)
+            return;
+
+        let place = Geocode.Place.new_with_location(this.mapView.geoclue.location.description,
+                                                    Geocode.PlaceType.UNKNOWN,
+                                                    this.mapView.geoclue.location);
+        this._placeStore.addFavorite(place);
+    },
+
+    _showFavoritesList: function(places) {
+        let model = this._favoritePopup.getModel();
+
+        if (places === null) {
+            this._favoritePopup.hide();
+            return;
+        }
+
+        places.forEach((function(place) {
+            let iter = model.append();
+            let location = place.get_location();
+            let icon = place.icon;
+
+            if (location == null)
+                return;
+
+            let description = GLib.markup_escape_text(location.description, -1);
+
+            model.set(iter,
+                      [FavoriteList.COL_DESCRIPTION,
+                       FavoriteList.COL_PLACE],
+                      [description,
+                       place]);
+
+            if (icon !== null) {
+                Utils.load_icon(icon, _PLACE_ICON_SIZE, function(pixbuf) {
+                    model.set(iter, [FavoriteList.COL_ICON], [pixbuf]);
+                });
+            }
+        }).bind(this));
+        this._favoritePopup.showResult();
+    },
+
+    _onListFavoriteActivate: function(){
+        let model = this._favoritePopup.getModel();
+
+        model.clear();
+        this._favoritePopup.showSpinner();
+
+        this._showFavoritesList(this._placeStore.getFavorites());
+
+    },
+
+    _onListFavoriteSelected: function(widget, iter) {
+        let model = this._favoritePopup.getModel();
+        let place = model.get_value(iter, FavoriteList.COL_PLACE);
+
+        this.mapView.showNGotoLocation(place);
+
+        this._placeStore.addRecent(place);
+        this._favoritePopup.hide();
+    },
+
     _initSearchWidgets: function() {
         this._searchPopup = new SearchPopup.SearchPopup(this._searchEntry, 10);
 
@@ -387,4 +491,5 @@ const MainWindow = new Lang.Class({
         aboutDialog.connect('response',
                             aboutDialog.destroy.bind(aboutDialog));
     }
+
 });
diff --git a/src/placeStore.js b/src/placeStore.js
index cb0a98f..42a7ba2 100644
--- a/src/placeStore.js
+++ b/src/placeStore.js
@@ -82,6 +82,19 @@ const PlaceStore = new Lang.Class({
         this._addPlace(place, PlaceType.FAVORITE, new Date().getTime());
     },
 
+    getFavorites: function(){
+    	let places = [];
+    	this.foreach(function(model, path, iter) {
+            let place    = model.get_value(iter, Columns.PLACE),
+                type     = model.get_value(iter, Columns.TYPE);
+
+            if(type == PlaceType.FAVORITE)
+            	places.push(place);
+        });
+
+        return places;
+    },
+
     addRecent: function(place) {
         if (this._exists(place, PlaceType.RECENT)) {
             this._updateAddTime(place);

(In reply to comment #57)
> I have created a patch to add a favorite place and view them as a list.
> I haven't worked on the UI, but the basic functionality with minimalist UI is
> ready.
> 
> A favorite place can be added by click the heart shaped icon on the top. It
> will add the current user location to the favorites.
> 
> Besides it is a list icon which on click displays the list of user favorite
> locations. Clicking on the favorite will take user to the location, just like
> in search.
> 
> This is my first patch. So please give me as much suggestions as you can. :) 
> 
> I am pasting the patch here.

Hi,

Thanks for the patch! Could you please add it as a attachment? It would make it easier for us to apply it to our local repositories. It would also make the review process better.

Created attachment 270885 [details] [review]
Patch for Bug # 722102

Review of attachment 270885 [details] [review]:

Thanks!

Another thing you could look into is using git format-patch for patch creation: https://openhatch.org/wiki/How_to_generate_patches_with_git_format-patch

And if you plan to contribute through bugzilla often maybe you would want to use git bz: http://wiki.koha-community.org/wiki/Git_bz_configuration

First of have you seen this mockup?: https://raw.github.com/gnome-design-team/gnome-mockups/master/maps/headerbar-and-popup-ideas.png
It has some input on what you are working on.

I think this patch would benefit from being split up in smaller patches. For instance one that adds the favorite popup, one that adds the functionality of getting favorites from placeStore, and one patch that ties everything together in the main window.

The way you implement marking of favorites seems odd to me. The location property on geoclue as far as I know is the last-known-location or the current location based on information from ip/gps/wifi or other source.
Wouldn't it make more sense to have the "mark favorite" functionality on a location marker on the map?


The favoritePopup is pretty much copy and pasted from the searchPopup, right? If you want to have the same functionality in both then why not make a super class? Something like a placeListPopover (or better name) that have an ui file and the basic functionality. And then maybe the searchPopup adds some stuff like the spinner to have it's functionality?

Also please look at bug #723895 for how I am hoping the logic of searchPopup will change. Maybe you could base your patch on that? Otherwise the logic of the MainWindow grows an awful lot.

::: src/placeStore.js
@@ +95,3 @@
+        return places;
+    },
+

How about instead of pulling everything out of the liststore and then adding it to a new later we use a TreeModelFilter? (https://developer.gnome.org/gtk3/stable/GtkTreeModelFilter.html).

So this becomes something like getFavoriteModel?

Thanks for the review Jonas.

I will modularize the patches and make a super class from which search and popup or any other popup menus can be derived.

Yup, mainWindow has to much code, will also modularize it.

For marking a favorite I have used the current user location, but yes we will want to move the favorite button to location bubble. See this mockup 

https://github.com/gnome-design-team/gnome-mockups/blob/master/maps/location-bubble.png.
Also can you suggest how to or which module should I know to add that favorite button to the location bubble.(something like toggle button).

For the FavoriteModel I was thinking of creating a new model just for favorites, currently favorite is just a PLACE_TYPE in the placeStore model, and to get the favorites I have to iterate through the list to check for the favorites.
Creating a new Model with add, delete, get, show features will be better.

Any further suggestions ?

(In reply to comment #62)
> Thanks for the review Jonas.
> 
> I will modularize the patches and make a super class from which search and
> popup or any other popup menus can be derived.

Sounds good if you feel that it makes sense and that the amount of code and/or look and feel mandates it.

> 
> Yup, mainWindow has to much code, will also modularize it.
> 

Please look at the bug that I linked to previously.

> For marking a favorite I have used the current user location, but yes we will
> want to move the favorite button to location bubble. See this mockup 
> 
> https://github.com/gnome-design-team/gnome-mockups/blob/master/maps/location-bubble.png.
> Also can you suggest how to or which module should I know to add that favorite
> button to the location bubble.(something like toggle button).

Great! I know Andreas and Mattias will get together to talk about design soon, so if you can talk to them in IRC while doing this that would probably help.

> 
> For the FavoriteModel I was thinking of creating a new model just for
> favorites, currently favorite is just a PLACE_TYPE in the placeStore model, and
> to get the favorites I have to iterate through the list to check for the
> favorites.
> Creating a new Model with add, delete, get, show features will be better.
>

The thing about placeStore is that favorite and recent need to be in the same model (in this case a ListStore) because we use the placeStore as a model fot the GtkEntryCompletion for the searchbar. But there is no need to create a model for just favorites if we add a function to placeStore.js like:

getModelForPlaceType: function(placeType) {
        let filter = new Gtk.TreeModelFilter({ child_model: this });

        filter.set_visible_func(function(model, iter) {
            let type = model.get_value(iter, Columns.TYPE);
            if (type && type === placeType)
                return true;
            return false;
        });

        return filter;
    },

Then we will get a model for the placeType specified.

> Any further suggestions ?

Not right now, we'll talk more when the next revision of your patches lands :)
Thanks again for contributing!

So, I created a super class for both the popups(search and favorites).

And have moved the favorite button to the place bubble, UI can be tweaked as required.

For the moment I was unable to work using bug #723895 as I couldn't apply that patch on my system due to some proxy error, I will integrate the code with that patch as soon as I solve this issue.

I will attach the patch so that you can review it and give suggestions.

I have used temporary UI buttons for marking favorites which can be replaced when Andreas or Mattias provides it.

Also,
To keep the marked favorites and adding of recent searches to the same PlaceStore instance I have used something like 
"let placeStore = Application.application._mainWindow._placeStore;"

which I think is a very bad way or it may be not because application is a global variable, but is there any workaround to it?

Created attachment 271059 [details] [review]
Added Support for addition, removal and displaying of favorites.

Review of attachment 271059 [details] [review]:

Thanks!

The patches in bug #723895 have been commited. They change quite a bit of how the searchPopup populates itself. The next version of your patches should be based upon that.
So you would need to rebase this work on current master.

Why do you only add the favorite adding functionality to userLocation? userLocation is a subclass of mapLocation and it makes much more sense for a mapLocation to have this functionality.
The userLocation is where we are in the world right now. We want to add other places as favorites, surely?

I have some comments below on other things as well.

::: data/icons/Makefile.am
@@ +19,3 @@
 	pin.svg					\
+	favorite-marked.svg		\
+	favorite-unmarked.svg	\

Could you try to align these?

::: src/mapView.js
@@ +178,3 @@
+        return place;
+    },
+

Why is this needed?

::: src/placeStore.js
@@ +95,3 @@
+        }
+        this._addPlace(place, PlaceType.RECENT, new Date().getTime());
+    },

Maybe this could be removePlace and take a PlaceType as argument along with the place.

@@ +100,3 @@
+        return this._exists(place, PlaceType.FAVORITE);
+    },
+

I think I would rather make _exists a public function.

@@ +113,3 @@
+        return places;
+    },
+

As I said before I do not want a function like this. I would much rather have the function I specified in the last round of reviews. That gives us a model with the specified PlaceType.

Review of attachment 271059 [details] [review]:

Also, I noted that there is some whitespace damage in the patch. You can see this if you go: git diff HEAD~1 --check

Created attachment 271099 [details]
favorite-unfilled icon

Created attachment 271101 [details]
favorite-filled icon

Maybe favorite-checked and favorite-unchecked are better names

I am uploading another patch with the changes you suggested.

I have moved the addFavorite functionality to MapLocation, and any location during the search can also be marked as favorite, it need not be current userLocation. UI may need a bit of work here.

All My changes are based on the patch from bug #723895, I have further modularised the searchPopup file and it now extends from placeListPopup. 

Sorry, I forgot about getModelForPlaceType, but I have now used that in the favorite list functions.

The getCurrentUserLocation method is needed as I want to add/remove this place to/from the favorites and in used in userLocation File.

Any further suggestion?

Created attachment 271162 [details] [review]
Merged with Bug#723895. Added Options for favorites: add, remove, list.

Created attachment 271163 [details] [review]
Merged with Bug#723895. Added Options for favorites: add, remove, list.

The getCurrentUserLocation function is not needed now.

I will upload another patch with the changes.

Created attachment 271164 [details] [review]
Merged with Bug#723895. Added Options for favorites: add, remove, list.

Review of attachment 271164 [details] [review]:

Thanks!

Regarding the commit log message, you could look a bit at https://wiki.gnome.org/Git/CommitMessages for how to write a nice one.

I also want this split up in smaller patches I think. 

* One to do the super class and the changes that brings to searchPopup.

* One to add the FavoriteList

* One that adds using of FavoriteList to mainWindow

* One that adds the logic to add/remove favorite from a mapLocation marker.

::: src/favoritePopup.js
@@ +17,3 @@
+ *
+ * Author: Jonas Danielsson <jonas@threetimestwo.org>
+ */

I am not the author of this.

@@ +31,3 @@
+const _PLACE_ICON_SIZE = 20;
+const _NUM_FAVORITES = 10;
+

If this is meant to be a public constant then it should not have an underscore prefix.

@@ +60,3 @@
+                });
+            }
+

Couldn't this class instead take the favorite model as an argument to its constructor? We would need to harmonize so that the PlaceListPopup columns matches the PlaceStore columns in regard to Icon, Name and Place but that should be doable, right? Then this function becomes unnecessary.

::: src/mainWindow.js
@@ +29,1 @@
 

This is not needed, right?

@@ +113,3 @@
+
+        this._favoritePopup.showSpinner();
+

I don't think we need to show a spinner here, there is no delay, right?

@@ +126,3 @@
+        this._placeStore.addRecent(place);
+        this._favoritePopup.hide();
+    },

This function is not exactly the same as onSearchPopupSelected. There need only be one I think. Maybe named onPopupSelected or similar and it could be  function(popup, place) { [...]. And then the hide call would be popup.hide();

::: src/mapLocation.js
@@ +60,3 @@
         this._mapView = mapView;
         this._view = mapView.view;
+        this.place = place;

Should be this._place if it's private.

@@ +127,3 @@
+        bubbleActor.set_x_expand(true);
+        bubbleActor.set_y_expand(true);
+        

Some whitespace damage here.

@@ +148,3 @@
+        locationActor.add_child(descriptionActor);
+
+        marker.add_actor(locationActor);

Do we need to create all these Actors on every show? Or can we do it in _init, at least can we put it in some kind of helper method? The code here is also very similar to the code in userLocation.js what exactly differs? Why can't userLocation just use the same code for show? Or just add somethings?

@@ +189,3 @@
+        else{
+            descriptionActor.add_child(favoriteUnMarkedActor);
+        }

This is duplicate code for marked and unmarked in here. We should be able to just do this code once, right? And take the icon name as a property something like a createFavoriteActor(checked) and depending on if it should be checked or not you create the actor from the appropriate icon.

@@ +197,3 @@
+            return;
+
+        let placeStore = Application.application._mainWindow._placeStore;

This is not a correct way of accessing the placeStore. Members that are named with a underscore prefix are private.

I would rather that the creation of the placeStore moves to application.js and is treated as a global in the same way as settings currently are.

@@ +208,3 @@
+        placeStore.removePlace(this.place,PlaceStore.PlaceType.FAVORITE);
+    },
+

And _onMark... and _onUnMark... methods should be able to be one method taking a boolean argument, right?

@@ +213,3 @@
+        let placeStore = new PlaceStore.PlaceStore();
+        placeStore.load();
+        return placeStore.exists(place,PlaceStore.PlaceType.FAVORITE);

And here you are creating a new placestore that should not be needed.

::: src/placeListPopup.js
@@ +17,3 @@
+ *
+ * Author: Jonas Danielsson <jonas@threetimestwo.org>
+ */

I don't feel like the author of this module.

@@ +129,3 @@
+            this.show();
+    },
+

showSpinner and showResult feels more at home in the searchPopup. The favorite list doest not really display "results" and I'm not sure we need a spinner at all for favorite as there is practically no delay.

::: src/placeStore.js
@@ +105,3 @@
+            this._removeIf((function(model, iter) {
+                let p = model.get_value(iter, Columns.PLACE);
+                return p.name === place.name;

I think we should also verify the type.

@@ +111,3 @@
+            return;
+        }
+        this._addPlace(place, PlaceType.RECENT, new Date().getTime());

Copy and paste error? Why would we want to add a new Recent place when removing a place?

::: src/userLocation.js
@@ +35,3 @@
+const Application = imports.application;
+
+const _FAVORITE_ICON_SIZE = 20;

These are not needed.

Review of attachment 271164 [details] [review]:

::: src/mainWindow.js
@@ +126,3 @@
+        this._placeStore.addRecent(place);
+        this._favoritePopup.hide();
+    },

"This function is now" not "This function is not" :)

Review of attachment 271164 [details] [review]:

I have made the changes to the current patch.

Waiting for your next review :)

::: data/icons/Makefile.am
@@ +19,3 @@
 	pin.svg					\
+	favorite-checked.svg	\
+	favorite-unchecked.svg	\

I don't know why it looks unaligned, it looks aligned in my editor. Maybe the issue is with the tab-space of my editor.

::: src/favoritePopup.js
@@ +17,3 @@
+ *
+ * Author: Jonas Danielsson <jonas@threetimestwo.org>
+ */

ok.

@@ +31,3 @@
+const _PLACE_ICON_SIZE = 20;
+const _NUM_FAVORITES = 10;
+

done. Made it Public so that mainApplication accesses it.

@@ +60,3 @@
+                });
+            }
+

Well, if I do that it would change the for the searchPopup also, because both of them derives from PlaceListPopup.

::: src/mainWindow.js
@@ +29,1 @@
 

Removed.

@@ +113,3 @@
+
+        this._favoritePopup.showSpinner();
+        this._favoritePopup = new FavoritePopup.FavoritePopup(this._favoriteButton, FavoritePopup._NUM_FAVORITES);

Removed.

@@ +126,3 @@
+        this._placeStore.addRecent(place);
+        this._favoritePopup.hide();
+

Yup, made a function, with popup as an argument.

::: src/mapLocation.js
@@ +60,3 @@
         this._mapView = mapView;
         this._view = mapView.view;
+        this.place = place;

done.

@@ +127,3 @@
+        bubbleActor.set_x_expand(true);
+        bubbleActor.set_y_expand(true);
+        let bubbleActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/bubble.svg");

done.

@@ +148,3 @@
+        locationActor.add_child(descriptionActor);
+
+        textActor.set_margin_right(6);

I think we should keep it this way because any UI modification which needs to be done can be done here, userLocation and mapLocation each have their own show function which can be tweaked with respect to them only.

About generating actors once I made a function which will initialize all these actors and keep them as a private variable.

However, I cant initialize the favoriteActors so I need to create an instance for them every time.

@@ +189,3 @@
+        else{
+            descriptionActor.add_child(favoriteUnMarkedActor);
+            descriptionActor.remove_child(favoriteUnMarkedActor);

Yes, the function is doing the same, I have moved it out of userLocation.show and made a method out of it. It is used in both show methods. It takes as argument to which this actor should be added, I can make it to take an icon name also.

@@ +198,3 @@
+
+        let placeStore = Application.application._mainWindow._placeStore;
+

done. Added placeStore functionality in application.js from where any other requiring it can access it maintaining the placeStore synchronized among all modules.

@@ +208,3 @@
+        placeStore.removePlace(this.place,PlaceStore.PlaceType.FAVORITE);
+    },
+            descriptionActor.remove_child(favoriteMarkedActor);

done.

@@ +213,3 @@
+        let placeStore = new PlaceStore.PlaceStore();
+        placeStore.load();
+        });

done.

::: src/placeListPopup.js
@@ +17,3 @@
+ *
+ * Author: Jonas Danielsson <jonas@threetimestwo.org>
+ */

But you are :)

@@ +129,3 @@
+            this.show();
+    },
+

yup, moved them to searchPopup.

::: src/placeStore.js
@@ +105,3 @@
+            this._removeIf((function(model, iter) {
+                let p = model.get_value(iter, Columns.PLACE);
+                return p.name === place.name;

done. Verified that type should be a key in PlaceType.

@@ +111,3 @@
+            return;
+        }
+                return p.name === place.name;

okay, I forgot to remove it from the typeTable.
        "delete this._typeTable[place.name];"

::: src/userLocation.js
@@ +35,3 @@
+const Application = imports.application;
+
+const _FAVORITE_ICON_SIZE = 20;

Removed.

I have changed a lot of code, So I am having trouble breaking it into smaller patches.
If you could guide me a bit about generating them. :)

I am attaching the next patch for the review.

Created attachment 271341 [details] [review]
Added Favorites functionality to maps.

Review of attachment 271341 [details] [review]:

Thanks for the patch!
There has been some commits to master so this needs to be rebased again to apply cleanly.

And also, I think that placeListPopup should be just placePopup. And when thinking more, I think it should be placePopover, because it is not really a popup anymore.
So maybe a patch that also renames all the Popup-classes to Popover could be considered as well.

I want somebody else to look at all the clutter stuff in mapLocation and userLocation, hopefully Mattias or Andreas can help me out there. And also give input on the design.
I guess we need an icon for the list favorite button as well?

There is some rfc work to see if we can replace the markers with popovers as well: https://bugzilla.gnome.org/show_bug.cgi?id=722871
Depending on how that goes we might want to rebase this work on that. But if you break up the patch in smaller patches with logical changes then some of them can go in before.

About breaking the patch up in to smaller patches. I imagine there are plenty of ways to do that. I am not the Git ninja that I would like to be. But maybe an approach like the following would work for you:

$ git checkout -b branch_name_for_the_split_up
$ git reset HEAD~1 <- this will reset all work and leave you with unstaged changes

Then you consider what you should be your first patch and add the new files to go into it with
$ git add [file]

And for the changes from existing files that should go into it you can go

$ git add -p to interactively decide what should be staged.

You should also read up on the git rebase command that can be used to smash commits together or edit them or reword the commit log. All this is tricky stuff and easy to get wrong, it will be good practice for you :)
Try to work on branches so that you don't accidentally lose your work.

Good luck!
And thanks again for contributing, we will get through this fine!

::: data/icons/Makefile.am
@@ +19,3 @@
 	pin.svg					\
+	favorite-checked.svg	\
+	favorite-unchecked.svg	\

Try viewing this in another editor, for instance vim.

::: src/application.js
@@ +94,3 @@
+    _initPlaceStore: function(){
+
+        application.placeStore = new PlaceStore.PlaceStore();

Look at how the settings global is done and try to emulate that. This should be placeStore = new PlaceStore.PlaceStore();

::: src/favoritePopup.js
@@ +33,3 @@
+    Extends: PlaceListPopup.PlaceListPopup,
+
+    _showFavoritesList: function(favoriteModel) {

I think we should have an init function that takes the model as an argument instead.

@@ +34,3 @@
+
+    _showFavoritesList: function(favoriteModel) {
+

Public methods should not have the underscore prefix.

@@ +58,3 @@
+            }
+
+        });

Same comment as last time, I think we should be able to use the model from getModelForPlaceType directly if make sure the column numbers are the same for the same types.

::: src/mainWindow.js
@@ +87,3 @@
+
+    _initFavorites: function(){
+

space between parenthesis and curly bracket.

@@ +95,3 @@
+        ], this);
+
+        this._favoritePopup = new FavoritePopup.FavoritePopup(this._favoriteButton, FavoritePopup.NUM_FAVORITES);

Maybe the favoritePopup can set the numberofvisible by it self, in its own _init function? And you can do the same with the searchPopup, none of them need to take the numVisible as an argument, but just pass it on to this.parent in their _init function.

@@ +104,3 @@
+
+    _onListFavoriteActivate: function(){
+

same here

::: src/mapLocation.js
@@ +214,3 @@
+        else
+            placeStore.removePlace(this._place,PlaceStore.PlaceType.FAVORITE);
+    },

Should be able to just go Application.placeStore.addFavorite / .removePlace

@@ +219,3 @@
+
+        let placeStore = Application.application.placeStore;
+        return placeStore.exists(place,PlaceStore.PlaceType.FAVORITE);

Same with this,

::: src/placeStore.js
@@ +121,3 @@
+                return p.name === place.name;
+            }), true);
+            delete this._typeTable[place.name];

This should be inside the function

Review of attachment 271341 [details] [review]:

::: data/icons/Makefile.am
@@ +19,3 @@
 	pin.svg					\
+	favorite-checked.svg	\
+	favorite-unchecked.svg	\

done. oh yeah vim worked.

::: src/application.js
@@ +94,3 @@
+    _initPlaceStore: function(){
+
+        application.placeStore = new PlaceStore.PlaceStore();

done. Made a global placeStore.

::: src/favoritePopup.js
@@ +33,3 @@
+    Extends: PlaceListPopup.PlaceListPopup,
+
+    _showFavoritesList: function(favoriteModel) {

it cannot take model as an argument because it gets updated. Also, for search results model are created from the places provided by geocode searching.

@@ +34,3 @@
+
+    _showFavoritesList: function(favoriteModel) {
+

okay, done.

@@ +58,3 @@
+            }
+
+        });

okay done, there was a difference in model parameter numbers, place and description were 1, 2 in placeStore whereas 2, 1 in ListPopup.
But this function is required to assign treeView this model and show the list.

::: src/mainWindow.js
@@ +87,3 @@
+
+    _initFavorites: function(){
+

done.

@@ +95,3 @@
+        ], this);
+
+        Utils.initActions(this.window, [

done, but since both can have same visibility I have defined a constant as the visible length list.

@@ +104,3 @@
+
+    _onListFavoriteActivate: function(){
+        ], this);

done.

::: src/mapLocation.js
@@ +214,3 @@
+        else
+            placeStore.removePlace(this._place,PlaceStore.PlaceType.FAVORITE);
+        favoriteUnMarkedActor.connect("button-press-event", function(stage, event) {

done.

@@ +219,3 @@
+
+        let placeStore = Application.application.placeStore;
+            self._onFavoriteClick(true);

done.

::: src/placeStore.js
@@ +121,3 @@
+                return p.name === place.name;
+            }), true);
+            }

inside which function ?? _removeIf ??

I have made changes w.r.t current master branch.
I will rename placeListPopup to placePopover once all these small issues get solved.

I am attaching another patch. Once finalised I will break it to smaller patches.

Meanwhile, I will be talking to Mattias or Andreas regarding the UI.

Created attachment 271371 [details] [review]
Added Location Favorites functionality

Review of attachment 271371 [details] [review]:

Thanks!

Some answers and some more comments :)

About the renaming, I think I want that before the favoritePopup is introduced. So just a patch to rename searchPopup to searchPopover and then one of your patch will do the super class placePopover and then the favoritePopover can be introduced.

::: src/application.js
@@ +94,3 @@
 
+    _initPlaceStore: function(){
+

Missing a space before the bracket.

::: src/favoritePopup.js
@@ +34,3 @@
+    showFavoritesList: function(favoriteModel) {
+
+        this._treeView.model = favoriteModel;

No, I still want there to be a constructor something like:

_init: function(relativeTo, model) {
     this.parent(relativeTo);

    this._treeView.model = model;
},

And the creation of the favoritePopup from mainWindow will look something like:

let model = Application.placeStore.getModelForPlaceType(PlaceStore.PlaceType.FAVORITE);
this._favoritePopup = new FavoritePopup.FavoritePopup(this._favoriteButton, model);


Then the show function does not need to take any arguments and the assignment is reduntant.
And yes the model will get updated, and that is ok :) The model you get from getModelForPlaceType is a filter on the placeStore model. The placeStore model is updated when a place is added and if that place is a favorite the model we have here will mark it as visible. So it's ok. It will get updated and we will show everything in it.

@@ +39,3 @@
+        if (!this.get_visible())
+            this.show();
+    },

And since we only ever want to show the list, this could be done in the _init function, right? And then the function does not need to exist. Since we can just call show on the favoritePopover.

::: src/placeListPopup.js
@@ +35,3 @@
+    DESCRIPTION:  2
+};
+

The changing of the ordering of the column is a change I'd like in a patch of its own, with its own justification. I think.

@@ +38,3 @@
+const PlaceListPopup = new Lang.Class({
+    Name: 'PlaceListPopup',
+    Extends: Gtk.Popover,

We might want to make this abstract as well.

Abstract: true,

@@ +42,3 @@
+    _init: function(relativeTo) {
+        this._numVisible = NUM_VISIBLE;
+

I was rather thinking that this base class could take numVisible as an argument to its constructor and then searchPopup and FavoritePopup could set the numVisible from its constructor, with this.parent(...
Then we can decide the numVisible for search and for favorite, they don't have to be the same.

::: src/placeStore.js
@@ +121,3 @@
+                return p.name === place.name;
+            }), true);
+            delete this._typeTable[place.name];

Inside the anonymous function, something like 

if (p.name === place.name) {
    this._typeTable[place.name] = null;
    return true;
}

Review of attachment 271371 [details] [review]:

::: src/application.js
@@ +94,3 @@
 
+    _initPlaceStore: function(){
+

done.

::: src/favoritePopup.js
@@ +34,3 @@
+    showFavoritesList: function(favoriteModel) {
+
+        this._treeView.model = favoriteModel;

done.

@@ +39,3 @@
+        if (!this.get_visible())
+            this.show();
+    },

yup, moved it to init.

::: src/placeListPopup.js
@@ +35,3 @@
+    DESCRIPTION:  2
+};
+

okay.

@@ +38,3 @@
+const PlaceListPopup = new Lang.Class({
+    Name: 'PlaceListPopup',
+    Extends: Gtk.Popover,

done, but you could explain in brief what it's for.

@@ +42,3 @@
+    _init: function(relativeTo) {
+        this._numVisible = NUM_VISIBLE;
+

done.

::: src/placeStore.js
@@ +121,3 @@
+                return p.name === place.name;
+            }), true);
+            }

done.

Okay, so renaming of popup's to popover's done.
Popovers now have their own constructors which will can the parent constructors with the necessary values.

Any further reviews before I start making smaller patches of each, and bang my head more around git. :)

Created attachment 271434 [details] [review]
Added Favorites functionality to maps, renamed popup classes to popover.

(In reply to comment #86)
> Okay, so renaming of popup's to popover's done.
> Popovers now have their own constructors which will can the parent constructors
> with the necessary values.
> 
> Any further reviews before I start making smaller patches of each, and bang my
> head more around git. :)

It will be easier to review once the patch is split up, might as well wait for it.

Splitting the patch up will be tricky, next time try to think how to split the changes up before you start, it will make things easier.

About the order of the patches, I would consider having the first patch be the renaming from searchPopup to serachPopover. Then maybe the making of searchPopover to be a subclass of placePopover. After that you can add the favorutePopover.

And we might want to wait with the marker patches to see where we go with the conversion to Popover bug. We can afford to wait. GNOME is in UI freeze atm.

That means that when your patches are good to go in, I will not commit them to master. Instead I will mark them as commit_after_freeze. And commit them when 3.12 has been released.

For your application you can point them here to see that they have been marked for inclusion, or if need be, have them get confirmation from me.

I have split the patches into smaller ones.
I will be adding them here.

Also, I have made patches which are totally independent of this bug like
"making global placeStore variable in application.js"
"Synchronization of model variables of searchPopup with placeStore " and others

Please let me know if these patches are to be associated with other bugs or I have to open a bug to upload them.

Any further suggestion on patches.

Created attachment 272339 [details] [review]
Made placeStore a globally accesible variable so that all modules using it can have synchronization

Created attachment 272340 [details] [review]
SearchPopup model was not in sync with PlaceStore model. PLACE was 2 in searchPopup while 1 in searchPopup. Synchronized them.

Created attachment 272341 [details] [review]
Added Favorite Checked and Unchecked Icons provided by Andreas Nilsson.

Created attachment 272342 [details] [review]
Refactored Code : Renamed Popup classes and variables to Popover Variables

Created attachment 272344 [details] [review]
Made PlaceStore.exists a Public Function.

Created attachment 272345 [details] [review]
Added removePlace and getModelForPlaceType to placeStore.

Created attachment 272346 [details] [review]
Added placePopover class which is the base class for searchPopover.

Created attachment 272347 [details] [review]
Added Favorite List Popover.

Created attachment 272348 [details] [review]
Added option to mark a Location as a Favorite, Refactored userLocation, Now bubble for place and userlocation are same.

Comment on attachment 272345 [details] [review]
Added removePlace and getModelForPlaceType to placeStore.

This is the patch number 6/9. There was an indentation issue and anonymous function was not always returning a value which is now fixed.

Comment on attachment 272345 [details] [review]
Added removePlace and getModelForPlaceType to placeStore.

This is the patch number 6/9. There was an indentation issue and anonymous function was not always returning a value which is now fixed.

Review of attachment 272348 [details] [review]:

I think we should wait with this code until bug #722871 is resolved. It feels silly to add UI for something that we will redo as a popover.

Review of attachment 272339 [details] [review]:

If we do not add the marking/unmarking of favorite to location in this bug then this is not needed atm.
We can come back to it, depending on what happens in bug #722871

About the commit message, read again the guidelines here: https://wiki.gnome.org/Git/CommitMessages

The description on the first line should be brief. And you should talk about what the patch does, not what it has done. So it should be something like "Make PlaceStore globally accessible" and if you feel more needs to be said that should go in the commit message body.

Review of attachment 272345 [details] [review]:

This should be split in to two patches each with its own justification.

::: src/placeStore.js
@@ +100,3 @@
     },
 
+    removePlace: function(place,placeType) {

Needs a space after the comma.

@@ +115,3 @@
+        if(!placeTypeCheck)
+            return;
+

I think all this should go away and instead we can have:

if (!this.exists(place, placeType)
    return;

And then we do not need the if statement below.

@@ +116,3 @@
+            return;
+
+        let self = this;

This seem unnecessary.

@@ +147,2 @@
     addRecent: function(place) {
+        if (this.exists(place, PlaceType.RECENT) || this.exists(place, PlaceType.FAVORITE)) {

Why was this added? I don't understand the purpose. This makes sure that if the place already exist as a recent then we update the add time. We should not update the addtime if the place already exists as a favorite.

Review of attachment 272341 [details] [review]:

This should be a part of the patch that adds the functionality of the buttons I think. But since that one should wait for bug #722871. We could just hold off with this for now.

Review of attachment 272344 [details] [review]:

Commit message should be something like: "placeStore: make exists method public"

Review of attachment 272340 [details] [review]:

Commit message first line way to long. Keep it brief and if necessary add more in body.

Review of attachment 272342 [details] [review]:

I want a better commit message for this that follow guidelines and explains a bit why this is needed.

Review of attachment 272346 [details] [review]:

"Added" should be "Add". Or maybe a more descriptive message would be "Make searchPopover subclass of placePopover".
And I would like some text about why we want this.

::: src/placePopover.js
@@ +116,3 @@
+        this.parent();
+    },
+

Blank line not needed.

::: src/searchPopover.js
@@ +31,2 @@
 const _PLACE_ICON_SIZE = 20;
+const NUM_VISIBLE = 10;

Private constants should have underscore prefix. Also maybe we could find a better name for it. Maybe _ITEMS_VISIBLE?

@@ +38,2 @@
+    _init: function(relativeTo) {
+        this.parent(relativeTo,NUM_VISIBLE);

space after comma.

Review of attachment 272347 [details] [review]:

About commit message: "Added" should be "add" and there is no need to capitalize each word.

::: src/favoritePopover.js
@@ +27,3 @@
+
+const _PLACE_ICON_SIZE = 20;
+const NUM_VISIBLE = 10;

Same comment about underscore prefix and name here as in searchPopover.

@@ +33,3 @@
+    Extends: PlacePopover.PlacePopover,
+
+    _init: function(relativeTo,model) {

Space after comma

::: src/main-window.ui
@@ +78,3 @@
         </child>
         <child>
+          <object class="GtkButton" id="list-favorite-button">

I think this should be GtkMenuButton, since it is possible to set the popover property of it.

::: src/mainWindow.js
@@ +88,3 @@
 
+    _initFavorites: function() {
+

Blank line not needed. And does it really init favorites? A better name is needed. Maybe _initFavoritePopover?

@@ +92,3 @@
+           {
+                properties: { name: 'list-favorite' },
+                signalHandlers: { activate: this._onListFavoriteActivate }

Is this needed, isn't it possible to set the popover property on the GtkMenuButton?

@@ +100,3 @@
+
+        let favoriteModel = this._placeStore.getModelForPlaceType(PlaceStore.PlaceType.FAVORITE);
+        this._favoritePopover = new FavoritePopover.FavoritePopover(this._favoriteButton,favoriteModel);

space after comma

@@ +110,3 @@
+        this.mapView.view.connect('button-press-event',
+                                  this._windowContent.grab_focus.bind(this._windowContent));
+

I think all this belong in _initFavoriteList, we do not need to set all this up each time the button is pressed?

@@ +119,3 @@
+        popover.hide();
+    },
+

In mainWindow the function that are calls _onSomething are grouped together and the function called _initSomething as well. Please move your functions around to follow that.

Thanks for the patches!

I think we need to wait a bit for bug #722871 is further a long before doing the actual adding/removal of favorites. Maybe you could move your placeStore-global and placeStore removal patches there?

Review of attachment 272339 [details] [review]:

::: src/application.js
@@ +103,3 @@
+        }
+    },
+    

Whitespace damage.

(In reply to comment #102)
> Review of attachment 272348 [details] [review]:
> 
> I think we should wait with this code until bug #722871 is resolved. It feels
> silly to add UI for something that we will redo as a popover.

Agreed. Will modify do the UI part when the ug #722871 is resolved.

Review of attachment 272339 [details] [review]:

This patch might be needed in general because I think PlaceStore acts as a DB for storing places and data across all modules should be synchronized i.e, all must have access to the same instance.

I am uploading another patch with the changes regarding whitespace damage and commit messages.

::: src/application.js
@@ +103,3 @@
+        }
+    },
+            placeStore.load();

done.

Created attachment 272530 [details] [review]
Make the PlaceSore globally accessible.

Created attachment 272533 [details] [review]
SearchPopup and PlaceStore model Synchronization

Review of attachment 272341 [details] [review]:

Agreed. Will do it with while hooking the service with UI controls when bug #722871 is resolved.

Created attachment 272535 [details] [review]
Rename of Popup classes and variables to Popover

Created attachment 272536 [details] [review]
PlaceStore : make exists method public.

Review of attachment 272345 [details] [review]:

Will provide separate patches.

::: src/placeStore.js
@@ +100,3 @@
     },
 
+    removePlace: function(place,placeType) {

done.

@@ +115,3 @@
+        if(!placeTypeCheck)
+            return;
+            if(placeType === PlaceType[key]){

Yes, was thinking the same. I don't know why I did this.

@@ +116,3 @@
+            return;
+
+            if(placeType === PlaceType[key]){

This is needed because 
    self._typeTable[place.name] = null;
is inside the _removeIf anonymous function.
    this._typeTable[place.name] = null;
won't work because the anonymous function is creating a closure of its own.
"this" will refer to anonymous function which will not have _typeTable as a property.

@@ +147,2 @@
     addRecent: function(place) {
+        if (this.exists(place, PlaceType.RECENT) || this.exists(place, PlaceType.FAVORITE)) {

I am removing it. But the thing I though was that if a person visits it favorite place, the favorite will come ahead in the list.

Created attachment 272537 [details] [review]
Added removePlace method to placeStore

Created attachment 272538 [details] [review]
Added getModelForPlaceType method to placeStore

Created attachment 272539 [details] [review]
Make searchPopover subclass of placePopover

Review of attachment 272346 [details] [review]:

::: src/placePopover.js
@@ +116,3 @@
+        this.parent();
+    },
+

done.

::: src/searchPopover.js
@@ +31,2 @@
 const _PLACE_ICON_SIZE = 20;
+const NUM_VISIBLE = 10;

done.

@@ +38,2 @@
+    _init: function(relativeTo) {
+        this.parent(relativeTo,NUM_VISIBLE);

done.

Review of attachment 272347 [details] [review]:

::: src/favoritePopover.js
@@ +27,3 @@
+
+const _PLACE_ICON_SIZE = 20;
+const NUM_VISIBLE = 10;

done.

@@ +33,3 @@
+    Extends: PlacePopover.PlacePopover,
+
+    _init: function(relativeTo,model) {

done.

::: src/main-window.ui
@@ +78,3 @@
         </child>
         <child>
+          <object class="GtkButton" id="list-favorite-button">

done.

::: src/mainWindow.js
@@ +88,3 @@
 
+    _initFavorites: function() {
+

Agreed.
done.

@@ +92,3 @@
+           {
+                properties: { name: 'list-favorite' },
+                signalHandlers: { activate: this._onListFavoriteActivate }

yes it is possible if we used GtkMenuButton.
done.

@@ +100,3 @@
+
+        let favoriteModel = this._placeStore.getModelForPlaceType(PlaceStore.PlaceType.FAVORITE);
+            }

done.

@@ +110,3 @@
+        this.mapView.view.connect('button-press-event',
+                                  this._windowContent.grab_focus.bind(this._windowContent));
+

done.

@@ +119,3 @@
+        popover.hide();
+    },
+        this._favoritePopover.connect('selected',

ohh, i didn't notice it earlier. Thanks.
done.

Created attachment 272541 [details] [review]
Add favorite list popover.

Review of attachment 272537 [details] [review]:

commit log could be briefer, maybe: "placeStore: Add removePlace method"

::: src/placeStore.js
@@ +114,3 @@
+                }
+                return false;
+            }), true);

if you write the anonymous function as 

this._removeIf((function(model, iter) {
[...]
}).bind(this), true);

Then you o not need the self variable.

@@ +118,3 @@
+        else{
+            return;
+        }

This else statement seems rather unnecessary.

Review of attachment 272538 [details] [review]:

Again, the commit message should talk about what the patch does, not what it has done. So "Added" should be "Add". And also since it only touches placeStore it might be an idea to have a "placeStore:" prefix.

But other than that it looks good, I will mark it as accepted_commit_after_freeze and you do not have to update it. I will fix up the commit log when committing it after 3.12.

Review of attachment 272530 [details] [review]:

I still feel this patch should be in bug #722871, please attach it there.

Also make sure that the commit log complies with the guidelines before you move it. The text of the body is way above the recommended number of characters.
See also comments below.

::: src/application.js
@@ +94,3 @@
 
+    _initPlaceStore: function() {
+

This empty line should not be here.

::: src/mainWindow.js
@@ +80,2 @@
     _initPlaces: function() {
+        this._placeStore = Application.placeStore;

Seems a bit silly to have a _initPlaces function just for doing this line.

Review of attachment 272533 [details] [review]:

Thanks.

I will mark this as accepted_commit_after_freeze, and a remainer to myself: Might need a better log message that says why we want this.

Review of attachment 272536 [details] [review]:

Thanks, I will mark this as accepted_commit_after_freeze.

Review of attachment 272539 [details] [review]:

Thanks,


I'm sorry for dragging this out. But I think I've changed my mind about the showSpinner function. The spinner and the treeView are on the superclass and the methods to control them should be there as well I think.
Myabe the base class can have methods showSpinner and showPlaces?

Also thinking more I think it should be called placesPopover and not placePopover.

I would love for Mattias or Zeeshan to weight in on this, so if you see them around please nudge them here.

Review of attachment 272541 [details] [review]:

Thanks!
See comments below.

Would love to get some UI eyes on this, maybe you could nudge Andreas?
The Menubutton should change at least, right?

::: src/main-window.ui
@@ +82,3 @@
+            <property name="can-focus">True</property>
+            <property name="valign">center</property>
+            <property name="action-name">win.list-favorite</property>

does this action do anything now?

::: src/mainWindow.js
@@ +100,3 @@
+        this.mapView.view.connect('button-press-event',
+                                  this._windowContent.grab_focus.bind(this._windowContent));
+

You only need the two selected signals, the menu popover will hide itself fine when focus is otherwise.

@@ +256,3 @@
+        if (!this._favoritePopover.get_visible())
+            this._favoritePopover.show();
+    },

This is not necessary. the function can be removed.

@@ +262,3 @@
+
+        this._placeStore.addRecent(place);
+        popover.hide();

I think this popover.hide can be removed.

Review of attachment 272535 [details] [review]:

I think the commit log will have to change, I do not really understand it as it is. And I would love to get Mattias or Zeeshans ok for this before accepting it.

Review of attachment 272530 [details] [review]:

I will attach it in bug #722871. Reduced the commit message, second line seemed rather unnecessary.

::: src/application.js
@@ +94,3 @@
 
+    _initPlaceStore: function() {
+

done.

::: src/mainWindow.js
@@ +80,2 @@
     _initPlaces: function() {
+        this._placeStore = Application.placeStore;

yup. moved it to the _init method.

Review of attachment 272537 [details] [review]:

Changed the commit message.

::: src/placeStore.js
@@ +106,3 @@
+
+        let self = this;
+        if (this.exists(place, placeType)) {

this check also seemed unnecessary. removed it.

@@ +114,3 @@
+                }
+                return false;
+        if (this.exists(place, placeType)) {

new trick for me, thanks.

@@ +118,3 @@
+        else{
+            return;
+                let p = model.get_value(iter, Columns.PLACE);

yes. removed.

Created attachment 272668 [details] [review]
placeStore Add removePlace method

Review of attachment 272541 [details] [review]:

Yup, I think I should keep a heart icon for right now. I will ask Andreas whether he has something in mind for this.

::: src/main-window.ui
@@ +82,3 @@
+            <property name="can-focus">True</property>
+            <property name="valign">center</property>
+            <property name="action-name">win.list-favorite</property>

no. forgot to remove it. sorry.

@@ +90,3 @@
+                <property name="visible">True</property>
+                <property name="icon-size">1</property>
+            <style>

used "emblem-favorite-symbolic" instead.

::: src/mainWindow.js
@@ +100,3 @@
+        this.mapView.view.connect('button-press-event',
+                                  this._windowContent.grab_focus.bind(this._windowContent));
+                                  this._onPopoverSelected.bind(this));

yup. removed them.

@@ +256,3 @@
+        if (!this._favoritePopover.get_visible())
+            this._favoritePopover.show();
+    },

yup.forgot to remove it. removed.

@@ +262,3 @@
+
+        this._placeStore.addRecent(place);
+

no, the popover doesn't hides itself on selecting a place from the list (Both search and favorite).

Created attachment 272671 [details] [review]
Add favorite list popover

Review of attachment 272671 [details] [review]:

Thanks!

I still want input from others on this before signing of on it. For instance does it needs to be as wide as it is right now?

::: src/mainWindow.js
@@ +90,3 @@
+        let favoriteModel = this._placeStore.getModelForPlaceType(PlaceStore.PlaceType.FAVORITE);
+        this._favoritePopover = new FavoritePopover.FavoritePopover(this._favoriteButton, favoriteModel);
+

To make the lines less long. Could this be

let model = ... and
let popover = ... ?

I do not see why this needs to be a member variable atm.

@@ +252,3 @@
+
+        this._placeStore.addRecent(place);
+        popover.hide();

In my setup I get the same behavior with the popover.hide() there or if it's gone.

Review of attachment 272668 [details] [review]:

Thanks!

The body of the commit message still has to long lines. But I can fix that up when committing.

Hi Mattias, Zeeshan 

Can I get an opinion from you guys on the 3 patches reviewed by Jonas.

Thanks

These patches doesn't apply anymore for me. Sorry for being late on this. Rishi could you rebase them maybe?

Mattias, I have rebased the patches according to master. You can apply them now.

Mattias : Also, the "actual marking of the favorites" patch is based on bug #722871, once it gets resolved than only it will make sense to actually implement it, but for testing purposes you can apply patch with attachment id #272348, its rejected by the way :P.

Jonas : I have also made the changes you asked for the favorite popover commit, and in my setup if I remove "popover.hide();" it stays there, I don't know why two machines are giving different behavior.

I will attaching the 3 patches now.

Created attachment 273412 [details] [review]
Rename of Popup classes and variables to Popover

Created attachment 273413 [details] [review]
Make searchPopover subclass of placePopover

Created attachment 273414 [details] [review]
Add favorite list popover

(In reply to comment #144)

> Mattias : Also, the "actual marking of the favorites" patch is based on bug
> #722871, once it gets resolved than only it will make sense to actually
> implement it, but for testing purposes you can apply patch with attachment id
> #272348, its rejected by the way :P.

Ohh, you need to apply favorite icons patch also to test it.

(In reply to comment #148)
> (In reply to comment #144)
> 
> > Mattias : Also, the "actual marking of the favorites" patch is based on bug
> > #722871, once it gets resolved than only it will make sense to actually
> > implement it, but for testing purposes you can apply patch with attachment id
> > #272348, its rejected by the way :P.
> 
> Ohh, you need to apply favorite icons patch also to test it.

Tried for a while to apply your patches but I'm still failing. Did you really rebase them onto master?
Except for the following two rejected patches:
 - "Added Favorite Checked and Unchecked Icons provided by Andreas Nilsson. "
 - "Added option to mark a Location as a Favorite, Refactored userLocation, Now bubble for place and userlocation are same."

...are there any others I need to pull in? Should I ignore some patches here?

If I try checking out a fresh master and just run "git bz apply 722102" (uncommenting the above two mentioned patches) I get this:

-----
Applying: Added getModelForPlaceType method to placeStore.
fatal: sha1 information is lacking or useless (src/placeStore.js).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
-----

Mattiasb : Yes I did, but I see after I rebased the patches (when there were 497 commits) some commits have been made to the repo (now 550 commits), I think thats why the patches fail.

I will rebase them again by tonight :).

Review of attachment 272348 [details] [review]:

While I'd leave the review of patch itself to Jonas and Mattias, I just wanted to remind you of the commit log guidelines: https://wiki.gnome.org/Git/CommitMessages . One convention not mentioned there but is generally also followed is use of present tense (e.g Added -> Add). Please try your best to follow these.

Thanks Zeeshan, I have been looking at that for my commit messages and Jonas has been guiding me through these messages.

Also, one more question, I have generated another batch of patches(9 patches rebased according to current master), so should I upload them here because some of the patches have been accepted. So will it cause any problem?

I have rebased the patches according to commit "ddba7aab9b1b988b53e8a37490c02059d0bb6196" (commit no. 500).

I will be attaching them now.

A couple of them will be only for testing purposes as their UI needs to be done after bug #722871.

Created attachment 273469 [details] [review]
SearchPopup and PlaceStore model Synchronization

Created attachment 273470 [details] [review]
Added Favorite Checked and Unchecked Icons

Only for testing purposes.

Created attachment 273471 [details] [review]
Rename of Popup classes and variables to Popover

Created attachment 273472 [details] [review]
PlaceStore : make exists method public.

Created attachment 273473 [details] [review]
placeStore Add removePlace method

Created attachment 273474 [details] [review]
Add getModelForPlaceType method to placeStore

Created attachment 273475 [details] [review]
Make searchPopover subclass of placePopover

Created attachment 273476 [details] [review]
Add favorite list popover

Created attachment 273477 [details] [review]
Add option to mark a location as favorite

(In reply to comment #162)
> Created an attachment (id=273477) [details] [review]
> Add option to mark a location as favorite

Only for testing purposes.

Still doesn't apply. Should the patches go in a specific order?

--------------

Bug 722102 - Add storing of recent/favorite places and complete against them in search

266155 (skipping, committed) - mainWindow: use place in search model
266157 (skipping, committed) - settings: add recent-places-limit
266286 (skipping, committed) - Add store for recent and favorite places
266287 (skipping, committed) - mainWindow: add search completion from PlaceStore
266288 (skipping, committed) - PlaceStore: add storing of favorites
266294 (skipping, committed) - mainWindow: add storing of recent places
266357 (skipping, committed) - mainWindow: move init of completion to ui file
267037 (skipping, committed) - mainWindow: add custom match func to completion
273469 - SearchPopup and PlaceStore model Synchronization
273470 - Added Favorite Checked and Unchecked Icons
273471 - Rename of Popup classes and variables to Popover
273472 - PlaceStore : make exists method public.
273473 - placeStore Add removePlace method
273474 - Add getModelForPlaceType method to placeStore
273475 - Make searchPopover subclass of placePopover
273476 - Add favorite list popover
273477 - Add option to mark a location as favorite

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: SearchPopup and PlaceStore model Synchronization.
Applying: Added Favorite Checked and Unchecked Icons provided by Andreas Nilsson.
Applying: Rename of Popup classes and variables to Popover.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	src/searchPopover.js
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Rename of Popup classes and variables to Popover.
The copy of the patch that failed is found in:
   /home/mattiasb/Code/gnome/gnome-maps/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Rename-of-Popup-classes-and-variables-to-Popover-XGp0p_.patch

----------------

Created attachment 275333 [details] [review]
SearchPopup and PlaceStore model Synchronization

SearchPopup model was not in sync with PlaceStore model.
Value of Column.PLACE was 2 in SearchPopup while 1 in PlacePopup.

Created attachment 275334 [details] [review]
placeStore: Make exists method public

The exists method is made public so others modules can use it.

Created attachment 275335 [details] [review]
placeStore: Add removePlace method

removePlace method removes a place form the placeStore List given the
place and the placeType.

Created attachment 275336 [details] [review]
Add getModelForPlaceType method to place store

Returns the Model specific to the given placeType.

Created attachment 275337 [details] [review]
Add new PlaceListPopover module

Add a generic module for listing place in a popover. The list will
display the description and the icon for a place on the map.

Created attachment 275338 [details] [review]
Make SearchPopup a subclass of PlaceListPopover

Created attachment 275340 [details] [review]
Add favorite list popover

The list will be populated with the marked favorites by the user.

(In reply to comment #164)
> Still doesn't apply. Should the patches go in a specific order?


Please try now Mattias.

I have rebased the patches. I have removed those that are not relevant right now, such as adding way to add favorites.

I also changed a bit of how this is done. This is not urgent at all, since we should wait until there is a way to add favorites before this is commited. So I am adding a depends on bug #722871 that is the bug for the new marker design.

Review for these patches is welcome tho, really all patches except the final one _could_ go in now.

One thing that is missing. Is a way to detect if there are not favorite places stored and make sure that we do not show an empty list to the users in that case.

Also I am not sure the favorite list should behave like the search popup and always have certain height instead of having the height of the number of items. So that might be something to look into.

I get this when I select a favourite:

----------
(gnome-maps:2662): Gjs-WARNING **: JS ERROR: Exception in callback for signal: selected: TypeError: popover is undefined
MainWindow<._onPopoverSelected@resource:///org/gnome/maps/mainWindow.js:252
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
_emit@resource:///org/gnome/gjs/modules/signals.js:124
PlaceListPopover<._onRowActivated@resource:///org/gnome/maps/placeListPopover.js:92
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
start@resource:///org/gnome/maps/main.js:28
@<main>:1
------------

Other than that it seems to work fine. I would love to see the multi-line cells like in the mockups though and maybe make the menu just a little bit smaller.

Created attachment 275400 [details] [review]
Add favorite list popover

The list will be populated with the marked favorites by the user.

I have rebased these patches according to the current Master.
It also depends on Damians patches on bug #722871.

Created attachment 283627 [details] [review]
Add PlaceListPopover module.

Created attachment 283628 [details] [review]
Make SearchPopup subclass of PlaceListPopover.

Created attachment 283629 [details] [review]
Add favorite list popover.

These patches can be applied as is.

SearchPopup and PlaceStore model Synchronization  
placeStore: Make exists method public 
placeStore: Add removePlace method 
Add getModelForPlaceType method to place store

These have been modified:
Add PlaceListPopover module.
Make SearchPopup subclass of PlaceListPopover. 
Add favorite list popover.

I can't seem to make previous ones obsolete :(

(In reply to comment #179)
>
> I can't seem to make previous ones obsolete :(

Have you tried from the 'Details' view accessible from the link next the patches below?

(In reply to comment #180)
> (In reply to comment #179)
> >
> > I can't seem to make previous ones obsolete :(
> 
> Have you tried from the 'Details' view accessible from the link next the
> patches below?

Ohh, no :).

(In reply to comment #181)
> (In reply to comment #180)
> > (In reply to comment #179)
> > >
> > > I can't seem to make previous ones obsolete :(
> > 
> > Have you tried from the 'Details' view accessible from the link next the
> > patches below?
> 
> Ohh, no :).
 
But I am not authorized :(.

(In reply to comment #182)
> (In reply to comment #181)
> > (In reply to comment #180)
> > > (In reply to comment #179)
> > > >
> > > > I can't seem to make previous ones obsolete :(
> > > 
> > > Have you tried from the 'Details' view accessible from the link next the
> > > patches below?
> > 
> > Ohh, no :).
> 
> But I am not authorized :(.

And you only found out at the end of your SoC. :( Anyway, better late than never. You can ask for getting authorized.

(In reply to comment #183)
> (In reply to comment #182)
> > (In reply to comment #181)
> > > (In reply to comment #180)
> > > > (In reply to comment #179)
> > > > >
> > > > > I can't seem to make previous ones obsolete :(
> > > > 
> > > > Have you tried from the 'Details' view accessible from the link next the
> > > > patches below?
> > > 
> > > Ohh, no :).
> > 
> > But I am not authorized :(.
> 
> And you only found out at the end of your SoC. :( Anyway, better late than
> never. You can ask for getting authorized.

I am able to obsolete patches that I have submitted, The patches were rebased and submitted by Jonas in some time ago thats why I might be unable to obsolete his patches.

Ok.

Lets try this again. Lets forget about messing around with searchPopup.
I think we should first do this bug:
https://bugzilla.gnome.org/show_bug.cgi?id=737841

Convert searchPopup to GtkListBox. And make sure that the ui file for a row is named general enought so that it can be used for the favourite-popup as well.

So the favourite popup also uses a GtkListBox.
And we maybe should wait until the bug: https://bugzilla.gnome.org/show_bug.cgi?id=737775

Is in, that adds buttons to mapBubble. So that we can add a favourite button there as well.

So to clarify, no making of a placePopover superclass. Leave searchPopup be and only re-use a future place-box-row.ui or what it will be called for both the favourite popover and the searchPopup.

Created attachment 291213 [details] [review]
PlaceStore: Fix exists method

Created attachment 291214 [details] [review]
PlaceStore: Add getModelForPlaceType method

Created attachment 291215 [details] [review]
PlaceStore: Add removePlace method

Created attachment 291216 [details] [review]
Split out PlaceListRow from SearchPopup

Created attachment 291217 [details] [review]
Add FavoritesPopover

Created attachment 291218 [details] [review]
MapBubble: Add favorite button

Review of attachment 291214 [details] [review]:

Looks good except for nit. Feel free to commit after fixing that one.

::: src/placeStore.js
@@ +164,3 @@
+                return true;
+            else
+                return false;

return (type === placeType);

Review of attachment 291214 [details] [review]:

Looks good except for nit. Feel free to commit after fixing that one.

::: src/placeStore.js
@@ +164,3 @@
+                return true;
+            else
+                return false;

return (type === placeType);

Review of attachment 291214 [details] [review]:

Looks good except for nit.

::: src/placeStore.js
@@ +164,3 @@
+                return true;
+            else
+                return false;

return (type === placeType);

Review of attachment 291215 [details] [review]:

ACK

Review of attachment 291216 [details] [review]:

Looks good otherwise.

::: src/gnome-maps.data.gresource.xml
@@ -7,3 @@
     <file preprocess="xml-stripblanks">zoom-control.ui</file>
     <file preprocess="xml-stripblanks">search-popup.ui</file>
-    <file preprocess="xml-stripblanks">search-popup-row.ui</file>

search-popup-row.ui seems ti still be left after this patch (or am I being fooled by Splinter?).

::: src/placeListRow.js
@@ +23,3 @@
+const Lang = imports.lang;
+
+const PlaceFormatter = imports.placeFormatter;

Newline after this.

::: src/searchPopup.js
@@ +63,3 @@
         // NOTE: the magic number 3 makes the scrolled window height be exactly
         //       aligned with 6 rows.
+        let rowHeight = PlaceListRow.ROW_HEIGHT;

No need for this intermediate.

@@ +94,3 @@
     },
 
+    updateResult: function(places, string) {

Why searchString → string ?

Review of attachment 291217 [details] [review]:

Looks good except for smaller nits.

::: src/favoritesPopover.js
@@ +49,3 @@
+        this._rows = 0;
+
+        let placeStore = Application.placeStore;

No need for this intermediate.

@@ +63,3 @@
+                row.set_header(header);
+            else
+                row.set_header(null);

> let header = before ? new Gtk.Separator() : null;
> row.set_header(header);

or

> if (!before) {
>   row.set_header(null);
>   return;
> }
>
> let header = new Gtk.Separator();
> row.set_header(header);

@@ +111,3 @@
+            this._revealer.reveal_child = true;
+        else
+            this._revealer.reveal_child = false;

this._revealer.reveal_child = this._rows > _N_VISIBLE;

(this._rowsrows → this._rows)

Review of attachment 291218 [details] [review]:

Except for the opacity thing: ACK-eti-ACK

::: data/gnome-maps.css
@@ +10,3 @@
+    opacity: 1.0;
+}
+

I think we can skip this and just rely on the toggled state.

Thanks! The one that adds to mainwindow is missing. And I think we need an initial state. Ok to commit after fixups, yathink?

Created attachment 291255 [details] [review]
PlaceStore: Add getModelForPlaceType method

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291256 [details] [review]
PlaceStore: Add removePlace method

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291257 [details] [review]
Split out PlaceListRow from SearchPopup

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291258 [details] [review]
Add FavoritesPopover

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291259 [details] [review]
MapBubble: Add favorite button

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291260 [details] [review]
MainWindow: Add favorites toggle

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291261 [details] [review]
PlaceStore: Add addPlace method

https://bugzilla.gnome.org/show_bug.cgi?id=722102

https://bugzilla.gnome.org/show_bug.cgi?id=728117

Created attachment 291262 [details] [review]
SearchResultBubble: Add favorite button

Created attachment 291265 [details] [review]
PlaceStore: Add getModelForPlaceType method

Created attachment 291266 [details] [review]
PlaceStore: Add removePlace method

Created attachment 291267 [details] [review]
Split out PlaceListRow from SearchPopup

Created attachment 291268 [details] [review]
Add FavoritesPopover

Created attachment 291269 [details] [review]
MapBubble: Add favorite button

Created attachment 291270 [details] [review]
MainWindow: Add favorites toggle

Created attachment 291271 [details] [review]
PlaceStore: Add addPlace method

Created attachment 291272 [details] [review]
SearchResultBubble: Add favorite button

Review of attachment 291265 [details] [review]:

ACK

Review of attachment 291266 [details] [review]:

ACK

Review of attachment 291267 [details] [review]:

ACK

Review of attachment 291268 [details] [review]:

Missed a little stuff last review. Fine to commit after these small changes.

::: src/favoritesPopover.js
@@ +76,3 @@
+            let text = this._entry.text.toLowerCase();
+            let title = row.title.toLowerCase();
+            let length = text.length;

Remove this, and then below...

@@ +78,3 @@
+            let length = text.length;
+            
+            return title.substring(0, length) === text.substring(0, length);

Do this: 
> return (text === title.substring(0, text.length));

str.substring(0, str.length) is id :P

@@ +110,3 @@
+            let visible = Math.min(this._rows, _N_VISIBLE);
+
+            this._scrolledWindow.min_content_height = visible * PlaceListRow.ROW_HEIGHT + 3;

Hm, copy that comment about pixel-perfectness above here as well ^

Review of attachment 291269 [details] [review]:

ACK

::: src/map-bubble.ui
@@ +87,3 @@
+                <child>
+                  <object class="GtkImage" id="bubble-favorite-button-image">
+                <property name="name">bubble-favorite-button-image</property>

Indentation.

Review of attachment 291269 [details] [review]:

ACK

::: src/map-bubble.ui
@@ +87,3 @@
+                <child>
+                  <object class="GtkImage" id="bubble-favorite-button-image">
+                <property name="name">bubble-favorite-button-image</property>

Indentation.

Review of attachment 291270 [details] [review]:

ACK

Review of attachment 291271 [details] [review]:

I'm a bit unsure about this patch to be honest. 
It /looks/ like it adds code for no real benefit. 

Perhaps I'm missing something.

Review of attachment 291271 [details] [review]:

...so please elaborate. :)

Review of attachment 291272 [details] [review]:

ACK except for small comment.

::: src/searchResultBubble.js
@@ +31,3 @@
 const Place = imports.place;
 const PlaceFormatter = imports.placeFormatter;
+const PlaceStore = imports.placeStore;

Unused?

(In reply to comment #200)
> Thanks! The one that adds to mainwindow is missing. 

A missing patch?

> And I think we need an initial state. 

I think it would look better if the favourite menubutton was just insensitive when there are no favourites underneath it.

> Ok to commit after fixups, yathink?

Yeah commit away!

Review of attachment 291271 [details] [review]:

Ok, so elaborate.

So we have addFavorite and addRecent, and later we may have addContact.
But we only need one removePlace that takes a PlaceType.
So I felt that instead of adding a new method for each type of place we could have an addPlace that takes a PlaceType.

It felt odd in the favorites code to have addFavorite in the then clause and then removePlace in the else clause. Lack of symmetri.
But I do not feel very strongly about it, I could do without this patch.

(In reply to comment #227)
> (In reply to comment #200)
> > Thanks! The one that adds to mainwindow is missing. 
> 
> A missing patch?
> 
> > And I think we need an initial state. 
> 
> I think it would look better if the favourite menubutton was just insensitive
> when there are no favourites underneath it.
> 

Yeah I did that initially. Need to restruture mainWindow a bit for it to work, since we bind the headerbar buttons to 'connected', but it can be done ofcourse.

Will also change the row-property on favorite popover to have a notify so we can connect to it and set sensitivity.

> > Ok to commit after fixups, yathink?
> 
> Yeah commit away!

Wee.

Review of attachment 291271 [details] [review]:

Makes sense!