Yup, would need mockup and also more info from geocode-glib.

(In reply to comment #1)
> Yup, would need mockup and also more info from geocode-glib.

Actually we don't need anything from geocode-glib here.

Created attachment 241560 [details]
search mockups

Here is a mockup of a simple search with suggestions and a more-results page.
The more results page would also be the landing area for the "more" in the gnome-shell search (if we decide to create a maps gnome-shell search provider that is).

Created attachment 250583 [details] [review]
Move search to mainWindow and add completion

Hi,

This patch add completion using gtkentrycompletion. I wanted to get some feedback on the approach. I've added an constant magic number limit of 4 to how many suggestions are shown in the popup view. This maybe should be a gsetting if we want to go forward with the approach.

The patch also makes geocode search the responsibility of mainWindow, not mapView.

I don't know if the gtkentrycompletion widget is possible to style in a way that will be satisfactory for Maps. Or if I should attempt a custom widget. Comments?

Mattias mentioned that autocompletion using the current service might be a no no. As a "workaround" the patch adds a timeout to perform the search every SEARCH_COMPLETION_TIMEOUT milliseconds, 1000 in this patch.

I didn't go ahead with the more results view but I wanted more input on how it should behave and look. If one searches in that view, should completion be active with popup, or should the page change? Should the page be just a listview?

Thanks.

Created attachment 250825 [details] [review]
Move search to mainWindow and add completion V2

Created attachment 251123 [details] [review]
Move search to mainWindow and add suggestions V3

Added a more option and a searchView that is not styled. I need suggestions on how to style it. It uses a GtkStack to switch between mapView and searchView.

Comments?

Created attachment 251343 [details] [review]
Move search to mainWindow and add suggestions V4

Make actions more reliable.

Very nice stuff. This makes search so much better!
Have just been playing around with it briefly, but will continue to test.

* After trying the more results for a bit, I you can kind of get stuck in that page if you are unlucky, so we need to figure out what is the best approach there. Maybe skip it and show 6 results in the dropdown + a ability to scroll for more? (like Firefox does)
* I'm curious about the numbers in the more results. What are those?
* Sometimes I get "6 more search results..." twice.

I think it would be good for the search to select the first item in the list by default, so hitting Return doesn't trigger the "Choose all the places at the same time" behavior.

Piling on here. Let me know if I should stop... :)
It's admittedly a bit harder, but I should write it down before I forget.

Searching for "Par" currently gives me "SAR, SAR" as a top result, instead of "Paris", the Capital of Love.

(In reply to comment #10)
> Piling on here. Let me know if I should stop... :)
> It's admittedly a bit harder, but I should write it down before I forget.
> 
> Searching for "Par" currently gives me "SAR, SAR" as a top result, instead of
> "Paris", the Capital of Love.

Don't stop :) It's good to have stuff written down.

I do not do any meddling with the search results, I just take what I get from the geocode search. Does searching for "Par" also give you Paris, or is it not in the list at all?

