GNOME Bugzilla – Bug 744032
GNOME Shell search provider
Last modified: 2018-03-23 15:06:12 UTC
I find it surprising that we don't have yet a search provider to quickly find a place from the shell. Apple Spotlight seems to allow it... If Clocks can display the time for a lot of cities, I guess we should be able to do the same with coordinate locations, which would display the place directly in Maps. With some types of searches (companies, monuments...), it might also be interesting to display some useful informations (a kind of Google's Knowledge Graph) such as photos, official website... that can be found on Wikidata. But I think I ask too much ;)
Hi, thanks for the feedback! There are some issues with this. The search service that Maps use to get information from OpenStreetMaps is Nominatim. We are not allowed to auto-complete or empploy search-ahead on this service. We could perform a search when the users hasn't typed for a while, but even this is a grey area I think. We could have a search-provider that completes against the recent places visited or the users favorite places, since we store them locally in Maps. I am not yet fully conviced. I am also working on storing recent routes one have performed, maybe when that is completed we could have search provider that searches the recent and favorite places and routes from Maps. Maybe.
In the absence of our own instance of Nominatim, which seems to require a large server, MapQuest seems to provide a version of the same API, without the usage limits. http://open.mapquestapi.com/nominatim/ Otherwise, without the completeness of OpenStreetMaps, Wikidata also offers coordinate locations for all cities and monuments. https://www.wikidata.org/wiki/Q206859 https://www.wikidata.org/w/api.php?action=wbgetclaims&entity=Q206859&property=P625 Probably without auto-complete or search-ahead, but I think it's better than nothing.
Yes, we looked into our own Nominatim and the price tag was in the $10000 range. Will have to look into the mapquest terms of use! If better we should be able to just repoint our nominatim.gnome.org proxy to it. Nominatim however isn't all that great. Type-ahead search would be an improvement but Nominatim does not do partial matches so it would still ned to type Paris to get a hit on Paris. And we want to stick with openstreetmaps, since we have editing planned :) Thanks Jonas
Maybe another easy to implement use case is to parse plain coordinates or geouris when they are written down in shell search box, so we can open those in Maps directly from the shell.
Yeah maybe... Feel free to implement something, if we start with a search provider that completes against our place store + some other stuff, we can see what that feels like. I don't want to add a provider, just because.
Photon (http://photon.komoot.de/) is the best OSM based reverse-geocoder I have found so far. It has search-as-you type support and uses the Nominatim import logic (basically just putting the data into an ES database for fast searches). I think Nominatim is a dead end and if we want to get something better we need to look for alternatives that actually provide what we need.
So to get this back on track. I would welcome patches for a search-provider that matches against: - Recent places / routes - Favourite places - lat/lon coordinates Anyone up for writing one?
For inspiration: https://git.gnome.org/browse/gnome-weather/tree/src/service
What details should we display in search results? I think for places : name, town, state, country. but I am not sure about routes. And for lat/lon in geo-uri format, we can simply display them back and when user clicks them, maps should open with those coordinates.
(In reply to Alaf from comment #9) > What details should we display in search results? > > I think for places : name, town, state, country. > but I am not sure about routes. > > And for lat/lon in geo-uri format, we can simply display them back and when > user clicks them, maps should open with those coordinates. How about the same infirmation we show in the search-popover in Maps on completion?
(In reply to Jonas Danielsson from comment #10) > (In reply to Alaf from comment #9) > > What details should we display in search results? > > > > I think for places : name, town, state, country. > > but I am not sure about routes. > > > > And for lat/lon in geo-uri format, we can simply display them back and when > > user clicks them, maps should open with those coordinates. > > How about the same infirmation we show in the search-popover in Maps on > completion? We should be able to re-use the placeFormatter class I think.
And I agree with lat/lon I think, and just convert them to geo:-uri, you can check for instance the utils.js code in Polari on how to launch a URI.
wait, now you got me confused. Polari opens default handler for urls in openURL function in utils.js. But we want to open maps with given coordinates taken from lat/lon or geo:uri given by user.(right?) examples of what I understood up to now. user inputs 'geo:37.786971,-122.399677' : we show result for that location and when user clicks, we open this location in maps. user input 37.786971 : we check if any route or place contains this as coordinate, if yes we show result. If user clicks, we open it in maps. user input 'New Delhi' : we match it against stored routes and places and show result. If user clicks, we open it in maps. I wanted to open everything using (geo:lat,lon) of places, since we can already open geo:uri in maps through command line. Later this might come in handy when we will try use online-services for searching?. Also, I think gnome-weather does this kind of thing in serviceProvider.js in _activateAction function by calling Gio.DBus.session.call(). So maybe we can do in same way?
(In reply to Alaf from comment #14) > wait, now you got me confused. > Polari opens default handler for urls in openURL function in utils.js. But > we want to open maps with given coordinates taken from lat/lon or geo:uri > given by user.(right?) > Well sure. > examples of what I understood up to now. > > user inputs 'geo:37.786971,-122.399677' : we show result for that location > and when user clicks, we open this location in maps. I am not sure about this one, why would one want to input a geo:-uri when we also accept lat/lon? > > user input 37.786971 : we check if any route or place contains this as > coordinate, if yes we show result. If user clicks, we open it in maps. > Not sure about this one either. Maybe. > user input 'New Delhi' : we match it against stored routes and places and > show result. If user clicks, we open it in maps. Yes, this one seems fine. > > I wanted to open everything using (geo:lat,lon) of places, since we can > already open geo:uri in maps through command line. > Later this might come in handy when we will try use online-services for > searching?. Maybe we instead could use actions via DBus? Like the show-contact action that Contacts use to show a contact in Maps. See application.js. > > Also, I think gnome-weather does this kind of thing in serviceProvider.js in > _activateAction function by calling Gio.DBus.session.call(). So maybe we can > do in same way? Yes, we can use activateAction, see utils.js in Maps for how we use activateAction. We use it today to activate actions on Clocks and Weather from send-to-dialog.js. I do not know a lot about service providers I have never looked into them. But I imagined that we would create a placeStore the same way we do in Maps application and complete against that in the provider. And for the lat/lon we would just match with regex. And give the option of open that point in Maps. Because I can't imagine a use-case were you would now the lat or lon of a point you did a route search on. We do not want to overload the search-provider system in GNOME with completions just because we can, right? Sounds like you are making progress tho! :)
(In reply to Jonas Danielsson from comment #15) > (In reply to Alaf from comment #14) > > wait, now you got me confused. > > Polari opens default handler for urls in openURL function in utils.js. But > > we want to open maps with given coordinates taken from lat/lon or geo:uri > > given by user.(right?) > > > > Well sure. > > > examples of what I understood up to now. > > > > user inputs 'geo:37.786971,-122.399677' : we show result for that location > > and when user clicks, we open this location in maps. > do we open maps on clicking a geo:uri? I don't know, my pc opens xdg-something.? > I am not sure about this one, why would one want to input a geo:-uri when we > also accept lat/lon? > > > > > user input 37.786971 : we check if any route or place contains this as > > coordinate, if yes we show result. If user clicks, we open it in maps. > > > > Not sure about this one either. Maybe. > yeah you are right. > > > user input 'New Delhi' : we match it against stored routes and places and > > show result. If user clicks, we open it in maps. > > Yes, this one seems fine. > > > > > I wanted to open everything using (geo:lat,lon) of places, since we can > > already open geo:uri in maps through command line. > > Later this might come in handy when we will try use online-services for > > searching?. > > > Maybe we instead could use actions via DBus? Like the show-contact action > that Contacts use to show a contact in Maps. See application.js. > > > > > Also, I think gnome-weather does this kind of thing in serviceProvider.js in > > _activateAction function by calling Gio.DBus.session.call(). So maybe we can > > do in same way? > > > Yes, we can use activateAction, see utils.js in Maps for how we use > activateAction. We use it today to activate actions on Clocks and Weather > from send-to-dialog.js. > > > I do not know a lot about service providers I have never looked into them. > But I imagined that we would create a placeStore the same way we do in Maps > application and complete against that in the provider. And for the lat/lon > we would just match with regex. And give the option of open that point in > Maps. Because I can't imagine a use-case were you would now the lat or lon > of a point you did a route search on. > yay!! that's exactly how I imagined of doing it :) Also I tried using already existing placestore.js, but since it imports application.js, I can't use it.(otherwise I.ll have to add everything in gresource.xml unnecessarily) Same application.js import problem comes with placeformatter.js too. It imports storedRoute.js, which imports application.js. That too only for one comparison. So maybe if I make it optional, then I can use placeformatter.js > We do not want to overload the search-provider system in GNOME with > completions just because we can, right? That is a whole lot, said awesomely in one line. :) > Sounds like you are making progress tho! :) yepp, I have done a lot. I have added necessary stuff to makefile, I have also somewhat learned how to make gresource.xml and org.gnome.Maps.BackgroundService.in files. Now mainly I just have to implement serviceProvider class. Also that problem I was facing in debugging, It was really silly, I was replacing entire gnome-shell, rather than restarting one process. Took me some time to realize.
(In reply to Alaf from comment #16) > > do we open maps on clicking a geo:uri? I don't know, my pc opens > xdg-something.? > > We do. Check the data/org.gnome.Maps.desktop.in file in the repository. It registers the following mime-types: MimeType=application/vnd.geo+json;x-scheme-handler/geo; This means that Maps will be available to handle geo: URIs. Maybe you need to install Maps on your system proper for it to take place. And maybe also run 'update-desktop-database' or something. For me when I clock geo: URIs for instance in the browser (maybe from the Geo URI wikipedia page) at least firefox suggests opening in Maps. > > I do not know a lot about service providers I have never looked into them. > > But I imagined that we would create a placeStore the same way we do in Maps > > application and complete against that in the provider. And for the lat/lon > > we would just match with regex. And give the option of open that point in > > Maps. Because I can't imagine a use-case were you would now the lat or lon > > of a point you did a route search on. > > > > yay!! that's exactly how I imagined of doing it :) > Also I tried using already existing placestore.js, but since it imports > application.js, I can't use it.(otherwise I.ll have to add everything in > gresource.xml unnecessarily) > > Same application.js import problem comes with placeformatter.js too. > It imports storedRoute.js, which imports application.js. That too only for > one comparison. So maybe if I make it optional, then I can use > placeformatter.js > Nice :) Maybe we can make placeFormatter and placeStore behave better? If it is not to messy we should be able to check if (!imports.storedRoute) or if (!imports.application) or similar checks and work around it? Or just structure the code differently. There shouldn't be any inherently depending on those things in the file. Alltho, maybe we need the storedRoute class? And the Place class as common code to be used in the service as well? Since the placeStore depends on saving Place and storedRoute is a subclass of Place? And in placeStore is application used for notifications? I am to lazy to check :) > > > We do not want to overload the search-provider system in GNOME with > > completions just because we can, right? > > That is a whole lot, said awesomely in one line. :) > > > Sounds like you are making progress tho! :) > > yepp, I have done a lot. I have added necessary stuff to makefile, I have > also somewhat learned how to make gresource.xml and > org.gnome.Maps.BackgroundService.in files. Now mainly I just have to > implement serviceProvider class. > Also that problem I was facing in debugging, It was really silly, I was > replacing entire gnome-shell, rather than restarting one process. Took me > some time to realize. Great Work!
(In reply to Jonas Danielsson from comment #17) > Nice :) Maybe we can make placeFormatter and placeStore behave better? > If it is not to messy we should be able to check if (!imports.storedRoute) > or if (!imports.application) or similar checks and work around it? > Or just structure the code differently. There shouldn't be any inherently > depending on those things in the file. Alltho, maybe we need the storedRoute > class? And the Place class as common code to be used in the service as well? > Since the placeStore depends on saving Place and storedRoute is a subclass > of Place? > > And in placeStore is application used for notifications? I am to lazy to > check :) Yeah, I have made small changes to PlaceStore, StoredRoute and geoclue, now they are free from import problems. I.ll submit a temporary commit so maybe you can review those changes. I think atleast these classes will be used. place, placeFormatter, placeMarker, placeStore, route, routeQuery, geoclue, storedRoute If(!imports.application) gives error if application.js is absent from gresource.xml so I used try catch.
Created attachment 317895 [details] [review] Remove necesstiy of importing application placeStore, storedRoute classes both imports 'application'. This makes them unusable if application is not available in imports. Now it is not necessory for placeStore and storedRoute classes to import application.
Review of attachment 317895 [details] [review]: Thanks Alaf! ::: src/geoclue.js @@ -25,3 @@ const Mainloop = imports.mainloop; -const Application = imports.application; Hah! Nice catch! ::: src/placeStore.js @@ +69,3 @@ + this._recentRoutesLimit = _RECENT_ROUTE_LIMIT; + } + Hmm, how about we instead add a parameter to this class? So in application.js were the place store is created we go: new PlaceStore.PlaceStore({ recentPlacesLimit: ..., recentRoutesLimit: ... }); And we do not need to import application anymore in placeStore. Seems nicer that this? ::: src/storedRoute.js @@ +57,3 @@ this._containsCurrentLocation = false; places.forEach((function(place) { + if (place === geoclue.place) Will this work? The point for this is to see if the route contains the current location. But will this new geoclue object you create really have time to get the current location? Seems fishy. Maybe do the same thing here and send the currentLocation as a parameter?
Review of attachment 317895 [details] [review]: ::: src/storedRoute.js @@ +57,3 @@ this._containsCurrentLocation = false; places.forEach((function(place) { + if (place === geoclue.place) And in the searchProvider case it could just be null, since we will not be adding any new storedRoute from the searchProvider, the check will not be important, since it will always be false. So it could be: if (place === currentPlace) instead.
Review of attachment 317895 [details] [review]: ::: src/placeStore.js @@ +69,3 @@ + this._recentRoutesLimit = _RECENT_ROUTE_LIMIT; + } + }; Ah, yes, you are right. That would be more cleaner. It will also work because we construct placeStore only once. (in application.js ~174) ::: src/storedRoute.js @@ +57,3 @@ this._containsCurrentLocation = false; places.forEach((function(place) { + if (place === geoclue.place) I am not sure, since current location may change after creation of storedRoute object. I think that is why its being dynamically taken from application.geoclue.place.? But yes you maybe right about geoclue obj not having enough time to acquire current location. Should I ask someone from geoclue, about how long it might take? or if we can 'make obj and take location' on the go?? Also on a side note: containsCurrentLocation is only used in _addRecentRoute in placeStore.js >if (stored.containsCurrentLocation) > return; I wasn't able understood why we don't store route if it contains current location? or basically why are we checking for current location. maybe I am missing something?
Review of attachment 317895 [details] [review]: ::: src/storedRoute.js @@ +57,3 @@ this._containsCurrentLocation = false; places.forEach((function(place) { + if (place === geoclue.place) Yes, the current location may change, you are right, so we need to send in a geoclue object instead, so make the storedRoute take a geoclue object, and if it is null then we know we do not need to care. The point is that when we are in searchProvider we will not store a new route, and this code guards against storing the current location. The reason we do not want to store the current location, is because that location changes. Right? And we do not have a good way of dealing with that. The name current location will be meaning less when the current location changes. And we do not want to reverse geocode it and store that because the result might be to far away to be meaningful. But in your case, sending the geoclue object as a construction parameter to storedRoute makes sense. You do not need to import application or geoclue and if geoclue-object is null you do not need to check if it is currentLocation, you will know it is not.
There is one more thing I just noticed. StoredRoute also contains a static method, which returns a new storedRoute. in that scenario we can not give it a geoclue object. and that might induce problems. Currentlocation is checked via stored.containsCurrentLocation, inside placeStore. (~146) So why not give a 'geoclue object' to placeStore as a param. Later call stored.containsCurrentLocation(geoclue.place) Inside storedRoute getter containsCurrentLocation(place) check if stored places contains the given place. And also then maybe we can rename containsCurrentLocation to containsLocation.
(In reply to Alaf from comment #24) > There is one more thing I just noticed. StoredRoute also contains a static > method, which returns a new storedRoute. in that scenario we can not give it > a geoclue object. and that might induce problems. > Well, from the searchProvider we will only ever read from the placeStore, never add to. In the placeStore there is no place that is the currentLocation. So the geoclue object is never needed from searchProvider and can always be null. No storedRoute will ever have the currentLocation, that is what the checks is all about. > > Currentlocation is checked via stored.containsCurrentLocation, inside > placeStore. > (~146) > > So why not give a 'geoclue object' to placeStore as a param. Later call > stored.containsCurrentLocation(geoclue.place) I think not, just pass null in the constructor from searchProvider, and do not bother about the static method, just pass null as geoclue (or not pass it at all so that it is undefined) from the static method as well. > > Inside storedRoute getter containsCurrentLocation(place) check if stored > places contains the given place. > > And also then maybe we can rename containsCurrentLocation to > containsLocation.
Thanks for pointing, it out, I think I got way too deep in it. Okay. Sure. Also, What properties should we match against in our search. Weather uses name, city, country. Our available options are : name, street_address, street, building, postal_code, area, town, state, county, country, country_code, continent I suggest name, street, area, town, country. And do also have to look in contacts? I think no. right.
Created attachment 318290 [details] [review] this is a temp commit
(In reply to Alaf from comment #26) > Thanks for pointing, it out, I think I got way too deep in it. Okay. Sure. > > Also, > What properties should we match against in our search. Weather uses name, > city, country. > > Our available options are : > name, street_address, street, building, postal_code, area, town, state, > county, country, country_code, continent > > I suggest name, street, area, town, country. > Hmm, I think maybe a simpler approach? In my mind this should feel like the search in Maps. So why not do exactly like in Maps and use the match-function in place.js? It only matches against name, but I feel that is good enough. > And do also have to look in contacts? I think no. right. No, contacts is Contacts business :)
Review of attachment 318290 [details] [review]: Looks promising! I know it is temp, I added some comments. Thanks! ::: data/Makefile.am @@ +15,3 @@ +org.gnome.Maps.BackgroundService.data.gresource: org.gnome.Maps.BackgroundService.data.gresource.xml $(service_resource_files) + $(AM_V_GEN) $(GLIB_COMPILE_RESOURCES) --target=$@ --sourcedir=$(srcdir) $< + Could this be formatted prettier? Like the sed row below maybe? @@ +49,2 @@ servicedir = $(datadir)/dbus-1/services +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service BackgroundService.service? Background.service? background.service? @@ +50,3 @@ +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service + +searchproviderdir = $(datadir)/gnome-shell/search-providers Is this what other use? why not just search-provider? (will we ever have more than one?) @@ +57,3 @@ org.gnome.Maps.service.in \ + org.gnome.Maps.BackgroundService.service.in \ + org.gnome.Maps.BackgroundService.data.gresource.xml \ Align the \ here and below ::: data/ShellSearchProvider2.xml @@ +1,1 @@ +<node> Can this be named anything? ::: src/Makefile.am @@ +1,1 @@ NULL = Can we have a src/service/Makefile.am for this stuff? ::: src/application.js @@ +176,3 @@ + placeStore = new PlaceStore.PlaceStore({ recentPlacesLimit: _recentPlacesLimit, + recentRoutesLimit: _recentRoutesLimit, + geoclue: this.geoclue }); All these stuff as a separate commit, right? ::: src/placeStore.js @@ +274,3 @@ + // this.set(iter, [Columns.ICON], [pixbuf]); + // }).bind(this)); + // } We could have a parameter to placeStore that says it is in background-mode? ::: src/service/main.js @@ +37,3 @@ + 'GtkClutter': '1.0', + 'Rest': '0.7', + 'Soup': '2.4' }); This list can't be true, right? @@ +60,3 @@ + +const BackgroundService = new Lang.Class({ + Name: 'MapsBackgroundService', Why different name? @@ +66,3 @@ + this.parent({ application_id: pkg.name, + flags: Gio.ApplicationFlags.IS_SERVICE, + inactivity_timeout: 60000 }); What does this timeout do? is it ms? s? make it a const and motivate the length. @@ +67,3 @@ + flags: Gio.ApplicationFlags.IS_SERVICE, + inactivity_timeout: 60000 }); + GLib.set_application_name(_("Maps")); This will change? @@ +85,3 @@ + _initPlaceStore: function() { + let _recentPlacesLimit = settings.get('recent-places-limit'); + let _recentRoutesLimit = settings.get('recent-routes-limit'); We could also set -1 for both of these I think, then you will not need settings? @@ +94,3 @@ + } catch (e) { + log('Failed to parse Maps places file, ' + + 'subsequent writes will overwrite the file!'); this log is not needed of course. @@ +97,3 @@ + } + + this.model = placeStore; No need for the placeStore temp variable. ::: src/service/searchProvider.js @@ +32,3 @@ + +const SearchProvider = new Lang.Class({ + Name: 'MapsSearchProvider', Different name? @@ +74,3 @@ + placeStore.foreach((function(model, path, iter) { + let p = model.get_value(iter, PlaceStore.Columns.PLACE); + if (p.name.indexOf(terms)> -1) { if (p.match(terms)) ? what is terms?
(In reply to Jonas Danielsson from comment #28) > (In reply to Alaf from comment #26) > > Thanks for pointing, it out, I think I got way too deep in it. Okay. Sure. > > > > Also, > > What properties should we match against in our search. Weather uses name, > > city, country. > > > > Our available options are : > > name, street_address, street, building, postal_code, area, town, state, > > county, country, country_code, continent > > > > I suggest name, street, area, town, country. > > > > Hmm, I think maybe a simpler approach? In my mind this should feel like the > search in Maps. So why not do exactly like in Maps and use the > match-function in place.js? It only matches against name, but I feel that is > good enough. > So that is place.match() and maybe a regex to see if it matches dd.dd, dd.dd type of lat/lon combo? > > > And do also have to look in contacts? I think no. right. > > No, contacts is Contacts business :)
Review of attachment 318290 [details] [review]: Thank you, Jonas for the review. ::: data/Makefile.am @@ +15,3 @@ +org.gnome.Maps.BackgroundService.data.gresource: org.gnome.Maps.BackgroundService.data.gresource.xml $(service_resource_files) + $(AM_V_GEN) $(GLIB_COMPILE_RESOURCES) --target=$@ --sourcedir=$(srcdir) $< + yeah. okay. @@ +49,2 @@ servicedir = $(datadir)/dbus-1/services +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service Weather uses this naming convention. Also since we also have BackgroundService.gresource @@ +50,3 @@ +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service + +searchproviderdir = $(datadir)/gnome-shell/search-providers undefined @@ +57,3 @@ org.gnome.Maps.service.in \ + org.gnome.Maps.BackgroundService.service.in \ + org.gnome.Maps.BackgroundService.data.gresource.xml \ okay. ::: src/application.js @@ +176,3 @@ + placeStore = new PlaceStore.PlaceStore({ recentPlacesLimit: _recentPlacesLimit, + recentRoutesLimit: _recentRoutesLimit, + geoclue: this.geoclue }); Yup. Should I submit now? or after finishing everything? ::: src/org.gnome.Maps.BackgroundService.src.gresource.xml @@ +10,3 @@ + <file>route.js</file> + <file>routeQuery.js</file> + <file>geoclue.js</file> I will also minimize this list, once service is up and running. ::: src/placeStore.js @@ +274,3 @@ + // this.set(iter, [Columns.ICON], [pixbuf]); + // }).bind(this)); + // so commenting it. yeah, okay. ::: src/service/main.js @@ +37,3 @@ + 'GtkClutter': '1.0', + 'Rest': '0.7', + 'Soup': '2.4' }); Hahaha, right. @@ +60,3 @@ + +const BackgroundService = new Lang.Class({ + Name: 'MapsBackgroundService', Again, followed same naming convention as weather. @@ +66,3 @@ + this.parent({ application_id: pkg.name, + flags: Gio.ApplicationFlags.IS_SERVICE, + inactivity_timeout: 60000 }); It is ms, but I don't know what exactly inactivity means here. Weather had it. I think this is max time in which we should return results. @@ +85,3 @@ + _initPlaceStore: function() { + let _recentPlacesLimit = settings.get('recent-places-limit'); + let _recentRoutesLimit = settings.get('recent-routes-limit'); would -1 will mean placelimit = infinity / all places ? @@ +94,3 @@ + } catch (e) { + log('Failed to parse Maps places file, ' + + 'subsequent writes will overwrite the file!'); yeah, although main maps application also loads in the same way. @@ +97,3 @@ + } + + this.model = placeStore; okay. ::: src/service/searchProvider.js @@ +32,3 @@ + +const SearchProvider = new Lang.Class({ + Name: 'MapsSearchProvider', Again, same convention as weather. @@ +74,3 @@ + placeStore.foreach((function(model, path, iter) { + let p = model.get_value(iter, PlaceStore.Columns.PLACE); + if (p.name.indexOf(terms)> -1) { Our search term, coming from user. basically a string.
@@ +50,3 @@ +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service + +searchproviderdir = $(datadir)/gnome-shell/search-providers yes, actually this is standard place where we need to register our service with gnome-shell.
Review of attachment 318290 [details] [review]: ::: src/application.js @@ +176,3 @@ + placeStore = new PlaceStore.PlaceStore({ recentPlacesLimit: _recentPlacesLimit, + recentRoutesLimit: _recentRoutesLimit, + geoclue: this.geoclue }); Update the other patch in this bug and that can go in before all this. ::: src/service/main.js @@ +60,3 @@ + +const BackgroundService = new Lang.Class({ + Name: 'MapsBackgroundService', Follow the Maps convention instead :) The name and the class should have the same name. BackgroundService... it does not say much ... but ok use it for both. @@ +66,3 @@ + this.parent({ application_id: pkg.name, + flags: Gio.ApplicationFlags.IS_SERVICE, + inactivity_timeout: 60000 }); Can we find out? @@ +85,3 @@ + _initPlaceStore: function() { + let _recentPlacesLimit = settings.get('recent-places-limit'); + let _recentRoutesLimit = settings.get('recent-routes-limit'); recentLimits are really only used when adding, to make sure we do not add to many. It is also used when reading from file the first time. Setting to -1 will bypass (as the code reads now) and add all there is in the file. And since we do not add anything from this backgroundService it will be the same as the limit from the application, right? @@ +94,3 @@ + } catch (e) { + log('Failed to parse Maps places file, ' + + 'subsequent writes will overwrite the file!'); Yes, but there are no writes from the background service... and no console? ::: src/service/searchProvider.js @@ +32,3 @@ + +const SearchProvider = new Lang.Class({ + Name: 'MapsSearchProvider', Use Maps convention instead. SearchProvider / SearchProvider, we know we are Maps. @@ +74,3 @@ + placeStore.foreach((function(model, path, iter) { + let p = model.get_value(iter, PlaceStore.Columns.PLACE); + if (p.name.indexOf(terms)> -1) { Ok then just use p.match.
(In reply to Alaf from comment #32) > @@ +50,3 @@ > +service_DATA = org.gnome.Maps.service > org.gnome.Maps.BackgroundService.service > + > +searchproviderdir = $(datadir)/gnome-shell/search-providers > > yes, actually this is standard place where we need to register our service > with gnome-shell. Ok!
Created attachment 318645 [details] [review] Remove necesstiy of importing application placeStore, storedRoute classes both imports 'application'. This makes them unusable if application is not available in imports. Now it is not necessory for placeStore and storedRoute classes to import application.
Review of attachment 318645 [details] [review]: Thanks Alaf, I will fix this up a bit (see comments) and push it! ::: src/application.js @@ +172,3 @@ _initPlaceStore: function() { + let _recentPlacesLimit = settings.get('recent-places-limit'); + let _recentRoutesLimit = settings.get('recent-routes-limit'); no need for _ prefix here, and not need for temp variables. @@ +176,3 @@ + placeStore = new PlaceStore.PlaceStore({ recentPlacesLimit: _recentPlacesLimit, + recentRoutesLimit: _recentRoutesLimit, + geoclue: this.geoclue }); No need to include geoclue here, right? ::: src/storedRoute.js @@ +58,3 @@ + + let currentLocation = null; + if(this.geoclue) if (...)
Yup, sorry somehow missed that. Too many files and branches. _/\_
Created attachment 318808 [details] Sceen cast Screen cast.
(In reply to Alaf from comment #38) > Created attachment 318808 [details] > Sceen cast > > Screen cast. Cool! nice work!
How should we match lat/long ? right now I am doing this. > const LAT_LONG_REGEX = /^\d+.\d*,\d+.\d*$/; . . . >let isLatLong = queryString.match(LAT_LONG_REGEX); . . . > if(isLatLong) { > let coordinates = [p.location.longitude, p.location.latitude].join(','); > if (queryString == coordinates) { ... and what about accuracy level? if query string is 23.5,77.416667 23.5,77.416667 23.5,77.416668 23.5,77.4166 23.5,77.41 which ones should be matched??
Same as in Maps I would suggest.https://git.gnome.org/browse/gnome-maps/tree/src/placeEntry.js#n38 Maybe move that somewhere?
Created attachment 318998 [details] [review] workable demo patch with this, shell search is almost complete 2 things remains, better lat/long comparison and starting maps.
I think _parseCoordinates, and _validateCoordinates both will do good in place.js. Along with regex. https://git.gnome.org/browse/gnome-maps/tree/src/placeEntry.js#n184
Review of attachment 318998 [details] [review]: Thanks! Some minor comments below, no in-depth review. ::: data/Makefile.am @@ +84,3 @@ $(NULL) # For uninstalled use Please take care to keep all \ aligned at 72 chars, you have created quite a mess here! :) ::: src/Makefile.am @@ +72,3 @@ $(NULL) install-exec-hook: Same in this file, keep the \ aligned. ::: src/application.js @@ +156,3 @@ + + // here think we might get a problem, since _initPlaceStore is called after adding action + So, yes, I think so, we can have a state property on placeStore, and if it is done then look the place up, and if not connect to 'notify::state'. @@ +234,3 @@ onActivate: this._onOsmAccountSetupActivate.bind(this) + }, + 'show-place':{ I think maybe show-stored ? Since it will not show a arbitrary place, right? And we might want an actual show-place sometime ::: src/org.gnome.Maps.BackgroundService.in @@ +1,2 @@ +#!@GJS@ +imports.package.init({ name: "gnome-maps", is this the correct name? ::: src/service/searchProvider.js @@ +83,3 @@ + if(isLatLong) { + let coordinates = [p.location.longitude, p.location.latitude].join(','); + if (queryString == coordinates) { === @@ +145,3 @@ + }, + + _activateAction: function(action, parameter, timestamp) { Why not use the one from Utils?
(In reply to Alaf from comment #43) > I think _parseCoordinates, and _validateCoordinates both will do good in > place.js. > Along with regex. > > https://git.gnome.org/browse/gnome-maps/tree/src/placeEntry.js#n184 Sure, agreed, we can move it to place.js and use it in background as well.
Review of attachment 318998 [details] [review]: ::: data/Makefile.am @@ +84,3 @@ $(NULL) # For uninstalled use Oops, I guess my sublime text doesn't play well with make files. I.ll make it pretty again. ::: src/org.gnome.Maps.BackgroundService.in @@ +1,2 @@ +#!@GJS@ +imports.package.init({ name: "gnome-maps", Yes, anything other than this does not work. gives this error >JS ERROR: Error: No JS module 'service' found in search path ::: src/service/searchProvider.js @@ +145,3 @@ + }, + + _activateAction: function(action, parameter, timestamp) { see comment below. @@ +192,3 @@ + // new GLib.Variant('s', locationUrl), + // timestamp); + 188 // the way this gets platform data somehow breaks it in background. 189 // using activateAction from weather for now. 190 // Utils.activateAction('org.gnome.Maps', 191 // 'show-location', 192 // new GLib.Variant('s', locationUrl), 193 // timestamp); I am suspecting this line https://git.gnome.org/browse/gnome-maps/tree/src/utils.js#n135
Review of attachment 318998 [details] [review]: ::: src/service/searchProvider.js @@ +192,3 @@ + // new GLib.Variant('s', locationUrl), + // timestamp); + I want to use the one from Utils, so maybe we can make that one work instead of this? If you print the desktop-startup-id here and from Utils (when starting whether for instance) what is the difference? What do we need context for in Utils? Can we get it without going through display?
yes, I agree using utils one is better. okay, I.ll investigate.
Created attachment 319004 [details] [review] moved parseCoordinates, validateCoordinates to place.js parseCoordinates, validateCoordinates and coordinate regex are moved to place.js. Now we can use them in background service.
Review of attachment 319004 [details] [review]: Thanks! Other than the mis-file this looks great! But the commit message needs some love. Maybe skip the the message body all together. And make the subject: "Move parsing of coordinates o Place module" ? ::: src/org.gnome.Maps.BackgroundService @@ +4,3 @@ + prefix: "/home/alaf/jhbuild/install", + libdir: "/home/alaf/jhbuild/install/lib" }); +imports.package.run(imports.service.main); No newline at end of file This file should not be here, right?
oho. sorry, too many file and branches. okay, I.ll fix that.
Created attachment 319006 [details] [review] move parsing of coordinates to Place module
Please obsolete old patches when adding new ones. If you are using git bz and have changed the name of the commit you can use "git attach -e ..." to edit which patch the new one obsoletes!
Review of attachment 319006 [details] [review]: Thanks!
For now, clicking on results give this in "journalctl -f" http://ur1.ca/oexv0 Any idea, why this is happening? PS. utils.activate_action works.
Alaf, with what patch and how do I reproduce the issue?
Created attachment 319046 [details] [review] full patch upto now
Steps to reporduce Register the search provider with GNOME Shell by dropping a file in which is /usr/share/gnome-shell/search-providers file contents: [Shell Search Provider] DesktopId=org.gnome.Maps.desktop BusName=org.gnome.Maps.BackgroundService ObjectPath=/org/gnome/Maps/BackgroundService Version=2 DefaultDisabled=true save it as org.gnome.Maps.search-provider.ini the go to setting and turn ON search for maps. now go to jhbuild/checkout/gnome-maps/src and run ./org.gnome.Maps.BackgroundService also moniter journalctl -f from another terminal. search somthing in shell-search, so that a maps result is displayed open it. wait for 10 seconds.
Created attachment 319047 [details] [review] full patch upto now
I will not review this more until patches that are obsolete are marked as such and patches that are meant for review / check applies on master.
it was for reproducing the error.
(In reply to Alaf from comment #61) > it was for reproducing the error. I know but it does not apply with git bz apply and using git bz apply is really hard when old patches are around, please clean up in this bug.
Created attachment 319048 [details] [review] full patch upto now
Review of attachment 319004 [details] [review]: reviewed.
Review of attachment 318998 [details] [review]: reviewed.
Thanks Alaf! please mark the patches at the top of this bug that are not needed anymore as obosolete otherwise git bz will try to apply them and it makes the bug hard to follow. Do you use git-bz? If not please start, it will help out alot! The following patches seems not needed anymore and should be marked obsolete: - moved parseCoordinates, validateCoordinates to place.js - workable demo patch with this, shell search is almost complete 2 things remains, better lat/long comparison and starting maps. - this is a temp commit I understand that they were temporary, but they interfere. When I go: $ git bz apply 744032 from the command line to fetch the patches, all are sugested and I have to edit a list, and guess which apply.
click details => edit details then mark as obsolete, it is burdensome to do in web-ui, easier with git-bz command line tool.
Created attachment 319050 [details] making all obsolete.
Created attachment 319074 [details] [review] full
$ LANG=en_US git bz apply 744032 Bug 744032 - GNOME Shell search provider 319074 - full Apply? [(y)es, (n)o, (i)nteractive] y Applying: full fatal: sha1 information is lacking or useless (src/application.js). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 full The copy of the patch that failed is found in: /home/jonasdn/jhbuild/checkout/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/full-Jl46ZZ.patch
Created attachment 319110 [details] [review] shell service
Thanks, I got results in shell! But not able to activate action, right. Are you albe to launch maps from the shell at all? Btw is the ico nreally working, seems I got generic for all matches?
Yes, I am able to launch maps from shell. jhbuild shell gnome-maps and also jhbuild shell gnome-maps/src/ ./org.gnome.Maps Ah, yes I now realize Icons are broken. Thanks for pointing it out.
Maps is not opening because activate action opens system default maps. Which does not have "show-stored" action. what do I do now? :/ How do I replace my default maps with jhbuild one?
Created attachment 319456 [details] [review] GNOME Shell search provider
Review of attachment 319456 [details] [review]: Thanks! Some early comments ::: src/application.js @@ +155,3 @@ + + let id = parameter.deep_unpack(); + this._createWindow(); I do not like the look of this, maybe change so that the get method just takes the id? ::: src/mapView.js @@ +110,3 @@ view.kinetic_mode = true; + if (!opening && !Application.openingStoredPlace) I do not like this trend, maybe we should switch to instead something like if (normalStartup) { ... } @@ +343,3 @@ + this._placeLayer.add_marker(marker); + new MapWalker.MapWalker(place, this).goTo(true); + }, this function is pretty much exactly the same as showSearchResult, maybe that could be showPlace instead? and used for both? ::: src/service/main.js @@ +32,3 @@ +const Gtk = imports.gi.Gtk; +const GtkClutter = imports.gi.GtkClutter; +const Maps = imports.gi.GnomeMaps; What are Maps, GtkClutter and GObject used for? @@ +41,3 @@ +// amount if time after which we close process after showing results. +// in MS +const INACTIVITY_TIMOUT = 60 * 1000; // 60 secs Seems like a long time? @@ +57,3 @@ + flags: Gio.ApplicationFlags.IS_SERVICE, + inactivity_timeout: INACTIVITY_TIMOUT }); + GLib.set_application_name(_("Maps")); Is the name really Maps? And why translated? Who is going to see this? @@ +73,3 @@ + recentRoutesLimit: -1, + inBackground: true }); + placeStore.load(); No try/catch here? What happens if the file is malformed? @@ +74,3 @@ + inBackground: true }); + placeStore.load(); + Remove this new line @@ +80,3 @@ + vfunc_dbus_register: function(connection, path) { + this.parent(connection, path); + No new line ::: src/service/searchProvider.js @@ +24,3 @@ +const Lang = imports.lang; + +const Settings = imports.settings; Not used @@ +37,3 @@ + _init: function(application) { + this._app = application; + No new line @@ +49,3 @@ + }, + + GetInitialResultSetAsync: function(params, invocation) { Why are we using ThisStyle on these functions? @@ +52,3 @@ + this._app.hold(); + + No new lines @@ +83,3 @@ + // print(p.location.longitude+","+p.location.latitude + "was saved"); + }; + } else if(p.match(queryString)) { check all your ifs and fors and whiles, we want a space before paranthesis. if ( for ( while ( ... @@ +90,3 @@ + + this._app.release(); + no new line @@ +108,3 @@ + } + this._app.release(); + No new line @@ +133,3 @@ + + this._app.release(); + no new line ::: src/utils.js @@ +134,2 @@ function _getPlatformData(appId, timestamp) { + // this breaks in background. No comments like this, either we change or we do not. @@ +145,3 @@ let objectPath = '/' + appId.replace(/\./g, '/'); + // let platformData = _getPlatformData(appId, timestamp); + let platformData = {'desktop-startup-id': new GLib.Variant('s', '_TIME' + timestamp) }; Not sure I like this, will there be a difference in the string? Please format it better if keeping.
Review of attachment 319456 [details] [review]: ::: src/application.js @@ +155,3 @@ + + let id = parameter.deep_unpack(); + this._createWindow(); okay, I.ll change placeStore.get to take either Id or a Place. ::: src/mapView.js @@ +110,3 @@ view.kinetic_mode = true; + if (!opening && !Application.openingStoredPlace) Ok. @@ +343,3 @@ + this._placeLayer.add_marker(marker); + new MapWalker.MapWalker(place, this).goTo(true); + }, yeah, okay. ::: src/service/main.js @@ +41,3 @@ +// amount if time after which we close process after showing results. +// in MS +const INACTIVITY_TIMOUT = 60 * 1000; // 60 secs Docs says 10 seconds should be default. so now 10 sec. @@ +73,3 @@ + recentRoutesLimit: -1, + inBackground: true }); + placeStore.load(); Good point. I guess we should just quit if file is malformed as nothing to search form then. ::: src/service/searchProvider.js @@ +49,3 @@ + }, + + GetInitialResultSetAsync: function(params, invocation) { We are implementing an Interface, so we have to keep same names.
Created attachment 319575 [details] [review] GNOME Shell search provider
Review of attachment 319575 [details] [review]: Thanks! ::: data/Makefile.am @@ +16,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Maps.BackgroundService.data.gresource.xml \ Could we use a variable to avoid this long line? @@ +57,2 @@ servicedir = $(datadir)/dbus-1/services +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service Can we split this to two lines like in other places? ::: src/Makefile.am @@ +30,3 @@ + --target=$@ \ + --sourcedir=$(srcdir)/service \ + --sourcedir=$(srcdir) $< Why are these both here and in data/ ? ::: src/application.js @@ +54,3 @@ let contactStore = null; let osmEdit = null; +let normalStartup = true; Maybe split this out in a separate patch? Do we not need this for --local option as well, for instance? ::: src/mapView.js @@ +394,3 @@ }, + showPlace: function(place, animation) { This should be a patch of its own ::: src/org.gnome.Maps.BackgroundService.src.gresource.xml @@ +16,3 @@ + <file>settings.js</file> + <file>translations.js</file> + <file>utils.js</file> Do we really need all this? ::: src/placeStore.js @@ +265,3 @@ added]); + if (place.icon !== null && !this._inBackground) { Why couldn't we load icons? Don't we need to be able to do this to have icons working? @@ +278,3 @@ + + if(place.uniqueID) + uniqueID = place.uniqueID; What is going on here? Why is uniqueID set to the place-object? ::: src/service/main.js @@ +57,3 @@ + + if (!pkg.moduledir.startsWith('resource://')) + this.debug = true; Is this ever used? @@ +71,3 @@ + placeStore.load(); + } catch(e) { + this.quit(); So what happens here? The search provider can not work until we have a places file?
Review of attachment 319575 [details] [review]: ::: src/mapView.js @@ +394,3 @@ }, + showPlace: function(place, animation) { okay. ::: src/placeStore.js @@ +278,3 @@ + + if(place.uniqueID) + uniqueID = place.uniqueID; if we call placeStore.get(idA) then uniqueID is directly assigned to idA, if we have called placeStore.get(place) then, uniqueId is assigned to place.uniqueID. So now we can call it both ways. At least that's what I tried to do. how should I make it better? ::: src/service/main.js @@ +71,3 @@ + placeStore.load(); + } catch(e) { + this.quit(); If we don't have places file, what we will compare search string to !?
Created attachment 320142 [details] [review] mapview: goto stored location only if starting maps normally
Created attachment 320143 [details] [review] mapView: rename showSearchResult to showPlace
Review of attachment 320142 [details] [review]: Thanks! ::: src/application.js @@ +54,3 @@ let contactStore = null; let osmEdit = null; +let normalStartup = true; Why add it here? It is never used in the context of an App-global variable? You could add it as a member to the class. Or you could go all the way and have it here. Then you do not have to send it to mainWindow at all and instead go: if (Application.normalStartup) { ... } in mapView.
Review of attachment 320142 [details] [review]: I still wonder though, do we not need to have normalStartup = false; when options.contains('local') is true?
Review of attachment 320143 [details] [review]: Thanks Alaf!
Created attachment 320325 [details] [review] mapview: goto stored location only if starting maps normally
Review of attachment 320325 [details] [review]: I kind of liked the idea of setting it as a global in application, sameway as osmEdit and stuff. And then we can remove all trace of this in mainWindow and mapView just checks Application.normalStartup. It can be true by default. And application set: normalStartup = false; In vfunc_open and when the --local flag is found. ::: src/mainWindow.js @@ +75,3 @@ + if (this.application.local_tile_path) { + _mapType = MapView.MapType.LOCAL; + this.application.normalStartup = false; ? why?
Created attachment 320342 [details] [review] mapview: goto stored location only if starting maps normally
Review of attachment 320342 [details] [review]: Looks fin
Review of attachment 319575 [details] [review]: ::: data/Makefile.am @@ +16,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Maps.BackgroundService.data.gresource.xml \ okay ::: src/org.gnome.Maps.BackgroundService.src.gresource.xml @@ +16,3 @@ + <file>settings.js</file> + <file>translations.js</file> + <file>utils.js</file> Although we don't directly use translations but its imported in place.js. settings I.ll remove. ::: src/placeStore.js @@ +265,3 @@ added]); + if (place.icon !== null && !this._inBackground) { with your patch in 761533, icons work. And no, we don't need to load icon pixbuf in background service. Its never used.
Created attachment 320687 [details] [review] GNOME Shell search provider
Review of attachment 320687 [details] [review]: Thanks! Looking good! ::: data/Makefile.am @@ +60,2 @@ servicedir = $(datadir)/dbus-1/services +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service service_data = \ ... \ ... ::: src/Makefile.am @@ +22,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Maps.BackgroundService.src.gresource.xml \ why does this need a $(srcdir) ? you are passing $(srcdir) to the program? ::: src/application.js @@ +154,3 @@ + this._createWindow(); + this._checkNetwork(); + this._mainWindow.present(); Is this really needed? We are activating an action on Maps, this should already be done I think. These three should be removed. @@ +158,3 @@ + let id = parameter.deep_unpack(); + let place = placeStore.get(id); + this._checkNetwork(); missing space? place, true ::: src/placeStore.js @@ +62,3 @@ delete params.recentRoutesLimit; + this._inBackground = params.inBackground; I think I prefer loadIcon @@ +265,3 @@ added]); + if (place.icon !== null && !this._inBackground) { then this becomes && this._loadIcon) @@ +278,3 @@ + + if(place.uniqueID) + uniqueID = place.uniqueID; I do not like this trickery at all it is magic Do a getById function instead. Or convert all other places to just pass the id, if that is all that is needed. ::: src/service/main.js @@ +20,3 @@ + */ + +pkg.initGettext(); What is getting translated? @@ +37,3 @@ + +// amount if time (in ms) for which service should wait for the calling message to arrive +const INACTIVITY_TIMOUT = 10 * 1000; // 10 secs 10 seconds seems like a lot? @@ +69,3 @@ + } catch(e) { + this.quit(); + } So regarding this. This means the background service will never work on the first session, right? We could add a Gio.FileMonitor if the file does not exists and try to load it when it does? And until then could we not match against lat/lon? @@ +85,3 @@ + + Utils.addActions(this, { + 'quit': { onActivate: this._onQuit.bind(this) }}); Do we really need this? What activates this action? @@ +91,3 @@ + vfunc_activate: function() { + // do nothing, this is a background service + }, Do we then need to have it defined? ::: src/service/searchProvider.js @@ +29,3 @@ +const PlaceFormatter = imports.placeFormatter; + +const SearchProviderInterface = Gio.resources_lookup_data('/org/gnome/shell/ShellSearchProvider2.xml', 0).toArray().toString(); This is not pretty. Are there no better way of doing this? Could we atleast set the path of the shellsearchprovider to intermediate variable? const _searchProviderPath = '/org... @@ +77,3 @@ + + let ret = []; + for (let i = 0; i < previous.length; i++) { Can't we go previous.forEach(function(something) { ... }); ? Or at least for (let i in previous) @@ +92,3 @@ + GetResultMetas: function(identifiers) { + this._app.hold(); + Remove empty line @@ +96,3 @@ + + for (let i = 0; i < identifiers.length; i++) { + empty line @@ +129,3 @@ + LaunchSearch: function(terms, timestamp) { + this._app.hold(); + empty @@ +130,3 @@ + this._app.hold(); + + this._activateAction('show-stored', new GLib.Variant('s', terms.join(' ')), timestamp); ? where is thus._activateAction? ::: src/utils.js @@ +135,2 @@ let info = Gio.DesktopAppInfo.new(appId + '.desktop'); + let id = new GLib.Variant('s', [info.get_id(),'_TIME',timestamp].join('-')); And you are sure this is the same as getting the launch context? Do we get the right notifications from the shell on launch?
Review of attachment 320687 [details] [review]: ::: data/Makefile.am @@ +60,2 @@ servicedir = $(datadir)/dbus-1/services +service_DATA = org.gnome.Maps.service org.gnome.Maps.BackgroundService.service already present naming convention https://git.gnome.org/browse/gnome-maps/tree/data/Makefile.am#n39 ::: src/Makefile.am @@ +22,3 @@ + --sourcedir=$(srcdir) \ + --generate-dependencies \ + $(srcdir)/org.gnome.Maps.BackgroundService.src.gresource.xml \ nope. I am passing gresource.xml to resource_files https://git.gnome.org/browse/gnome-maps/tree/src/Makefile.am#n11 ::: src/application.js @@ +154,3 @@ + this._createWindow(); + this._checkNetwork(); + this._mainWindow.present(); I am not sure. These three are also present in '_onShowContactActivate' As it appears, 'vfunc_activate' should be called somewhere when activating action, but its not called. if we remove these three lines, application crashes. _onShowContactActivate https://git.gnome.org/browse/gnome-maps/tree/src/application.js#n139 vfunc_activate https://git.gnome.org/browse/gnome-maps/tree/src/application.js#n277 @@ +158,3 @@ + let id = parameter.deep_unpack(); + let place = placeStore.get(id); + this._checkNetwork(); oops. ::: src/placeStore.js @@ +62,3 @@ delete params.recentRoutesLimit; + this._inBackground = params.inBackground; okay :) @@ +278,3 @@ + + if(place.uniqueID) + uniqueID = place.uniqueID; okay, I.ll try both ways, whatever changes least, I.ll do. ::: src/service/main.js @@ +69,3 @@ + } catch(e) { + this.quit(); + } I am not sure that we need to use a file monitor. I think background services are restarted every time user searches and later killed(quit) after inactivity. So even if user has recent searches it will work. how should we match against lat/lon without having anything in place store? ah maybe display one result of same lat/lon back to user, where he can click to go to that lat/lon ? something like that? ::: src/service/searchProvider.js @@ +29,3 @@ +const PlaceFormatter = imports.placeFormatter; + +const SearchProviderInterface = Gio.resources_lookup_data('/org/gnome/shell/ShellSearchProvider2.xml', 0).toArray().toString(); okay. @@ +77,3 @@ + + let ret = []; + for (let i = 0; i < previous.length; i++) { sure. @@ +130,3 @@ + this._app.hold(); + + this._activateAction('show-stored', new GLib.Variant('s', terms.join(' ')), timestamp); Haww !!! thanks for pointing it out. I almost missed it. LaunchSearch is called when the user clicks on the provider icon to display more search results in the application. tl:dr we need another action something like 'show-search', which should show search results with current terms in the application. ::: src/utils.js @@ +135,2 @@ let info = Gio.DesktopAppInfo.new(appId + '.desktop'); + let id = new GLib.Variant('s', [info.get_id(),'_TIME',timestamp].join('-')); Strings produced by both are different.I am not sure what difference does it make (Need expert advice here!). but one thing is certain, Gdk.Display does not work in background. Also this approach I have taken from gnome-weather.
Review of attachment 320687 [details] [review]: ::: src/service/main.js @@ +69,3 @@ + } catch(e) { + this.quit(); + } Yes, that is how I always imagined lat/lon match working. Not against placeStore but showing arbitrary lat/lon in Maps ::: src/utils.js @@ +135,2 @@ let info = Gio.DesktopAppInfo.new(appId + '.desktop'); + let id = new GLib.Variant('s', [info.get_id(),'_TIME',timestamp].join('-')); Ok, I am scared of regression in the opening of stuff from Maps. The values here somehow make sure we get the correct timing and animations and popup/popunder from shell I think. Not sure what best way is. Can we try to get the launch context from some other way that does not involve display? what does the gapplication-tool do? It is a commandline tool
Review of attachment 320687 [details] [review]: ::: src/application.js @@ +158,3 @@ + let id = parameter.deep_unpack(); + let place = placeStore.get(id); + this._checkNetwork(); And what happends if this is a stored route?
Created attachment 321733 [details] [review] GNOME Shell search provider
Created attachment 321734 [details] [review] GNOME Shell search provider
Review of attachment 321734 [details] [review]: Thanks! Some comments below. I want this to match against places _and_ routes in the place store. I do not want to match against latitude longitude of stuff in the place store, what would be the use-case for that? I'd rather not match lat/lon at all. I do want to match against recent routes however, the same way we do in the application, and show the same information we do, and open the route. ::: src/application.js @@ +177,3 @@ + place = new Place.Place({ store: false, + location: location }); + }; I do not like this magic "look at the length of args" can't we have two parameters to the action instead? Or maybe better, two actions? One showLocation that takes latitude/longitude parameters. And one showStored that takes an uniqueId of the placeStore and the type as parameters, so we know if it is a route or a place. @@ +181,3 @@ + }, + + _onShowSearch:function (action, parameter) { No, we do not want this, do a _onShowLocation instead that has two parameters one lat one lon. ::: src/placeStore.js @@ +286,3 @@ + } + return false; + }).bind(this)); I see no reason this needs to bind to this? @@ +288,3 @@ + }).bind(this)); + return storedPlace; + }, I do not see the point of this, a user will never know a latitude/longitude pair and want to match against places in the place store... avoid feature creep at all cost. No code without use-case. @@ +292,3 @@ + getFromId: function(uniqueID) { + return this.get({ uniqueID: uniqueID }); + }, Does the get: function(place) below ever need anything other than the id from place? Couldn't we convert it to take an id instead? Then we do not need this hack? And convert all callers. ::: src/service/main.js @@ +83,3 @@ + + Utils.addActions(this, { + 'quit': { onActivate: this._onQuit.bind(this) }}); Who sends this action? ::: src/service/searchProvider.js @@ +63,3 @@ + let p = model.get_value(iter, PlaceStore.Columns.PLACE); + let type = model.get_value(iter, PlaceStore.Columns.TYPE); + if (type === PlaceStore.PlaceType.RECENT_ROUTE) return; What about favorite route? And why can't we match against routes? I think that would be my biggest use-case of this search-provider. @@ +78,3 @@ + if (Place.Place.validateCoordinates(lat, lon)) { + let data = [queryString, PlaceStore.PlaceType.LATLONG].join(','); + ret.push(data); Just open this location if the user push, no search from placeStore, use a show-location action. @@ +144,3 @@ + this._app.hold(); + Utils.activateAction('org.gnome.Maps', + 'show-search', show-location with lat/lon
Review of attachment 321734 [details] [review]: ::: src/application.js @@ +178,3 @@ + location: location }); + }; + if (args.length === 2) { okay. I split it in two actions. @@ +181,3 @@ + }, + + // place-id,type ah no, onShow Search is different from lat/long thing or place thing. It passes only the search terms (query string that user type) and search them in placeEntery. See, when we search something, results are displayed, and if we click on the maps Icon, not on any of search results that time this action is activated. and this search terms in shown in placeEntery. ::: src/placeStore.js @@ +288,3 @@ + }).bind(this)); + return storedPlace; + if (p.location.latitude === lat && p.location.longitude === lon) { okay @@ +292,3 @@ + getFromId: function(uniqueID) { + return this.get({ uniqueID: uniqueID }); + return true; okay.
Created attachment 323422 [details] [review] routeQuery: move Transporatation to route StoredRoute.js imports routeQuery, for using routeQuery.Transportaion. after #f4cdc18, routeQuery.js uses application. This makes StoredRoute unusable in background service mode.
Created attachment 323437 [details] [review] placeStore: take uniqueID as parameter in placestore.get
Created attachment 323438 [details] [review] GNOME Shell Search provider
Review of attachment 323422 [details] [review]: Do we not need to fix up places that reference RouteQuery.Transporation, like sidebar? $ git grep Transportation [...] src/sidebar.js: let transport = RouteQuery.Transportation; [...]
Review of attachment 323437 [details] [review]: Looks fine!
Review of attachment 323438 [details] [review]: Thanks! Could we perhaps break out the creation of new actions t oa patch of its own? So one patch that adds each action and explains why it is needed? ::: src/application.js @@ +161,3 @@ }, + _onShowStored:function (action, parameter) { no spaces before parenthesis @@ -161,0 +163,21 @@ + _onShowStored:function (action, parameter) { + normalStartup = false; + ... 18 more ... where does parameter come from? @@ +182,3 @@ + let place; + let data = parameter.deep_unpack(); + }, why can't we have two parameters one lat one lon? @@ -161,0 +163,26 @@ + _onShowStored:function (action, parameter) { + normalStartup = false; + ... 23 more ... needs blank line @@ +192,3 @@ + this._createWindow(); + this._checkNetwork(); + this._createWindow(); Seeing this for the third time... maybe a small helper is called for? _presentWindow: function() { this._createWindow(); this._checkNetwork(); this._mainWindow.present(); }, ? @@ +196,3 @@ + let data = parameter.deep_unpack(); + this._mainWindow._placeEntry.text = data; + this._mainWindow.present(); _-prefix on placeEntry means the property is private and should not be referenced from here. What does this function do? I do not udnerstand why we need a showSearch @@ +289,1 @@ } when is this show-search ever used? I do not get it, why do we want it? ::: src/service/searchProvider.js @@ +130,3 @@ + Utils.activateAction('org.gnome.Maps', + 'show-location', + new GLib.Variant('s', data), why a string? Why not two integers? @@ +146,3 @@ + 'show-search', + new GLib.Variant('s', terms.join(' ')), + timestamp); what is the purpose of this?
Created attachment 323725 [details] [review] routeQuery: move Transporatation to route StoredRoute.js imports routeQuery, for using routeQuery.Transportaion. after #f4cdc18, routeQuery.js uses application. This makes StoredRoute unusable in background service mode.
Created attachment 323732 [details] [review] application: Add new actions show-location: Shows location from 'lat,lon' show-stored: Shows stored route or place, from uniqueID show-search: Search Map for given 'terms'
Created attachment 323733 [details] [review] application: Add new actions show-location: Shows location from 'lat,lon' show-stored: Shows stored route or place, from uniqueID show-search: Search Map for given 'terms'
Review of attachment 323733 [details] [review]: Thanks! Splitting this up in three patches would be better. ::: src/application.js @@ +174,3 @@ + let place; + let data = parameter.deep_unpack(); + let [lat,lon] = data.split(','); Can't we avoid using strings for this? And instead to two doubles? @@ +187,3 @@ + let data = parameter.deep_unpack(); + this._mainWindow._placeEntry.text = data; + this._mainWindow._placeEntry._onActivate(); This is not acceptable. 1) placeEntry is a private member of mainWindow and should _not_ be accessed here 2) You are calling a private member-function that is a callback. 3) We have to be able to do better than this! Maybe create a search-action in mainWindow.js?
Review of attachment 323733 [details] [review]: marking
Created attachment 324522 [details] [review] routeQuery: move Transporatation to route StoredRoute.js imports routeQuery, for using routeQuery.Transportaion. after #f4cdc18, routeQuery.js uses application. This makes StoredRoute unusable in background service mode.
Review of attachment 324522 [details] [review]: Looks fine!
Created attachment 324722 [details] [review] application: add show stored action shellSearch calls show-stored action when user selects a result that is a stored route or a place.
Created attachment 326285 [details] [review] application: add show location action shellSearch use show-location action to display a point from latlong string. Example: User directly enters a latlong string, 23.323,34.233 in search. we show single result, which opens requested point in maps.
I don't know if this belongs on a separate bug, but it would be useful to have Maps show up as a search provider in the overview for any string, in terms of being able to quickly pull up a map for an arbitrary location or route. At the moment in a web browser, I can add a search keyword (e.g. "m") for $juggernaut maps, and search with: m timbuktu or m new york to london and get a useful map very quickly (especially since web browsers are often permanently open). Especially in the routing case For Maps, this is much laborious at the moment. I have to: * launch Maps * Tab-Enter to open the routing sidebar * Type the start location, hit Enter, wait for results, arrow to the first result * Tab to the destination box, type the destination, hit Enter, wait for results, arrow to the first result, hit Enter As an alternative, maybe Maps could look for an overview search keyword, e.g. 'map', and split on "to" if it's in the string. This would enable: <Super> map new york to london <Enter> to achieve the same result very quickly. Maps would then detect "to", use the first result for "new york" as the origin, the first result for "london" as the destination, and immediately display results.
I need to close this for migration purposes, sorry for the noise.