(In reply to comment #8)
> Very nice stuff. This makes search so much better!
> Have just been playing around with it briefly, but will continue to test.
> 
> * After trying the more results for a bit, I you can kind of get stuck in that
> page if you are unlucky, so we need to figure out what is the best approach
> there. Maybe skip it and show 6 results in the dropdown + a ability to scroll
> for more? (like Firefox does)

Might be possible, I only included the rudimentary search-view to get a feel for the ux. Will try to cook up something more firefoxy.

> * I'm curious about the numbers in the more results. What are those?

It's metadata from the geocode search, its the type of the place found, they are defined in the geodode library:

typedef enum {
        GEOCODE_PLACE_TYPE_UNKNOWN = 0,
        GEOCODE_PLACE_TYPE_BUILDING,
        GEOCODE_PLACE_TYPE_STREET,
        GEOCODE_PLACE_TYPE_TOWN,
        GEOCODE_PLACE_TYPE_STATE,
        GEOCODE_PLACE_TYPE_COUNTY,
        GEOCODE_PLACE_TYPE_LOCAL_ADMINISTRATIVE_AREA,
        GEOCODE_PLACE_TYPE_POSTAL_CODE,
        GEOCODE_PLACE_TYPE_COUNTRY,
        GEOCODE_PLACE_TYPE_ISLAND,
        GEOCODE_PLACE_TYPE_AIRPORT,
        GEOCODE_PLACE_TYPE_RAILWAY_STATION,
        GEOCODE_PLACE_TYPE_BUS_STOP,
        GEOCODE_PLACE_TYPE_MOTORWAY,
        GEOCODE_PLACE_TYPE_DRAINAGE,
        GEOCODE_PLACE_TYPE_LAND_FEATURE

[...]

Since I sometimes would get results that was hard to determine what it was, I thought maybe we could use the type to guide users. By transforming it to strings or images or something.

> * Sometimes I get "6 more search results..." twice.

I think I know the reason for this bug, will be fixed in later patch versions.

Thanks for feedback!

(In reply to comment #9)
> I think it would be good for the search to select the first item in the list by
> default, so hitting Return doesn't trigger the "Choose all the places at the
> same time" behavior.

I thought about that too, but didn't want to make a to intrusive change since this would mean that nothing would trigger the "show everything" anymore. And I think the behavior in this patch is sort of what Google Maps does.

But of course we do what we think is best. I'll try with your suggestion in next version.

Created attachment 251532 [details] [review]
Move search to mainWindow and add suggestions V5

So try this patch.

It does away with the searchView and the more, and just adds all suggestions to the entrycompletion.

The entrycompletion has built in checks for when it thinks scrollbars should be added. I provoced it and found out that on my monitor and setup it's at 25 entries sort of.

Since the search always give back 10 matches (default in geocode-glib it seems) the scrollbar will never be shown. 
The patch also does the goes to the first result of the search if you press enter.

Play around with it and see if you like the feel. It should work to just type something fast, like "Malmö", and you should go to the first match even tho the suggestions popup hasn't arrived yet.

Thanks

Created attachment 251533 [details] [review]
Move search to mainWindow and add suggestions V5-test

This one sets the answer_count to 30 so that you can see how the scrollbar looks with many matches.

Are we happy with the default answer_count of 10, btw?

Created attachment 251534 [details] [review]
Move search to mainWindow and add suggestions V5-test

Sorry, this one sets the answer_count to 30

(In reply to comment #11)
> (In reply to comment #10)
> > Piling on here. Let me know if I should stop... :)
> > It's admittedly a bit harder, but I should write it down before I forget.
> > 
> > Searching for "Par" currently gives me "SAR, SAR" as a top result, instead of
> > "Paris", the Capital of Love.
> 
> Don't stop :) It's good to have stuff written down.
> 
> I do not do any meddling with the search results, I just take what I get from
> the geocode search. Does searching for "Par" also give you Paris, or is it not
> in the list at all?

It is not in the list at all. It only matches full names. I think that is ok for now though. Matching Par with Paris can go into another bug.

(In reply to comment #11)

> I do not do any meddling with the search results, I just take what I get from
> the geocode search. Does searching for "Par" also give you Paris, or is it not
> in the list at all?

It is not in the list at all. It only matches full names. I think that is ok for now though. Matching Par with Paris can go into another bug.

(In reply to comment #16)
> Created an attachment (id=251534) [details] [review]
> Move search to mainWindow and add suggestions V5-test
> 
> Sorry, this one sets the answer_count to 30

This works great!
Some small UI things (that we can take care of as followup bugs), but in general this one is good to go I would say.

Unfortunately we can't do this until we have at least our own proxy of Nominatim since Nominatim's TOS specifically forbids doing this:

http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy#Unacceptable_Use

(In reply to comment #20)
> Unfortunately we can't do this until we have at least our own proxy of
> Nominatim since Nominatim's TOS specifically forbids doing this:
> 
> http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy#Unacceptable_Use

What's blocking for setting this up on gnome.org?
Do we need new hardware or so?

(In reply to comment #21)
> (In reply to comment #20)
> > Unfortunately we can't do this until we have at least our own proxy of
> > Nominatim since Nominatim's TOS specifically forbids doing this:
> > 
> > http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy#Unacceptable_Use
> 
> What's blocking for setting this up on gnome.org?

We need to file a bug on admin team with details about Nominatim setup. I was hoping that Mattias could do that. :P

> Do we need new hardware or so?

Thats something Andrea Veri could tell.

Created attachment 251631 [details]
Log from #nominatim on oftc

A log form #nominatim on oftc where me, Zeeshan and Jussi asked about nominatim usage.

(In reply to comment #22)
> We need to file a bug on admin team with details about Nominatim setup. I was
> hoping that Mattias could do that. :P

I'll do that then. I thought the hardware requirements were too great to even think about this. Or do you mean setting up a proxy? Either way we should contact the admin team and / or the foundation I guess.

> > Do we need new hardware or so?
> 
> Thats something Andrea Veri could tell.

Here's some info on this: http://wiki.openstreetmap.org/wiki/Nominatim/Installation#Fedora.2FCentOS

Also check the logs above (https://bugzilla.gnome.org/attachment.cgi?id=251631)

(In reply to comment #24)
> (In reply to comment #22)
> > We need to file a bug on admin team with details about Nominatim setup. I was
> > hoping that Mattias could do that. :P
> 
> I'll do that then.

Cool, thanks!

> I thought the hardware requirements were too great to even
> think about this. Or do you mean setting up a proxy?

A proxy would be a good start, yes but I don't think its us who should make that decision so in the bug we should mention both possibilities and that it'd be preferred to have our own server.

Created attachment 251641 [details]
IRC-log with Andrea on #sysadmin

The takeaway from this discussion is that the proxy-stuff is easy to do and can probably be deployed soon while hosting by ourselves is costly (and we don't have the resources right now).

Regarding the bug as such:
We can be pretty much certain that we won't have our own nominatim instance in time for 3.10 (if ever) so we must abide to their ToS which states we can't make autocomplete-stuff. 

Zeeshan: I'm not sure how a proxy server would help here either as a lot of the requests still would go through especially before we have filled our cache with data. 

Anyway. My suggestion for this is that we don't do autocomplete and instead performs only one search when the user either a) presses ENTER or b) has stopped typing for 2 seconds. The results should then be listed in the dropdown from the text entry just as the current patch (since we want the user to select the exact location he/she wants to go to).

(In reply to comment #28)
> Regarding the bug as such:
> We can be pretty much certain that we won't have our own nominatim instance in
> time for 3.10 (if ever) so we must abide to their ToS which states we can't
> make autocomplete-stuff. 
> 
> Zeeshan: I'm not sure how a proxy server would help here either as a lot of the
> requests still would go through especially before we have filled our cache with
> data. 

A proxy server doesn't imply that we can than launch excessive queries, we still gotta limit the number of queries as much as possible.

> Anyway. My suggestion for this is that we don't do autocomplete

I'd rather we we save the searches and only autocomplete with those.

> and instead
> performs only one search when the user either a) presses ENTER or b) has
> stopped typing for 2 seconds. The results should then be listed in the dropdown
> from the text entry just as the current patch (since we want the user to select
> the exact location he/she wants to go to).

Yeah, doing this in addition to 'autocompletion with saved searches' would already make up a very nice UI. Oh and we should show a spinner in the dropdown when we are querying (or do we already do that in the attached patches?).

(In reply to comment #29)
> (In reply to comment #28)
> > Regarding the bug as such:
> > We can be pretty much certain that we won't have our own nominatim instance in
> > time for 3.10 (if ever) so we must abide to their ToS which states we can't
> > make autocomplete-stuff. 
> > 
> > Zeeshan: I'm not sure how a proxy server would help here either as a lot of the
> > requests still would go through especially before we have filled our cache with
> > data. 
> 
> A proxy server doesn't imply that we can than launch excessive queries, we
> still gotta limit the number of queries as much as possible.

That was in reply to "Unfortunately we can't do this until we have at least our own proxy of
Nominatim [...]" :)
What I mean is that (as you say) a proxy doesn't help us in this regard. We can't do autocomplete.

> Yeah, doing this in addition to 'autocompletion with saved searches' would
> already make up a very nice UI. Oh and we should show a spinner in the dropdown
> when we are querying (or do we already do that in the attached patches?).

We need a concept of history and favourites before we can do this properly. Basically we should only autocomplete / search for saved local data. (As a sidenote, this is probably what a future shell search provider should provide).

However, for 3.10 I think we should just be satisfied with showing the results of only one search in the dropdown and not do any autocomplete. My guess is that that's doable in time for 3.10. Jonas what do you think? Zeeshan since you're on vacation I can do the reviews if you feel comfortable with that.

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > Regarding the bug as such:
> > > We can be pretty much certain that we won't have our own nominatim instance in
> > > time for 3.10 (if ever) so we must abide to their ToS which states we can't
> > > make autocomplete-stuff. 
> > > 
> > > Zeeshan: I'm not sure how a proxy server would help here either as a lot of the
> > > requests still would go through especially before we have filled our cache with
> > > data. 
> > 
> > A proxy server doesn't imply that we can than launch excessive queries, we
> > still gotta limit the number of queries as much as possible.
> 
> That was in reply to "Unfortunately we can't do this until we have at least our
> own proxy of
> Nominatim [...]" :)
> What I mean is that (as you say) a proxy doesn't help us in this regard. We
> can't do autocomplete.
>
> > Yeah, doing this in addition to 'autocompletion with saved searches' would
> > already make up a very nice UI. Oh and we should show a spinner in the dropdown
> > when we are querying (or do we already do that in the attached patches?).
>
> We need a concept of history and favourites before we can do this properly.

I don't think so, especially not the favorites. We basically just save all search terms used by user. Many mobile Maps apps do that AFAIK (at least the N9 one does).

> Basically we should only autocomplete / search for saved local data. (As a
> sidenote, this is probably what a future shell search provider should provide).

Haven't really thought about shell search provider yet so can't comment on this one.

> However, for 3.10 I think we should just be satisfied with showing the results
> of only one search in the dropdown and not do any autocomplete. My guess is
> that that's doable in time for 3.10.

Yeah. I'm fine with that.

> Jonas what do you think? Zeeshan since
> you're on vacation I can do the reviews if you feel comfortable with that.

I've been doing reviews for things that I think are 3.10 material. I'll be back on Friday and I intend to work some hours on Sunday too.

Created attachment 251708 [details] [review]
Move search to mainWindow and add suggestions V6

Hi,

So this patch only populates the popup upon enter. I think it feels allrignt.
It uses a GtkComboBox instead of a GtkEntryCompletion, since there is no completion going on anymore.

Thanks
Jonas

Created attachment 251709 [details] [review]
Move search to mainWindow and add suggestions V6 [type]

And this patch...

This adds a second column where it specified the type of the match.
For instance when I get three matches on a street nearby 'Spånehusvägen',
with this patch I can see that two of them are bus stop.

I'm not sure.

Check out both and see what you like.

Thanks

I like the one without [type] best.

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

Review of attachment 251708 [details] [review]:

Looks good enough for first implementation otherwise. Oh and you want to update the commit log.

::: src/main-window.ui
@@ +30,3 @@
         </style>
         <child type="title">
+          <object class="GtkComboBox" id="search-box">

Don't think we want a combobox here. For searches, we still want to use a search entry and search results/suggestions shown appear in an overlay popup as shown in mockup:

https://wiki.gnome.org/Design/Apps/Maps?action=AttachFile&do=get&target=Maps.png

The main issue I see the combobox' drop down, using that for 'execute search' seems very odd from UX POV.

Having said all that, I think we could go with this approach for getting this in before 3.10 UI freeze.

::: src/mapView.js
@@ -89,3 @@
     },
 
-    geocodeSearch: function(string) {

I really don't like moving this to mainWindow.js. Why is it needed?

(In reply to comment #36)
> Review of attachment 251708 [details] [review]:
> 
> Looks good enough for first implementation otherwise. Oh and you want to update
> the commit log.
> 
> ::: src/main-window.ui
> @@ +30,3 @@
>          </style>
>          <child type="title">
> +          <object class="GtkComboBox" id="search-box">
> 
> Don't think we want a combobox here. For searches, we still want to use a
> search entry and search results/suggestions shown appear in an overlay popup as
> shown in mockup:
> 
> https://wiki.gnome.org/Design/Apps/Maps?action=AttachFile&do=get&target=Maps.png
> 
> The main issue I see the combobox' drop down, using that for 'execute search'
> seems very odd from UX POV.
> 
> Having said all that, I think we could go with this approach for getting this
> in before 3.10 UI freeze.

Hehe, got scared before reading the last line. I agree.

> 
> ::: src/mapView.js
> @@ -89,3 @@
>      },
> 
> -    geocodeSearch: function(string) {
> 
> I really don't like moving this to mainWindow.js. Why is it needed?

Not strictly needed. Moved it back. Maybe refactor to search class later.

New patch on its way. Changes this, whitespace damage in ui and Suggestions to SearchResults in log and mainWindow.

Thanks!

Created attachment 251853 [details] [review]
Make search results popup in a GtkComboBox V7

Review of attachment 251853 [details] [review]:

::: src/mainWindow.js
@@ +79,3 @@
+        this._searchEntry = new Gtk.SearchEntry();
+        this._searchBox.add(this._searchEntry);
+        this._searchBox.set_entry_text_column(SearchResults.COL_DESCRIPTION);

Either:
  this._searchBox.entry_text_column = SearchResults.COL_DESCRIPTION;
or earlier
  this._searchEntry = new Gtk.SearchEntry({
      entry_text_column: SearchResults.COL_DESCRIPTION
  });

:)

@@ +224,3 @@
+            this.mapView.geocodeSearch(searchString,
+                                       this._updateSearchResults.bind(this));
+        }

Is this patch actually based on master? Seems like these removals references patches that are only in your repo?
I can't actually apply it which strengthens my suspicion. :)

::: src/mapView.js
@@ +106,2 @@
             }
+            if (places != null)

!== if you are specific about checking for null. If you are checking for "truthy" just do if(places)

(In reply to comment #39)
> Review of attachment 251853 [details] [review]:
> 
> ::: src/mainWindow.js
> @@ +79,3 @@
> +        this._searchEntry = new Gtk.SearchEntry();
> +        this._searchBox.add(this._searchEntry);
> +        this._searchBox.set_entry_text_column(SearchResults.COL_DESCRIPTION);
> 
> Either:
>   this._searchBox.entry_text_column = SearchResults.COL_DESCRIPTION;
> or earlier
>   this._searchEntry = new Gtk.SearchEntry({
>       entry_text_column: SearchResults.COL_DESCRIPTION
>   });
> 
> :)

Ok!

> 
> @@ +224,3 @@
> +            this.mapView.geocodeSearch(searchString,
> +                                       this._updateSearchResults.bind(this));
> +        }
> 
> Is this patch actually based on master? Seems like these removals references
> patches that are only in your repo?
> I can't actually apply it which strengthens my suspicion. :)
> 

Really? It applies cleanly on master for me. Was your repo up-to-date?
Somethings were pushed recently

> ::: src/mapView.js
> @@ +106,2 @@
>              }
> +            if (places != null)
> 
> !== if you are specific about checking for null. If you are checking for
> "truthy" just do if(places)

Ok!

Review of attachment 251853 [details] [review]:

::: src/mainWindow.js
@@ +224,3 @@
+            this.mapView.geocodeSearch(searchString,
+                                       this._updateSearchResults.bind(this));
+        }

Applies fine here!

Review of attachment 251853 [details] [review]:

Looks good for first iteration otherwise.

::: src/mainWindow.js
@@ +234,3 @@
+    },
+
+    _updateSearchResultsModel: function (places) {

I don't see the need to create a new model for each search. Just clear the model using gtk_list_store_clear() and add the new results. In which case, you'd want to change the name of this method.

::: src/mapView.js
@@ +110,3 @@
     },
 
+

Redundant change?

Created attachment 251972 [details] [review]
Make search results popup in a GtkComboBox V8

Updated after review.

(In reply to comment #40)
> (In reply to comment #39)
> > Review of attachment 251853 [details] [review] [details]:
> > 
> > ::: src/mainWindow.js
> > @@ +79,3 @@
> > +        this._searchEntry = new Gtk.SearchEntry();
> > +        this._searchBox.add(this._searchEntry);
> > +        this._searchBox.set_entry_text_column(SearchResults.COL_DESCRIPTION);
> > 
> > Either:
> >   this._searchBox.entry_text_column = SearchResults.COL_DESCRIPTION;
> > or earlier
> >   this._searchEntry = new Gtk.SearchEntry({
> >       entry_text_column: SearchResults.COL_DESCRIPTION
> >   });
> > 
> > :)

Btw, is this Maps/Gjs coding style? To avoid setters and go for properties directly? What are the benefits?

Review of attachment 251972 [details] [review]:

Apart from naming nitpicks, all good.

::: src/mainWindow.js
@@ +76,3 @@
     },
 
+    _initSearch: function() {

Nitpick: The name makes it sound like you are initializing a search, _initSearchWidgets perhaps?

@@ +233,3 @@
+    },
+
+    _updateSearchResultsModel: function (places) {

You forgot or missed the "In which case, you'd want to change the name of this method." part in my review comment. '_populateSearchResultModel' or simply '_showSearchResults' perhaps?

Also I think there is no need for '_updateSearchResults' to exist then.

(In reply to comment #44)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > Review of attachment 251853 [details] [review] [details] [details]:
> > > 
> > > ::: src/mainWindow.js
> > > @@ +79,3 @@
> > > +        this._searchEntry = new Gtk.SearchEntry();
> > > +        this._searchBox.add(this._searchEntry);
> > > +        this._searchBox.set_entry_text_column(SearchResults.COL_DESCRIPTION);
> > > 
> > > Either:
> > >   this._searchBox.entry_text_column = SearchResults.COL_DESCRIPTION;
> > > or earlier
> > >   this._searchEntry = new Gtk.SearchEntry({
> > >       entry_text_column: SearchResults.COL_DESCRIPTION
> > >   });
> > > 
> > > :)
> 
> Btw, is this Maps/Gjs coding style? To avoid setters and go for properties
> directly? What are the benefits?

The only benefit is that is more obvious that you are simply setting a property. In Vala, this is a good idea since vala itself will translate the property access to a call to appr. setting/getter method but I don't think gjs does any such smart things so I'm not sure its a great idea to do so in gjs code.

Created attachment 252012 [details] [review]
Make search results popup in a GtkComboBox V9

Something like this perhaps?

Thanks for review!

Pushed the patch with some minor changes to _populateSearchResultsModel().