GNOME Bugzilla – Bug 731587
Add Support for Fetching and Displaying Point of Interests (POI)
Last modified: 2018-03-26 12:39:14 UTC
As a Part of GSoC project [1], I have been working on supporting POIs in GNOME-Maps. I have used overpass API to fetch POIs. I will be uploading the relevant patches here. [1] https://wiki.gnome.org/Outreach/SummerOfCode/2014/Projects/RishiRajSinghJhelumi_MapsPointsOfInterests
Created attachment 278452 [details] [review] POI Patch V1 -Test This is the first version of the patch to support fetching and displaying of POIs. The patch is rebased to the current master at this time. The icons for POIs are NOT dependent on geocode-glib. In this patch I have used some maki icons and shown how Maps can use it without depending for icon on Geocode, although the place type is Geocode. The current version of the patch supports: 1. Calls to overpass to get POIs 2. Rendering of POIs 3. Caching (both memory and File) 4. Some dummy POI tags that needs to be searched for (this list will be modified later). 5. POI icons for some types. Please suggest necessary changes and I will modify the patch accordingly, and possibly break it into smaller patches if required.
Created attachment 278490 [details] [review] POI Patch V1 -Test POI Patch V1 -Test This is the first version of the patch to support fetching and displaying of POIs. The patch is rebased to the current master at this time. The icons for POIs are NOT dependent on geocode-glib. In this patch I have used some maki icons and shown how Maps can use it without depending for icon on Geocode, although the place type is Geocode. The current version of the patch supports: 1. Calls to overpass to get POIs 2. Rendering of POIs 3. Caching (both memory and File) 4. Some dummy POI tags that needs to be searched for (this list will be modified later). 5. POI icons for some types. Please suggest necessary changes and I will modify the patch accordingly, and possibly break it into smaller patches if required.
Some short notes after a quick review: 1. Split the patches up a bit. Look into old patch-reviews for how this should be done. 2. Remove any out commented code 3. Remove all remaining debug-logging (or use Utils.debug, but then see to it that you have sensible debug messages). Also in general: go over the code 2-3 times and try to see that you haven't missed anything. Then re-submit and I'll happily review it! :)
Yeah, I agree with Mattias, I am reluctant to really review this without having it split up. Also, about the POI icons. I do not think we should any any icons to maps that are already in Geocode. Our subclass of Maps could first check if their is an icon for the current type available from Geocode and use that, and if not fall back to the ones Maps provide. Or we go a route where Geocode no longer provides icons, deprecating the get_icon method and have Maps provide all of them. Mattias, input?
Why not add missing icons to geocode-glib?
(In reply to comment #5) > Why not add missing icons to geocode-glib? In a mail from Rishi he stated the following to me: "So, the other day Mattias and me had a discussion with Bastein about whether the poi's should be included in geocode-glib or in Maps. He suggested for now, include the pois in the Maps itself, after a round or two when it becomes useful to other users, they may include it later. So, how should I go about implementing place types in Maps, because geocode doesn't provide apis that can be overridden." I haven't talked to Bastian myself.
And of course one course of action could be to only use the types/icon for POIs in maps that we are able to get into geocode-glib.
(In reply to comment #6) > (In reply to comment #5) > > Why not add missing icons to geocode-glib? > > > In a mail from Rishi he stated the following to me: > > "So, the other day Mattias and me had a discussion with Bastein about > whether the poi's should be included in geocode-glib or in Maps. > He suggested for now, include the pois in the Maps itself, after a > round or two when it becomes useful to other users, they may include > it later. > > So, how should I go about implementing place types in Maps, because > geocode doesn't provide apis that can be overridden." > > I haven't talked to Bastian myself. This seems to be about POIs rather that their icons. We already have plenty of place types and icons in geocode-glib so I don't think adding more is wrong.
(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #5) > > > Why not add missing icons to geocode-glib? > > > > > > In a mail from Rishi he stated the following to me: > > > > "So, the other day Mattias and me had a discussion with Bastein about > > whether the poi's should be included in geocode-glib or in Maps. > > He suggested for now, include the pois in the Maps itself, after a > > round or two when it becomes useful to other users, they may include > > it later. > > > > So, how should I go about implementing place types in Maps, because > > geocode doesn't provide apis that can be overridden." > > > > I haven't talked to Bastian myself. > > This seems to be about POIs rather that their icons. We already have plenty of > place types and icons in geocode-glib so I don't think adding more is wrong. Well, I kind of agree. Even thou I understand that Bastian might not want to be glib about adding new types without knowing if they will be used [by others than maps]. This is how the discussion on mail continued, I sort of asked the same question as you, and Mattias replied: "> > So, the other day Mattias and me had a discussion with Bastein about > > whether the poi's should be included in geocode-glib or in Maps. > > He suggested for now, include the pois in the Maps itself, after a > > round or two when it becomes useful to other users, they may include > > it later. > > > > What exactly is meant by pois here? The Geocode.PlaceTypes? Or the icons? > He didn't want to add any new place types at the moment? Yeah exactly, he wants the place types to be universally usable, which I think makes sense."
Created attachment 279256 [details] [review] Add Overpass.js
Created attachment 279257 [details] [review] Add geoMath.js
Created attachment 279258 [details] [review] Add mapOverlaySource.js
Created attachment 279259 [details] [review] Add place.js
Created attachment 279260 [details] [review] Add poi.js
Created attachment 279261 [details] [review] Make mapView globally accesible. I am not sure about this, but it can be made global as our application has a single View of the Map.
Created attachment 279262 [details] [review] Add poiMapSource, poiRenderer and poiLocation.
Created attachment 279263 [details] [review] Add base maki icons. It contains icons provided by the geocode-glib. Its because to remove dependency on geocode-glib for icons. Added a few new icons from the maki set.
Created attachment 279264 [details] [review] Add poiLayer and plug-in POIMapSource with mapView.
Review of attachment 279256 [details] [review]: Thanks! Please change the log message to something nicer, no need to mention what file is added. Maybe: Add wrapper module for the Overpass API And fix up the body a bit as well. ::: src/overpass.js @@ +64,3 @@ + // HTTP Session Variables + this._session = new Soup.Session(); + this._session.use_thread_context = true; Is this really needed? @@ +65,3 @@ + this._session = new Soup.Session(); + this._session.use_thread_context = true; + Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault()); Same here, what does this do? Isn't this done automagicly? @@ +68,3 @@ + }, + + addSearchTag: function(key, value) { I think maybe "addTag" would be enough. @@ +69,3 @@ + + addSearchTag: function(key, value) { + Remove empty line. @@ +72,3 @@ + // The special phrase supported by OSM can be found at: + // http://wiki.openstreetmap.org/wiki/Nominatim/Special_Phrases/EN + The Nominatim special phrases have nothing to do with OSM tags, right? Maybe remove this. @@ +80,3 @@ + + send: function(bbox, callback) { + Remove empty line. @@ +89,3 @@ + + this._session.queue_message(request, (function(obj, message) { + Why these empty lines? @@ +103,3 @@ + + callback(pois); + We could skip the pois variable here and call callback on the JSON.parse... in the try-block and callback([]); in the catch-block. @@ +126,3 @@ + this._getOutputString() + ]); + }, Would it make sense to use the new HTTP(Query) module (added by Mattias as part of the route code) for this (and some of the below)? @@ +147,3 @@ + + _getTagString: function() { + Again with the empty line. @@ +169,3 @@ + +}); +Utils.addSignalMethods(Overpass.prototype); I do not see any signals being emitted, so this can be removed. And then the Utils module is not needed.
Review of attachment 279257 [details] [review]: Thanks. Looks ok.
Review of attachment 279258 [details] [review]: Thanks! Please fixup commit log similar as for the Overpass wrapper. ::: src/mapOverlaySource.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Jonas Danielsson <jonas@threetimestwo.org> Feel free to change this to you. @@ +29,3 @@ +const _NAME = 'GNOME Maps Overlay'; +const _LICENSE = 'tbd'; +const _LICENSE_URI = 'tbd'; These needs to be addressed. My initial 'tbd' meant "To be determined", maybe this needs to be the Maki license, I am not sure. Maybe you could ask Mattias on IRC or he could chime in here. And maybe they should be empty in this abstract class, and set in baseclass that actually produce data.
Review of attachment 279257 [details] [review]: Commit log: Add GeoMath module. And fixup body.
Review of attachment 279262 [details] [review]: Thanks! Please fixup the commit log as in other reviews. Please also run git diff --check and fixup all white space damage. ::: src/poiLocation.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Jonas Danielsson <jonas@threetimestwo.org> Change to you. @@ +34,3 @@ +const POILocation = new Lang.Class({ + Name: 'POILocation', + Extends: MapLocation.MapLocation, Isn't MapLocation on the way out? With Damians patches? Maybe this should based on his code? @@ +50,3 @@ + + show: function() { + Extra empty line again. ::: src/poiMapSource.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Jonas Danielsson <jonas@threetimestwo.org> Change this to you. @@ +109,3 @@ + let bbox = GeoMath.bboxFromTile(tile); + this.overpassQuery.send(bbox, (function(pois) { + Extra empty line. @@ +117,3 @@ + + tile.connect('render-complete', (function(tile, data, size, error) { + Extra empty line. @@ +118,3 @@ + tile.connect('render-complete', (function(tile, data, size, error) { + + if(!error) { Insert a space after if, also below, and in any other code you add :) @@ +127,3 @@ + this.next_source.fill_tile(tile); + } + Extra empty line. @@ +131,3 @@ + } +}); +Utils.addSignalMethods(POIMapSource.prototype); What signals are emitted? ::: src/poiRenderer.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Jonas Danielsson <jonas@threetimestwo.org> Change to you. @@ +29,3 @@ +const POIRenderer = new Lang.Class({ + Name: 'POIRenderer', + Extends: Champlain.Renderer, Whitespace damage, please use git diff --check on all new code. @@ +59,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack What does this hack do exactly? Is it only to bring the data to the render-complete callback? Then can't it be passed instead of null below? How will this work with the caches? will they handle the render-complete correctly? And store cache correctly? @@ +63,3 @@ + } +}); +Utils.addSignalMethods(POIRenderer.prototype); What signals are emitted?
Review of attachment 279261 [details] [review]: No, I don't like this. MapView could be passed as an argument when creating the renderer.
Review of attachment 279259 [details] [review]: I don't see the point of having a class that extends GeocodePlace and adds or changes nothing.
Review of attachment 279260 [details] [review]: Thanks! Please fixup commit log. ::: src/poi.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Jonas Danielsson <jonas@threetimestwo.org> Change to you. @@ +108,3 @@ + let key = null; + let value = null; + Extra line. @@ +118,3 @@ + return getPOIIconFromTag(key, value); +} + Please fix up whitespace damage. Everywhere. @@ +141,3 @@ + + return poi; +} Is the JSON in these functions specific to the Overpass API? If so it should live in Overpass.js @@ +156,3 @@ + + get_icon: function() { + Extra line @@ +169,3 @@ + set place_type(value) { + this._place_type = value; + } So the place_type property won't affect this object at all? And there is no way to set or get the type outside of construction? @@ +171,3 @@ + } +}); +Utils.addSignalMethods(POI.prototype); What signals are emitted?
Review of attachment 279260 [details] [review]: Also this patch can't come before the icon commit, since it references them.
Review of attachment 279263 [details] [review]: I am still not sure about this. I do not want to add _any_ icons that are also in geocode-glib. I think. Maybe we should just limit ourselves to the icons there. Either that or have the Poi.js code check if an icon exists in geocode and then use that, otherwise use a local one. Zeeshan, Mattias, please chime in.
Review of attachment 279264 [details] [review]: ::: src/mapView.js @@ +97,3 @@ + this.poiLayer.show_all_markers(); + } + }).bind(this)); Can't this be done from the poiOverlaySource or the poiRenderer? Having that define here feels wrong. That should be the knowledge of the overlay only.
Review of attachment 279258 [details] [review]: Remember that we use 4 spaces, no tabs for indentation.
Review of attachment 279262 [details] [review]: ::: src/poiLocation.js @@ +34,3 @@ +const POILocation = new Lang.Class({ + Name: 'POILocation', + Extends: MapLocation.MapLocation, As Jonas said, this one should be called POIMarker and inherits from MapMarker (see #722871) @@ +46,3 @@ + + // Add popover code for POI here + log(place.name); In the near future, you will need to implement MapMarker::_createBubble method here, just check the SearchResultMarker class at #722871 (the latest attachment) for reference. Also, feel free to reuse the SearchResultBubble to create your own bubble and/or rename SearchResultBubble if you feel that we can use the same class for SearchResultMarker and POIMarker bubbles (according the mockups, both bubbles are really similar). @@ +74,3 @@ + + }).bind(this)); + } Once you port this to be a MapMarker, you'll need to move the actor creation code to the constructor and implements addToLayer method to show the marker. POIRenderer needs to have a reference to the MapView instead of making it global (as Jonas said), so POIRenderer will call the addToLayer method, you wont need _poiLayer property anymore on this class. Also, you can think about a way to have several layers for each POI type and select the appropriate layer in POIRenderer, this enables us to add features like POI filtering. @@ +77,3 @@ + +}); +Utils.addSignalMethods(POILocation.prototype); Be careful with addSignalMethods, if you inherit your class from a native class and call this method for your class, you will override the natives connect/disconnect/emit methods and possibly you will need some aspirin to figure out whats happening :) ::: src/poiMapSource.js @@ +131,3 @@ + } +}); +Utils.addSignalMethods(POIMapSource.prototype); You're inheriting from Champlain.TileSource in the hierarchy tree, so you actually don't need to do that.
Review of attachment 279257 [details] [review]: ::: src/geoMath.js @@ +57,3 @@ + bottom: tileToLatitude(tile.zoom_level, tile.y + 1), + right: tileToLongitude(tile.zoom_level, tile.x + 1) + }); Alignment: return new Champlain.BoundingBox({ top: tileToLatitude(tile.zoom_level, tile.y), left: tileToLongitude(tile.zoom_level, tile.x), bottom: tileToLatitude(tile.zoom_level, tile.y + 1), right: tileToLongitude(tile.zoom_level, tile.x + 1) });
Sorry, duplicated review :(
Thanks Jonas, Damian,I will come up with updated patches soon.
Created attachment 282172 [details] [review] Add math module.
Created attachment 282173 [details] [review] Add base maki icons.
Created attachment 282174 [details] [review] Add MapOverlay module.
Created attachment 282175 [details] [review] Add POI place module.
Created attachment 282176 [details] [review] Add overpass wrapper.
Created attachment 282177 [details] [review] Add poi popover and bubble.
Created attachment 282179 [details] [review] Add unicode conversion methods.
Created attachment 282181 [details] [review] Add poi overlay and renderer.
Created attachment 282182 [details] [review] mapView: plugin poiMapSource.
All of the patches are based on Damian's popover patches from https://bugzilla.gnome.org/show_bug.cgi?id=722871.
About the Maki icons for the POIs being in Maps or geocode-glib. I think that they should be in Maps as it would be better if we could remove the dependency on geocode-glib for the Icons. Mattias, also thinks that its a bit weird having icons in geocode-glib. But still, if I am missing any point behind it, please let me know.
Review of attachment 282173 [details] [review]: Maybe I should not review this one because I'm quite stupid using autotools :) , but anyways. ::: data/icons/Makefile.am @@ +36,3 @@ + scalable_places_poi-theatre.svg \ + scalable_places_poi-museum.svg + This should be terminated with $(NULL) ? Also, please visualize this file with 8 tab width and align backslashes with tabs. @@ +87,3 @@ + $(INSTALL_DATA) $(srcdir)/maki/$$icon $(DESTDIR)$(datadir)/icons/gnome/$$ICON; \ + done + Maybe we need to follow the same convention used in public_icons (that is, using cut, instead of sed), just to be polish. @@ +90,1 @@ uninstall-icons: Forgot uninstall rules for poi_icons?
Review of attachment 282174 [details] [review]: Hmmm... do we want this class? it doesn't add anything else to ChamplainTileSource except for a fill_tile default implementation (which actually doesn't add anything useful to the derived classes). Also, there is no need to override the getters in that way if you initialize the properties to the parent object in the constructor. Maybe we can remove this commit and just inherit POIMapSource from ChamplainTileSource.
Review of attachment 282175 [details] [review]: ::: src/poi.js @@ +98,3 @@ + +function getPOIIconFromTag(key, value) { + return poiTypes[key][value] || _POI_DEFAULT_ICON; If "key" doesn't exist in "poiTypes", it will crash instead of returning _POI_DEFAULT_ICON. Can overpass send to us a new "key" that we don't have defined in "poiTypes"? @@ +107,3 @@ + for(k in poiTypes) { + if (k in place.tags) + key = k; Could we stop the loop when "k in place.tags == true" right? @@ +109,3 @@ + key = k; + } + value = place.tags[key]; What happen if we cannot find the tag (key == null)? @@ +130,3 @@ + }); + return icon; + }, What about override "icon" property getter? get_icon for a method name is odd. @@ +132,3 @@ + }, + + get place_type() { Overriding GeocodePlace::place-type property getter doesn't seem to be a good idea, the types of this._type and for place_type property are different and setting place_type to Geocode.PlaceType.POINT_OF_INTEREST as you did in overpass.js is fine. Maybe you should add a property called poi_type to store this value (having this.poi_type without getter and setter is fine in this case since getter and setter doesn't have any particularity) @@ +135,3 @@ + return this._type; + } + Extra blank line
Review of attachment 282176 [details] [review]: ::: src/overpass.js @@ +40,3 @@ +function convertJSONPlaceToPOI(place) { + let name = _UNKNOWN; + if(place.tags) Space after "if" @@ +48,3 @@ + accuracy: 0, + description: name + }); You should align this like: let location = new Geocode.Location({ latitude: place.lat, longitude: place.lon, accuracy: 0, description: name }); (I hope that looks well aligned in fixed width font :P ) @@ +56,3 @@ + osm_id: place.id.toString(), + type: POI.getPOITypeFromPlaceJSON(place) + }); Same here @@ +96,3 @@ + 'key': key, + 'value': value + }); Same here @@ +105,3 @@ + method: 'GET', + uri: uri + }); Same here @@ +126,3 @@ + BASE_URL, + this._generateOverpassQuery(bbox) + ]); What about? Format.vprintf('%s?data=%s', [ BASE_URL, this._generateOverpassQuery(bbox) ]); @@ +138,3 @@ + this._getTagString(), + this._getOutputString() + ]); Same here @@ +148,3 @@ + bbox.top, + bbox.right + ]); Same here @@ +156,3 @@ + key, + value + ]); Same here @@ +166,3 @@ + tag.key, + tag.value + ]); Same here @@ +177,3 @@ + this.outputSortOrder, + this.outputCount, + ]); Same here @@ +179,3 @@ + ]); + } + Unneeded blank line
Review of attachment 282177 [details] [review]: ::: src/poiMarker.js @@ +48,3 @@ + pixbuf.get_height(), + pixbuf.get_rowstride() + ); Alignment @@ +59,3 @@ + layer.add_marker(this); + } + }, Why this? I think this is already managed is POIRenderer/POIMapSource. @@ +69,3 @@ + place: this.place, + mapView: this._mapView + }); Alignment
Review of attachment 282181 [details] [review]: ::: src/poiMapSource.js @@ +32,3 @@ +const _FILE_CACHE_NUM = 1e9; +const _MEMORY_CACHE_NUM = 200; +const _MIN_POI_DISPLAY_ZOOM_LEVEL = 16; This one can be useful in other parts of Maps as I'll tell you later, you should name it MIN_POI_DISPLAY_ZOOM_LEVEL (without the initial underscore) @@ +79,3 @@ + vfunc_fill_tile: function(tile) { + + if (tile.get_state() === Champlain.State.DONE || tile.get_state() === Champlain.State.LOADED) What about tile.state instead tile.get_state()? @@ +83,3 @@ + + if (tile.zoom_level < _MIN_POI_DISPLAY_ZOOM_LEVEL) { + tile.set_state(Champlain.State.DONE); Same here: tile.state = Champlain.State.DONE @@ +99,3 @@ + this.cache.store_tile(tile, tile.data, tile.data.length); + } + tile.set_state(Champlain.State.DONE); Same here: tile.state = Champlain.State.DONE @@ +106,3 @@ + }).bind(this)); + } + Extra blank line here ::: src/poiRenderer.js @@ +28,3 @@ + +const _UNKNOWN = 'Unknown'; +const _MIN_POI_DISPLAY_ZOOM_LEVEL = 16; You must reuse the const declared in poiMapSource @@ +34,3 @@ + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ +function correctJSONData(data) { What about "fixJSONData"? also, I think this should be part of POIRenderer as a private method. @@ +53,3 @@ + let view = this._mapView.view; + + this._mapView.poiLayer = new Champlain.MarkerLayer(); Hmmmm... I don't know... this should be a property of MapView or ChamplainRenderer, should be constructed in MapView::_initLayers method or here? another opinion would be great. @@ +54,3 @@ + + this._mapView.poiLayer = new Champlain.MarkerLayer(); + this._mapView.poiLayer.set_selection_mode(Champlain.SelectionMode.MULTIPLE); Why multiple selection? I ask you because I am working in markers patch-set so the layers with multiple selection shows many bubbles at a time (I'm showing the bubble when the marker got selected). @@ +64,3 @@ + poiLayer.show_all_markers(); + } + }).bind(this)); Why remove all markers here? Doing poiLayer.hide_all_markers or poiLayer.hide instead is not enough when zoom is higher than min? @@ +114,3 @@ + place: place, + mapView: this._mapView + }); Alignment @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack I still do not understand this well. If it's not avoidable, it deserves a better comment.
As general comments: - Great work! - Don't end the commit subject with a period - Don't add an extra line to the end of classes body - Mattias, Jonas, are we ending files with new lines or there is nothing specified? - Check all the alignment problems (surely I didn't marked everything) - Use "let a = object.foo" instead "let a = object.get_foo()" where possible - Use "object.foo = a" instead "object.set_foo(a)" where possible
Review of attachment 282172 [details] [review]: Looks ok.
Review of attachment 282173 [details] [review]: Still want to get a reply from Bastian on bug #730896 before I decide on this.
Review of attachment 282174 [details] [review]: I think I agree with Damian here, until we need more things to be overlay-sources we might just want to inherit from ChamplainTileSource.
Review of attachment 282175 [details] [review]: ::: src/poi.js @@ +130,3 @@ + }); + return icon; + }, Agreed, do a getter for icon property. @@ +132,3 @@ + }, + + get place_type() { I think the issue is that we would like this object to be treated as a GeocodePlace, such as we use as a "place" type in rest of Maps. But at the same time Rishi wants to add more types than the GeocodePlace has. And GeocodeGlib does not want to add more types at the moment. So the "real" solution might be to switch to our own "Place" object and convert the GeocodePlace we get from forward/reverse search to that. But not sure.
Review of attachment 282179 [details] [review]: Are thre no Glib based functions for this?
Review of attachment 282181 [details] [review]: We will need to fill in the name/license/other vfuncs here if we remove the super class. We need to think about what the license should say.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ What is the reason for this? is it a bug in Champlain? Have you filed a bug report? Is it reproducible with a small example? @@ +53,3 @@ + let view = this._mapView.view; + + this._mapView.poiLayer = new Champlain.MarkerLayer(); I think it should be constructed in the _initLayers method of MapView. @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack I do not like it one bit. Is it really needed? Can't it be passed in the emit below? Will this work properly with the caching? (between memory and file)
Review of attachment 282182 [details] [review]: Maybe log: mapView: Use poiMapSource to overlay Points of Interests. ::: src/mapView.js @@ +117,3 @@ let source = this._factory.create_cached_source(mapType); this.view.set_map_source(source); + this.view.add_overlay_source(this._poiSource, 255); Overlay sources are removed when the map source is set. So I guess this needs to be re-set on source-changed?
Review of attachment 282177 [details] [review]: ::: src/poiMarker.js @@ +59,3 @@ + layer.add_marker(this); + } + }, I did this because in our chat you told that you will be removing addToLayer method as it was just used in UserLocation, and you will be moving that method to UserLocationMarker, so I thought I shouldn't depend on it. Anyways, I need this method., I would override yours even if you keep it.
Review of attachment 282181 [details] [review]: ::: src/poiMapSource.js @@ +79,3 @@ + vfunc_fill_tile: function(tile) { + + if (tile.get_state() === Champlain.State.DONE || tile.get_state() === Champlain.State.LOADED) Yeah, now it sound sane as mapView also use properties directly instead of methods. @@ +83,3 @@ + + if (tile.zoom_level < _MIN_POI_DISPLAY_ZOOM_LEVEL) { + tile.set_state(Champlain.State.DONE); yup. @@ +99,3 @@ + this.cache.store_tile(tile, tile.data, tile.data.length); + } + tile.set_state(Champlain.State.DONE); yup. ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ I am not sure, I have checked the @@ +54,3 @@ + + this._mapView.poiLayer = new Champlain.MarkerLayer(); + this._mapView.poiLayer.set_selection_mode(Champlain.SelectionMode.MULTIPLE); Because if I keep it SINGLE, if I click on any marker all of them get selected. @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack Jonas, you remember the thread about this named POI Caching. I tried various things, but every time it produces the same error. (gnome-maps:12007): GLib-GObject-CRITICAL **: g_value_unset: assertion 'G_IS_VALUE (value)' failed (gnome-maps:12007): Gjs-WARNING **: JS ERROR: Error: Cannot convert non-null JS value to G_POINTER It may be a champlain bug, I am not sure.
Review of attachment 282176 [details] [review]: ::: src/overpass.js @@ +40,3 @@ +function convertJSONPlaceToPOI(place) { + let name = _UNKNOWN; + if(place.tags) :) @@ +48,3 @@ + accuracy: 0, + description: name + }); I think my alignment looks more clearer. { 'key': 'value', 'key': 'value', 'key': 'value', } with k-v pairs or function parameters in separate lines and closing at the last line. Same case everywhere. But if maps follows the other convention STRICTLY I will surely switch.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ I am not sure, I have checked the JSON from the overpass server and it looks fine. Maybe its not a champlain bug and it due to the "render-complete" hack I am using. @@ +53,3 @@ + let view = this._mapView.view; + + this._mapView.poiLayer = new Champlain.MarkerLayer(); I initially did there, but I thought since POIRenderer renders on POILayer it should be in POIRenderer. @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack I have been unable to pass it as the second parameter to the emit. Look at https://developer.gnome.org/libchamplain/unstable/ChamplainTile.html#ChamplainTile-render-complete . Yes, it works properly with caching.
Review of attachment 282175 [details] [review]: ::: src/poi.js @@ +98,3 @@ + +function getPOIIconFromTag(key, value) { + return poiTypes[key][value] || _POI_DEFAULT_ICON; possible, if nominatim adds more tags. I will write a null check. @@ +107,3 @@ + for(k in poiTypes) { + if (k in place.tags) + key = k; yup. :) @@ +109,3 @@ + key = k; + } + value = place.tags[key]; ^^ same as above. @@ +130,3 @@ + }); + return icon; + }, ok. @@ +132,3 @@ + }, + + get place_type() { Yeah, I think it needs to be overridden because of the new POI icon types. I could change to poi_type but it won't change anything, i would still have to pass it as a parameter.
Review of attachment 282179 [details] [review]: Not direct functions.
Review of attachment 282176 [details] [review]: ::: src/overpass.js @@ +48,3 @@ + accuracy: 0, + description: name + }); I told because I used the same alignment as you in my first patches versions and Mattias told me to change it, so I think these are strict style conventions :)
Review of attachment 282177 [details] [review]: ::: src/poiMarker.js @@ +59,3 @@ + layer.add_marker(this); + } + }, Yeah, I will definitely remove addToLayer method from MapMarker. But what I tried to mean is, why check "this._mapView.view.zoom_level >= _MIN_POI_DISPLAY_ZOOM_LEVEL", I think marker itself should not worry about this since it's already managed in POIRenderer/POIMapSource classes. Then, if I am right and you don't need that "if", there is no need of addToLayer method.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +54,3 @@ + + this._mapView.poiLayer = new Champlain.MarkerLayer(); + this._mapView.poiLayer.set_selection_mode(Champlain.SelectionMode.MULTIPLE); Yeah, sorry, that was a bug in my patches, is solved in the latest versions, so, use SINGLE instead. @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack Maybe wrapping data in some sort of object?
Review of attachment 282177 [details] [review]: ::: src/poiMarker.js @@ +59,3 @@ + layer.add_marker(this); + } + }, Ohh, I actually need that "If" statement because I am rendering on POILayer not the tile, lso renderer has no information about it. If I remove the "if" statement when the POIRenderer is rendering the POIs and I pan away to a different zoom level, it will still add the marker to the layer. So, all the POIs get cramped up, and that looks ugly. That "if" statement prevents it from happening.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack I tried Wrapping in GObject.Object, Glib.ByteArray but nothing works. Basically the "render-complete" emit signal wants its 1st parameter to be of type G_TYPE_GPOINTER but I think, that gjs is unable to convert string to gpointer type. Thats why the error "Gjs-WARNING **: JS ERROR: Error: Cannot convert non-null JS value to G_POINTER". Is it a bug in gjs?, I don't know. Then I tried changing this line in champlain https://github.com/GNOME/libchamplain/blob/master/champlain/champlain-tile.c#L373 to "G_TYPE_STRING, G_TYPE_UINT, G_TYPE_BOOLEAN);" and it doesn't throw an error, but doesn't work properly because the tile image data is not a string, so it throws "pixbuf errors". It may be a libchamplain bug also. Is there any GENERIC data type in GObject ?? or can I typecast data in gjs itself and convert it to G_TYPE_POINTER??
Review of attachment 282172 [details] [review]: Please use git diff --check on all your changes. Saw now that this adds whitespace errors. ::: src/geoMath.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Rishi Raj Singh Jhelumi <rishiraj.devel@gmail.com> Whitespace damage above.
Review of attachment 282176 [details] [review]: ::: src/overpass.js @@ +112,3 @@ + return; + } + Whitespace damage.
Review of attachment 282177 [details] [review]: ::: src/poiBubble.js @@ +49,3 @@ + this.add(ui.box); + } + Remove this line ::: src/poiMarker.js @@ +71,3 @@ + }); + } + Remove this.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ Well we need to find out. I do not really want to commit this with a hack like this to solve an un-explained problem :) Maybe you could add a bugzilla entry on libchamplain and explain the problem there. Maybe Jiri can help debug it. Also if you could try writing a small example that reproduce it that would be nice. @@ +52,3 @@ + + let view = this._mapView.view; + trailing whitespace @@ +53,3 @@ + let view = this._mapView.view; + + this._mapView.poiLayer = new Champlain.MarkerLayer(); We create all other layers in mapView, move it there. @@ +83,3 @@ + tile.set_state(Champlain.State.LOADING); + + /** Trailing whitespace. @@ +85,3 @@ + /** + * The tile has no data so, + * its a rendering error. Whitespace! Get into the habit of never sending something out for review before you have checked for whitespace damage. Also, does this really need to be a multi-line comment? @@ +94,3 @@ + /** + * The tile is already rendered and the data exists on the MarkerLayer so, + * its not a rendering error. Here as well. @@ +100,3 @@ + tile.emit('render-complete', null, 0, false); + return; + } Why do you need this isRendered stuff? Isn't this what we have the cache for? @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack Yeah it seems the caches do not use the data argument of the rendered callback. @@ +122,3 @@ + tile.emit('render-complete', null, this.data.length, false); + } + Remove this.
I get a lot of: Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null And: (gnome-maps:7258): Gjs-WARNING **: JS ERROR: Error: Failed to convert UTF-8 string to JS string: Invalid byte sequence in conversion input start@resource:///org/gnome/maps/main.js:28 @<main>:1 Have you seen this?
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +27,3 @@ +const Utils = imports.utils; + +const _UNKNOWN = 'Unknown'; Remove this.
Jonas, I have did the changes you have mentioned. I am waiting for Damian's patches from bug #722871 to be based on master as it doesn't apply to me.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ I will file a bug. @@ +85,3 @@ + /** + * The tile has no data so, + * its a rendering error. Sorry, I used to do git diff --check after I have made a commit, thats why it didn't show any whitespace damage to me. Will remove all whitespace damamge. @@ +100,3 @@ + tile.emit('render-complete', null, 0, false); + return; + } It is because the renderer renders the POIs on the MarkerLayer, not the tile. Hence it doesn't have no knowledge of which tiles have been rendered. See these images, you will feel the difference. http://postimg.org/gallery/1i9o31qim/817eb01a/ You see, on a particular level, when a user pans away and comes back POIRenderer renders the POIs on MARKER LAYER again on top of existing poi's, because it has knowledge about its own tiles, not the Layer. @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack I will file a bug for this in champlain, if it is not a champlain bug, then it probably is a gjs bug.
(In reply to comment #78) > I get a lot of: > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null I get this one on the master branch sometimes. Not with the POIs. > > And: > (gnome-maps:7258): Gjs-WARNING **: JS ERROR: Error: Failed to convert UTF-8 > string to JS string: Invalid byte sequence in conversion input > start@resource:///org/gnome/maps/main.js:28 > @<main>:1 > Yeah, I get this one. I have no idea from where it originates. Possibly the conversion of POI json to array. > Have you seen this?
(In reply to comment #78) > I get a lot of: > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null This is because, I install the maki icons to the same location as geocode-glib. Now, when I checkout some other branch and run "jhbuild make", it deletes these icons, hence geocode-glib isn't also able to access these icons, as these icons have been deleted. > > And: > (gnome-maps:7258): Gjs-WARNING **: JS ERROR: Error: Failed to convert UTF-8 > string to JS string: Invalid byte sequence in conversion input > start@resource:///org/gnome/maps/main.js:28 > @<main>:1 > > Have you seen this?
(In reply to comment #83) > (In reply to comment #78) > > I get a lot of: > > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null > > This is because, I install the maki icons to the same location as geocode-glib. > Now, when I checkout some other branch and run "jhbuild make", it deletes these > icons, hence geocode-glib isn't also able to access these icons, as these icons > have been deleted. So, if you run build geocode-glib again, it doesn't throw this message. So, to solve the conflict should I change the install location of the POIs ?? Current location "$(DESTDIR)$(datadir)/icons/gnome/scalable/places/poi-XXX.svg" To "$(DESTDIR)$(datadir)/icons/gnome/scalable/places/pois/poi-XXX.svg" or any other ?? Current location is "/usr/share/icons/gnome/scalable/places/poi-XXX.svg" > > > > > And: > > (gnome-maps:7258): Gjs-WARNING **: JS ERROR: Error: Failed to convert UTF-8 > > string to JS string: Invalid byte sequence in conversion input > > start@resource:///org/gnome/maps/main.js:28 > > @<main>:1 > > > > Have you seen this?
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +118,3 @@ + }).bind(this)); + + tile.data = this.data; // Hack Filed as bug #734628.
Review of attachment 282181 [details] [review]: ::: src/poiRenderer.js @@ +33,3 @@ + * The data that is cached using champlain in file system adds some extra characters at the end + * of the JSON String. The method removes these charachters, so that the JSON is a list of POIs. + */ Well, reproduction of this would be difficult as only some of the cached strings append extra characters to the end, most don't. I could cache random strings and check whether the cached strings have extra characters appended. Filed as bug #734629 .
(In reply to comment #84) > (In reply to comment #83) > > (In reply to comment #78) > > > I get a lot of: > > > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null > > > > This is because, I install the maki icons to the same location as geocode-glib. > > Now, when I checkout some other branch and run "jhbuild make", it deletes these > > icons, hence geocode-glib isn't also able to access these icons, as these icons > > have been deleted. > > So, if you run build geocode-glib again, it doesn't throw this message. > So, to solve the conflict should I change the install location of the POIs ?? > > Current location > "$(DESTDIR)$(datadir)/icons/gnome/scalable/places/poi-XXX.svg" > To > "$(DESTDIR)$(datadir)/icons/gnome/scalable/places/pois/poi-XXX.svg" or any > other ?? > > Current location is > "/usr/share/icons/gnome/scalable/places/poi-XXX.svg" You should file a new bug on gnome-maps for the adding of Icons. That can go in after the icons have been removed from geocode-glib. Maybe you could ask on #gnome-hackers where to install the icons properly. > > > > > > > > > > > And: > > > (gnome-maps:7258): Gjs-WARNING **: JS ERROR: Error: Failed to convert UTF-8 > > > string to JS string: Invalid byte sequence in conversion input > > > start@resource:///org/gnome/maps/main.js:28 > > > @<main>:1 > > > > > > Have you seen this?
Review of attachment 282182 [details] [review]: ::: src/mapView.js @@ +117,3 @@ let source = this._factory.create_cached_source(mapType); this.view.set_map_source(source); + this.view.add_overlay_source(this._poiSource, 255); once the source is reset, I add the existing poiOverlay on top of it again, so no need to reset the poiSource.
Created attachment 283613 [details] [review] Add math module.
Created attachment 283614 [details] [review] Add base maki icons.
Created attachment 283615 [details] [review] Add POI place module.
Created attachment 283616 [details] [review] Add overpass wrapper.
Created attachment 283617 [details] [review] Add unicode conversion methods.
Created attachment 283618 [details] [review] Add POI overlay, renderer, marker and bubble modules.
Created attachment 283619 [details] [review] Use poiMapSource to overlay POIs.
Review of attachment 283614 [details] [review]: This has a separate bug now, right? bug #734722 So the patch should move there.
Review of attachment 283616 [details] [review]: Thanks. Can none of the functions in the HTTP.js module be of help in this code? For constructing the query. ::: src/overpass.js @@ +28,3 @@ + +const _DEFAULT_TIMEOUT = 180; +const _DEFAULT_MAXSIZE = 536870912; Where does these numbers come from? @@ +85,3 @@ + // HTTP Session Variables + this._session = new Soup.Session(); + Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault()); Is this needed? Why? Isn't it default? @@ +116,3 @@ + return Format.vprintf('%s?data=%s', [ BASE_URL, + this._generateOverpassQuery(bbox) ]); + }, Is this meant to be public? In that case, why?
Review of attachment 283618 [details] [review]: So, hmm. How about doing what Jiri suggested instead? Instead of using the overlay approach we inherit from Champlain MarkerLayer and on the appropriate zoom levels request from overpass with the view bounding box as bounding box. And then just add that custom layer to the mapView. We would have to write a custom caching thingie. But that could come after as well. ::: src/poi-bubble.ui @@ +3,3 @@ +<interface> + <requires lib="gtk+" version="3.12"/> + <object class="GtkBox" id="box"> Grid. Below as well. ::: src/poiBubble.js @@ +46,3 @@ + this.add(ui.box); + } +}); Isn't this code the exact same as the searchResult bubble? Can something be done about that?
Review of attachment 283616 [details] [review]: ::: src/overpass.js @@ +28,3 @@ + +const _DEFAULT_TIMEOUT = 180; +const _DEFAULT_MAXSIZE = 536870912; All the default values are given here: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL @@ +85,3 @@ + // HTTP Session Variables + this._session = new Soup.Session(); + Soup.Session.prototype.add_feature.call(this._session, new Soup.ProxyResolverDefault()); No, its not. I am behind a proxy so, checked it :). @@ +116,3 @@ + return Format.vprintf('%s?data=%s', [ BASE_URL, + this._generateOverpassQuery(bbox) ]); + }, When I wrote the wrapper, I thought someone may want just the url :P, and query sometime later in future. but I don't see that happening. I will change it to private.
(In reply to comment #97) > Review of attachment 283616 [details] [review]: > > Thanks. > > Can none of the functions in the HTTP.js module be of help in this code? For > constructing the query. There is only one parameter (data) that needs to be sent to the server. Can be used for this only: let query = new HTTP.Query({'data': this._generateOverpassQuery(bbox)}); query.toString(); Should I use, or keep it as is?? > > ::: src/overpass.js > @@ +28,3 @@ > + > +const _DEFAULT_TIMEOUT = 180; > +const _DEFAULT_MAXSIZE = 536870912; > > Where does these numbers come from? > > @@ +85,3 @@ > + // HTTP Session Variables > + this._session = new Soup.Session(); > + Soup.Session.prototype.add_feature.call(this._session, new > Soup.ProxyResolverDefault()); > > Is this needed? Why? Isn't it default? > > @@ +116,3 @@ > + return Format.vprintf('%s?data=%s', [ BASE_URL, > + > this._generateOverpassQuery(bbox) ]); > + }, > > Is this meant to be public? In that case, why?
(In reply to comment #96) > Review of attachment 283614 [details] [review]: > > This has a separate bug now, right? bug #734722 > > So the patch should move there. ohh, yes :).
(In reply to comment #98) > Review of attachment 283618 [details] [review]: > > So, hmm. > > How about doing what Jiri suggested instead? Instead of using the overlay > approach we inherit from Champlain MarkerLayer and on the appropriate zoom > levels request from overpass with the view bounding box as bounding box. And > then just add that custom layer to the mapView. A quick solution for the above using MarkerLayer, without caching. https://github.com/rishirajsinghjhelumi/GNOME-Maps/tree/poi-marker-layer-test > > We would have to write a custom caching thingie. But that could come after as > well. About this. Some Approaches : 1. This could be done using the same mechanism as is used by libchamplain. i.e, to store the POIs as a tile. We have a bounding box of the view. Since, the bounding box of the view is bigger than tile, it must have many tiles in it. So, We find all the tiles within this view(bounding box). And for each tile save the corresponding JSON of POIs in the database ( pois can be clustered into tiles easily :). ). These tiles will be saved in database using Slippy Map Tilenames. http://wiki.openstreetmap.org/wiki/Slippy_map_tilenames While during loading, we again have a bounding box of the view. Again, we find all the tiles within this view and if the tile has been cached, we use its data, else we query for it. I will have to implement both in-memory and file-caching for the Maps. No part of the above will use ChamplainRenderer, Champain{Tile/File/Memory}Cache or ChamplainMapSource. Suggestions ?? 2. We keep on using overlay source, and write our own {File/Memory} cache and renderer modules, independent of Champlain. 3. Make Champlain more introspectable :) (At least the 'render-complete' signal). But Jiri said that it would require API changes and won't happen in 0.12.x; Thoughts ??
(In reply to comment #102) > (In reply to comment #98) > > Review of attachment 283618 [details] [review] [details]: > > > > So, hmm. > > > > How about doing what Jiri suggested instead? Instead of using the overlay > > approach we inherit from Champlain MarkerLayer and on the appropriate zoom > > levels request from overpass with the view bounding box as bounding box. And > > then just add that custom layer to the mapView. > > A quick solution for the above using MarkerLayer, without caching. > https://github.com/rishirajsinghjhelumi/GNOME-Maps/tree/poi-marker-layer-test > Cool, looks simpler :) > > > > > We would have to write a custom caching thingie. But that could come after as > > well. > > About this. > Some Approaches : > > 1. > > This could be done using the same mechanism as is used by libchamplain. > i.e, to store the POIs as a tile. > > We have a bounding box of the view. > Since, the bounding box of the view is bigger than tile, it must have many > tiles in it. > So, We find all the tiles within this view(bounding box). > And for each tile save the corresponding JSON of POIs in the database ( pois > can be clustered into tiles easily :). ). > These tiles will be saved in database using Slippy Map Tilenames. > http://wiki.openstreetmap.org/wiki/Slippy_map_tilenames > Sure that sounds like a valid approach, would like Mattias to weigh in tho. But, I think caching could be a separate bug. I want to start merging stuff :) So lets first focus on getting the functionality in, usinga a customer layer. Then add caching in a later bug. Or at least in a separate patch. > > While during loading, we again have a bounding box of the view. > Again, we find all the tiles within this view and if the tile has been cached, > we use its data, else we query for it. > > I will have to implement both in-memory and file-caching for the Maps. > No part of the above will use ChamplainRenderer, > Champain{Tile/File/Memory}Cache or ChamplainMapSource. > > Suggestions ?? Well the custom caching module could cache to memory during operations and save to file periodically or at exit. I guess. Or just always save to file but also keep the state in memory so when you query the cache it always looks at memory. Maybe. Dunno. > > 2. > > We keep on using overlay source, and write our own {File/Memory} cache and > renderer modules, independent of Champlain. I think we want to move towards inteligent layers for other stuff (routing, markers, etc...) so I think the layer approach is better. > > 3. > > Make Champlain more introspectable :) (At least the 'render-complete' signal). > But Jiri said that it would require API changes and won't happen in 0.12.x; > Yeah that might be something that Champlain want as well, feel free to work on this, but maybe after this? :) > > Thoughts ?? Great job!
Please rebase on latest master and latest Damian work before posting patches again!
Created attachment 284008 [details] [review] Add math module.
Created attachment 284009 [details] [review] Add POI place module.
Created attachment 284010 [details] [review] Add overpass wrapper.
Created attachment 284011 [details] [review] Add poi Marker and bubble modules
Created attachment 284012 [details] [review] Add POI Marker Layer
Created attachment 284013 [details] [review] Use poiMarkerLayer to overlay POIs
Review of attachment 284009 [details] [review]: I would rather have a general place.js module that could handle this. I would be needed to get icons for our search results as well I guess if geocode-glib removes the icon property from place.
Review of attachment 284010 [details] [review]: ::: src/overpass.js @@ +55,3 @@ + + return poi; +} If we had a place.js couldn't that module have a static newFromOverpass?
Review of attachment 284011 [details] [review]: ::: src/poiBubble.js @@ +42,3 @@ + ui.image.pixbuf = pixbuf; + }); + ui.labelTitle.label = place.name; Name is the only interesting thing we want to show? We do not have access to other stuff? Like wiki?
Review of attachment 284012 [details] [review]: ::: src/poiMarkerLayer.js @@ +41,3 @@ + let view = this._mapView.view; + view.connect('notify::latitude', this._onViewMoved.bind(this)); + view.connect('notify::longitude', this._onViewMoved.bind(this)); what about noify::size? and notify::zoom-level?
Review of attachment 284011 [details] [review]: ::: src/poiMarker.js @@ +55,3 @@ + layer.add_marker(this); + } + }, I do not think this method is needed.
(In reply to comment #111) > Review of attachment 284009 [details] [review]: > > I would rather have a general place.js module that could handle this. I would > be needed to get icons for our search results as well I guess if geocode-glib > removes the icon property from place. Makes sense, will do.
Review of attachment 284010 [details] [review]: ::: src/overpass.js @@ +55,3 @@ + + return poi; +} yes, having it in place.js will be good.
Review of attachment 284011 [details] [review]: ::: src/poiBubble.js @@ +42,3 @@ + ui.image.pixbuf = pixbuf; + }); + ui.labelTitle.label = place.name; Yes, we do. We have contacts, wiki, website of some of the POIs available. Will include them in next patch. ::: src/poiMarker.js @@ +55,3 @@ + layer.add_marker(this); + } + }, ahh, not anymore :)
Review of attachment 284012 [details] [review]: ::: src/poiMarkerLayer.js @@ +41,3 @@ + let view = this._mapView.view; + view.connect('notify::latitude', this._onViewMoved.bind(this)); + view.connect('notify::longitude', this._onViewMoved.bind(this)); will have to try notify::size. No need fro notify::zoom-level i guess, because the corresponding lat, lon also change.
Review of attachment 284012 [details] [review]: ::: src/poiMarkerLayer.js @@ +41,3 @@ + let view = this._mapView.view; + view.connect('notify::latitude', this._onViewMoved.bind(this)); + view.connect('notify::longitude', this._onViewMoved.bind(this)); Btw, how do we prevent the onViewMoved being called twice because it is bound to both lat and lon? Will the overPass query be sent twice?
Created attachment 284154 [details] [review] Add math module.
Created attachment 284155 [details] [review] Add overpass wrapper.
Created attachment 284156 [details] [review] Add Place module.
Created attachment 284157 [details] [review] Add unicode conversion methods. I will be needing this as it will be used during file caching.
Created attachment 284158 [details] [review] Add poi Marker and bubble modules
Created attachment 284159 [details] [review] add TileMemoryCache Module
Created attachment 284160 [details] [review] Add POI Marker Layer
Created attachment 284161 [details] [review] Use poiMarkerLayer to overlay POIs
Review of attachment 284158 [details] [review]: ::: src/poiBubble.js @@ +53,3 @@ + continue; + content.push(tag + ' = ' + this.place.tags[tag]); + } So we just add all tags we have? I think I would prefer to add the properties we do want to keep in Place.js and then add them here if they exist. So that we can get links clickable and so forth. ::: src/poiMarker.js @@ +52,3 @@ + get anchor() { + return { x: Math.floor(this.width / 2), + y: this.height - 3 }; This is taken from the searchResultMarker, is this the best y for poi icons as well?
Review of attachment 284156 [details] [review]: Shouldn't we add properties for the types we can get from Overpass to this module? And maybe a method to create from a pure GeocodePlace so that we will get the correct icon then as well? What do you thik is the best way to convert the rest of the code to use this object? I guess we need to update the placeStore as well?
Review of attachment 284157 [details] [review]: What does these functions do that glib does not? For instance these: https://developer.gnome.org/glib/2.37/glib-Unicode-Manipulation.html
Review of attachment 284160 [details] [review]: ::: src/poiMarkerLayer.js @@ +64,3 @@ + + view.connect('notify::latitude', this._onViewMoved.bind(this)); + view.connect('notify::longitude', this._onViewMoved.bind(this)); Same question. Will this fire onViewMoved twice on every move? What will that mean? Two overPass queries for every pan? @@ +68,3 @@ + view.connect('notify::zoom-level', (function() { + this._renderedTiles = {}; + this.remove_all(); Is these needed if ware still within the correct zoom level to show pois? @@ +124,3 @@ + mapView: this._mapView }); + if (this._mapView.view.zoom_level >= MIN_POI_DISPLAY_ZOOM_LEVEL) + this.add_marker(poiMarker); Is this check needed? Could we get here on a different zoom level? @@ +125,3 @@ + if (this._mapView.view.zoom_level >= MIN_POI_DISPLAY_ZOOM_LEVEL) + this.add_marker(poiMarker); + }).bind(this)); Here you loop through the places twice. Once with the map and then again to create the poiMarker. Can't you just have place: Place.newFromOverpass(... @@ +140,3 @@ + let minY = Math.floor( source.get_y(zoom, bbox.top) / size ); + let maxX = Math.floor( source.get_x(zoom, bbox.right) / size ); + let maxY = Math.floor( source.get_y(zoom, bbox.bottom) / size ); No space around paranthesis. @@ +156,3 @@ + + _onViewMoved: function() { + Extra line. @@ +172,3 @@ + this._cacheTiles(tiles, tilesContent); + this._loadTiles(tiles); + }).bind(this)); Would it be faster/better to loop through the tiles and send a query per tile? Would the first results appear sooner?
Review of attachment 284161 [details] [review]: ::: src/mapView.js @@ +97,3 @@ let mode = Champlain.SelectionMode.SINGLE; + this._poiLayer = new POIMarkerLayer.POIMarkerLayer({ mapView: this, + selection_mode: mode }); indentation?
Review of attachment 284160 [details] [review]: Don't we need some kind of limitation of how many tiles to keep in memory? Can't this get out of hand from long sessions? We should probably have a limit and drop tiles if we reach it?
Review of attachment 284160 [details] [review]: ::: src/poiMarkerLayer.js @@ +148,3 @@ + y: y, + zoom_level: zoom, + size: size })); Nit: we do not actually use tile.zoom / tile.size in the code? Should we? @@ +181,3 @@ + _isRendered: function(tile) { + return ((tile.get_x() + '/' + tile.get_y()) in this._renderedTiles); + } Above: use tile.x / tile.y instead of get_x and get_y. Also, did you consider using the same encoding as in Champlain for the key? Does it matter performance wise?
Review of attachment 284155 [details] [review]: ::: src/overpass.js @@ +87,3 @@ + } + }).bind(this)); + }, Do we need a way of canceling a message? So that we can cancel something on onMoved etc?
Did not seem to work for me, I see no icons and I see a lot of: Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null Could it be proxy related?
Review of attachment 284160 [details] [review]: ::: src/poiMarkerLayer.js @@ +64,3 @@ + + view.connect('notify::latitude', this._onViewMoved.bind(this)); + view.connect('notify::longitude', this._onViewMoved.bind(this)); Yes. I am searching for a way to combine this signals and send a single signal if multiple things have changed. something like a 'view-moved' signal, which emits when the view is moved irrespective of what is changed. Now, I should be able to connect to a single signal. Maybe this signal should be in champlain.?? @@ +68,3 @@ + view.connect('notify::zoom-level', (function() { + this._renderedTiles = {}; + this.remove_all(); Yes, otherwise, there will be two pois on top of each other (one for each level). @@ +124,3 @@ + mapView: this._mapView }); + if (this._mapView.view.zoom_level >= MIN_POI_DISPLAY_ZOOM_LEVEL) + this.add_marker(poiMarker); The situation happens when the POIs are being loaded and the user zooms out. Will have to cancel the loading of POIs if zoom-level changed. @@ +125,3 @@ + if (this._mapView.view.zoom_level >= MIN_POI_DISPLAY_ZOOM_LEVEL) + this.add_marker(poiMarker); + }).bind(this)); yeah, the map can be removed. @@ +140,3 @@ + let minY = Math.floor( source.get_y(zoom, bbox.top) / size ); + let maxX = Math.floor( source.get_x(zoom, bbox.right) / size ); + let maxY = Math.floor( source.get_y(zoom, bbox.bottom) / size ); removed. @@ +148,3 @@ + y: y, + zoom_level: zoom, + size: size })); tile.zoom is required. see TileMemoryCache. @@ +156,3 @@ + + _onViewMoved: function() { + removed. @@ +172,3 @@ + this._cacheTiles(tiles, tilesContent); + this._loadTiles(tiles); + }).bind(this)); It will be a lot of queries to the server, but it will be a very safe bet. and will function exactly same as ChamplainTileSource. @@ +181,3 @@ + _isRendered: function(tile) { + return ((tile.get_x() + '/' + tile.get_y()) in this._renderedTiles); + } ahh, I could do that, it should be faster.
(In reply to comment #134) > Review of attachment 284160 [details] [review]: > > Don't we need some kind of limitation of how many tiles to keep in memory? > Can't this get out of hand from long sessions? > We should probably have a limit and drop tiles if we reach it? Yes, I will be including a Queue in TileMemoryCache and limit that Queue to the number passed while creating the cache.
(In reply to comment #137) > Did not seem to work for me, I see no icons and I see a lot of: > > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null > > > Could it be proxy related? You need to apply the 'maki icons patch' from bug #734722 as well :)
Review of attachment 284155 [details] [review]: ::: src/overpass.js @@ +87,3 @@ + } + }).bind(this)); + }, Yes we might. looking into it.
Review of attachment 284161 [details] [review]: ::: src/mapView.js @@ +97,3 @@ let mode = Champlain.SelectionMode.SINGLE; + this._poiLayer = new POIMarkerLayer.POIMarkerLayer({ mapView: this, + selection_mode: mode }); oops.
Review of attachment 284158 [details] [review]: ::: src/poiBubble.js @@ +53,3 @@ + continue; + content.push(tag + ' = ' + this.place.tags[tag]); + } Well, there are 51694 keys overall. http://taginfo.openstreetmap.org/api/4/keys/all?sortname=count_all&sortorder=desc (careful 13.5 MB of data :P). So which keys should we decide is again the question, as was which pois should we use. Here also we could do some statistics and choose the top x keys ?? ::: src/poiMarker.js @@ +52,3 @@ + get anchor() { + return { x: Math.floor(this.width / 2), + y: this.height - 3 }; seems like some sort of magic number.
Review of attachment 284158 [details] [review]: ::: src/poiBubble.js @@ +53,3 @@ + continue; + content.push(tag + ' = ' + this.place.tags[tag]); + } Well it will have to depend. On what UI we want. Say we want to support wiki-links, then we add a place in the bubble to show wiki, we make sure the Place class has a wiki property and we make sure that the newFromOverpass-method sets it on the object. Same for each property we want to show in the bubble. ::: src/poiMarker.js @@ +52,3 @@ + get anchor() { + return { x: Math.floor(this.width / 2), + y: this.height - 3 }; It is, it makes the popover place in a niceway in regards to the pin.svg icon. Is the same true for the maki icons? Or should a different value be used?
(In reply to comment #140) > (In reply to comment #137) > > Did not seem to work for me, I see no icons and I see a lot of: > > > > Gjs-Message: JS LOG: Failed to load pixbuf: TypeError: info is null > > > > > > Could it be proxy related? > > > You need to apply the 'maki icons patch' from bug #734722 as well :) That did it :) It looks good!
Review of attachment 284158 [details] [review]: ::: src/poiBubble.js @@ +53,3 @@ + continue; + content.push(tag + ' = ' + this.place.tags[tag]); + } We at least need a better way to format the data than right now: cryptic_tagname = value Do you agree?
> Review of attachment 284156 [details] [review]: > > Shouldn't we add properties for the types we can get from Overpass to this > module? what types ?? > > And maybe a method to create from a pure GeocodePlace so that we will get the > correct icon then as well? ahh, yes we should definitely, but if we re-factor the code to use Place class, this may not be required. > What do you thik is the best way to convert the rest of the code to use this > object? Since it inherits from GeocodePlace, all the properties of GeocodePlace are still there. We will need to re-factor code that creates a new GeocodePlace, uses place.icon or place.place_type properties or any other but mainly these. I will re-factor the code once it gets in. > I guess we need to update the placeStore as well? Yes, PlaceStore can make use of this object and save the place_type and tags also in the JSON, which will solve Damian's issue with searchResultBubble where it (Fallback for places coming from PlaceStore) since placeStore currently doesn't have tags. We can write a method to convert Place to PlaceJSON and store that and vice versa i.e, load JSON which I guess will be same as newFromOverpass.
(In reply to comment #146) > Review of attachment 284158 [details] [review]: > > ::: src/poiBubble.js > @@ +53,3 @@ > + continue; > + content.push(tag + ' = ' + this.place.tags[tag]); > + } > > We at least need a better way to format the data than right now: > > cryptic_tagname = value > > Do you agree? Yes, "addr:" and "gnis:" and similar could be combined and some other stuff as well. The 1st symbol could be capitalised etc. I will try to write a method for that.
(In reply to comment #148) > (In reply to comment #146) > > Review of attachment 284158 [details] [review] [details]: > > > > ::: src/poiBubble.js > > @@ +53,3 @@ > > + continue; > > + content.push(tag + ' = ' + this.place.tags[tag]); > > + } > > > > We at least need a better way to format the data than right now: > > > > cryptic_tagname = value > > > > Do you agree? > > Yes, "addr:" and "gnis:" and similar could be combined and some other stuff as > well. The 1st symbol could be capitalised etc. > I will try to write a method for that. We will still end up with a bunch of tags with cryptic names and non-pretty names. It would be better to catch the tags that we do want and make sure they are formatted in a nice way. Links should be clickable and send you to a browser and so forth. If you don't want to add the tags we want as properties to the Place module. Then at least write something that makes this looks like something else like a dump of the tags direct from the OSM database.
Review of attachment 284158 [details] [review]: ::: src/poiBubble.js @@ +53,3 @@ + continue; + content.push(tag + ' = ' + this.place.tags[tag]); + } I have implemented a function for this. Takes as input a (tag, value) and applies some magic on it :P. ::: src/poiMarker.js @@ +52,3 @@ + get anchor() { + return { x: Math.floor(this.width / 2), + y: this.height - 3 }; It should be: { x: Math.floor(this.width / 2), y: this.height }; -3 is not required.
Created attachment 284263 [details] [review] Add poi Marker and bubble modules
Created attachment 284264 [details] [review] Add POI Marker Layer
Created attachment 284265 [details] [review] Use poiMarkerLayer to overlay POIs
(In reply to comment #151) > Created an attachment (id=284263) [details] [review] > Add poi Marker and bubble modules Some Images on how it currently looks: http://postimg.org/gallery/33m9it1z2/8f97559b/
Review of attachment 284263 [details] [review]: ::: src/poiBubble.js @@ +73,3 @@ +} + +function prettifyOSMTag(tag, value) { This function's works as follows in-order: - Ignore Exact tags, then - Use Exact tags, then - Use Partial tags, then - Ignore Partial tags, then - Use as is. This is just a model and it can be improved at every iteration.
Review of attachment 284159 [details] [review]: So, I was looking into which library I can use for adding queue to tileMemoryCache to maintain first x tiles. GLib supports queues, but I am unable to use it in GJS. Even docs (https://people.gnome.org/~gcampagna/docs/) have no mention of GLib Queue. So, is there any other module or should I implement Queue class of my own ??
Review of attachment 284263 [details] [review]: So, I think still we should decide which tags we want to include and include them, with nice names and formatting. I do not want to pass the raw tag names to the ui. A white-list sort of.
(In reply to comment #157) > Review of attachment 284263 [details] [review]: > > So, I think still we should decide which tags we want to include and include > them, with nice names and formatting. > I do not want to pass the raw tag names to the ui. A white-list sort of. A white list would be something that could be created but it will be hard as there are a lot of tags (50k+) and even the tags that have a high count are sometimes useless to show to the user. If you look at the code, you will find that I am searching for a substring for a lot of cases as even the key names may be different but may point to the same context and contains relevant info. For example for wikipedia there are keys like wikipedia, wikipedia:en, wikipedia:in, bridge:wikipedia:de and many more. similarly for "website" contact:website, website, website2 etc which need to be used some how. And many keys having a namespace are actually worthless as they contain numbers relevant to some external source, which should be removed. thats why the ignoredCodes list. This was all based on some analysis I did on http://taginfo.openstreetmap.org/keys for keys having count > 100,000. The "using partial tags" are somewhere we can optimise so that we do not return the tag if none of the four conditions above satisfy. Again, this is just a model and this can be improved.
Review of attachment 284264 [details] [review]: ::: src/poiMarkerLayer.js @@ +69,3 @@ + this._renderedTiles = {}; + this.remove_all(); + this._onViewMoved(); On a zoom in we should never need to remove the markers and re-fetch them, right? Since no new markers could appear. And on zoom out we could theoretically be able to avoid fetching quite a few, right? But then again, maybe we could just limit pois to the max zoom level? @@ +79,3 @@ + for (let i = 0; i < bboxes.length; i++) { + tilesContent.push([]); + } This cannot be needed right? @@ +83,3 @@ + pois.forEach((function(poi) { + for (let i = 0; i < bboxes.length; i++) { + if (bboxes[i].covers(poi.lat, poi.lon)) { Just go if (!tilesContent[i]) tilesContent[i] = []; here?
(In reply to comment #158) > (In reply to comment #157) > > Review of attachment 284263 [details] [review] [details]: > > > > So, I think still we should decide which tags we want to include and include > > them, with nice names and formatting. > > I do not want to pass the raw tag names to the ui. A white-list sort of. > > A white list would be something that could be created but it will be hard as > there are a lot of tags (50k+) and even the tags that have a high count are > sometimes useless to show to the user. > > If you look at the code, you will find that I am searching for a substring for > a lot of cases as even the key names may be different but may point to the same > context and contains relevant info. > > For example > for wikipedia there are keys like > wikipedia, wikipedia:en, wikipedia:in, bridge:wikipedia:de and many more. > similarly for "website" > contact:website, website, website2 etc > which need to be used some how. > > And many keys having a namespace are actually worthless as they contain numbers > relevant to some external source, which should be removed. thats why the > ignoredCodes list. > > This was all based on some analysis I did on > http://taginfo.openstreetmap.org/keys for keys having count > 100,000. > > The "using partial tags" are somewhere we can optimise so that we do not return > the tag if none of the four conditions above satisfy. > > Again, this is just a model and this can be improved. So how about selecting about 15 or 20 "pieces of information" that we are interested in. Like: opening hours, website, wiki, phone number, city, postal code, country, wheelchair etc... And include the most common tags for them. And have functions to format them. Is that to much work? And we can add more information we want later and which tags that we have to use for it.
(In reply to comment #160) > (In reply to comment #158) > > (In reply to comment #157) > > > Review of attachment 284263 [details] [review] [details] [details]: > > > > > > So, I think still we should decide which tags we want to include and include > > > them, with nice names and formatting. > > > I do not want to pass the raw tag names to the ui. A white-list sort of. > > > > A white list would be something that could be created but it will be hard as > > there are a lot of tags (50k+) and even the tags that have a high count are > > sometimes useless to show to the user. > > > > If you look at the code, you will find that I am searching for a substring for > > a lot of cases as even the key names may be different but may point to the same > > context and contains relevant info. > > > > For example > > for wikipedia there are keys like > > wikipedia, wikipedia:en, wikipedia:in, bridge:wikipedia:de and many more. > > similarly for "website" > > contact:website, website, website2 etc > > which need to be used some how. > > > > And many keys having a namespace are actually worthless as they contain numbers > > relevant to some external source, which should be removed. thats why the > > ignoredCodes list. > > > > This was all based on some analysis I did on > > http://taginfo.openstreetmap.org/keys for keys having count > 100,000. > > > > The "using partial tags" are somewhere we can optimise so that we do not return > > the tag if none of the four conditions above satisfy. > > > > Again, this is just a model and this can be improved. > > > So how about selecting about 15 or 20 "pieces of information" that we are > interested in. Like: > > opening hours, > website, > wiki, > phone number, > city, > postal code, > country, > wheelchair > > etc... > > And include the most common tags for them. And have functions to format them. > Is that to much work? And we can add more information we want later and which > tags that we have to use for it. no, its not much work, its just adding tags :). btw, these tags should be searched as a substring right ?? or just exact matches??
(In reply to comment #161) > (In reply to comment #160) > > (In reply to comment #158) > > > (In reply to comment #157) > > > > Review of attachment 284263 [details] [review] [details] [details] [details]: > > > > > > > > So, I think still we should decide which tags we want to include and include > > > > them, with nice names and formatting. > > > > I do not want to pass the raw tag names to the ui. A white-list sort of. > > > > > > A white list would be something that could be created but it will be hard as > > > there are a lot of tags (50k+) and even the tags that have a high count are > > > sometimes useless to show to the user. > > > > > > If you look at the code, you will find that I am searching for a substring for > > > a lot of cases as even the key names may be different but may point to the same > > > context and contains relevant info. > > > > > > For example > > > for wikipedia there are keys like > > > wikipedia, wikipedia:en, wikipedia:in, bridge:wikipedia:de and many more. > > > similarly for "website" > > > contact:website, website, website2 etc > > > which need to be used some how. > > > > > > And many keys having a namespace are actually worthless as they contain numbers > > > relevant to some external source, which should be removed. thats why the > > > ignoredCodes list. > > > > > > This was all based on some analysis I did on > > > http://taginfo.openstreetmap.org/keys for keys having count > 100,000. > > > > > > The "using partial tags" are somewhere we can optimise so that we do not return > > > the tag if none of the four conditions above satisfy. > > > > > > Again, this is just a model and this can be improved. > > > > > > So how about selecting about 15 or 20 "pieces of information" that we are > > interested in. Like: > > > > opening hours, > > website, > > wiki, > > phone number, > > city, > > postal code, > > country, > > wheelchair > > > > etc... > > > > And include the most common tags for them. And have functions to format them. > > Is that to much work? And we can add more information we want later and which > > tags that we have to use for it. > > no, its not much work, its just adding tags :). > btw, these tags should be searched as a substring right ?? or just exact > matches?? Just exact matches I think. Each piece of information we want to include could have the tags that it needs to have and a way to format itself.
Created attachment 285089 [details] [review] Add math module.
Created attachment 285090 [details] [review] Add overpass wrapper.
Created attachment 285091 [details] [review] Add Place module.
Created attachment 285092 [details] [review] Add Queue ADT.
Created attachment 285093 [details] [review] add TileMemoryCache Module
Created attachment 285094 [details] [review] Add SQLite DB Connection Module
Created attachment 285095 [details] [review] Use DB Source.
Created attachment 285096 [details] [review] Add TileDBCache Module.
Created attachment 285097 [details] [review] Add poi Marker and bubble modules
Created attachment 285098 [details] [review] Add POI Marker Layer
Created attachment 285099 [details] [review] Use poiMarkerLayer to overlay POIs
Review of attachment 285098 [details] [review]: Cool. ::: src/poiMarkerLayer.js @@ +116,3 @@ + this._setRendered(tile); + return; + } Couldn't the memory cache be called just cache? And try to load from the db cache if not in memory? To avoid having to deal with two caches here and below? @@ +122,3 @@ + this._displayTile(JSON.parse(unescape(this._DBCache.get(tile)))); + this._setRendered(tile); + return; Shouldn't the dbcache add this to the memory cache?
Review of attachment 285089 [details] [review]: Are all these used? ::: src/geoMath.js @@ +48,3 @@ + let n = (1 << zoom) * 1.0; + let latRad = latitude * Math.PI / 180.0; + return Math.floor( ( 1 - Math.log( Math.tan(latRad) + Math.sec(latRad) ) / Math.PI ) * 0.5 * n); Some un-needed spaces here.
Review of attachment 285091 [details] [review]: How would this work with the places that we receive from GeocodeGlib? How would we convert them? We do not have the tags.
Review of attachment 285092 [details] [review]: Nifty. And there is nothing like this in Glib? ::: src/queue.js @@ +25,3 @@ + const Queue = new Lang.Class({ + Name: 'Queue', + Cool, can this be used in placeStore as well? @@ +31,3 @@ + }, + + enQueue: function(data) { just enqueue @@ +38,3 @@ + }, + + deQueue: function() { just dequeue @@ +41,3 @@ + if (this.size() === 0) { + throw "Empty Queue!!!"; + } Do we really need to throw an exception here? Is it really something that we expect to never happen? Can't we just return the item we dequeue and null if empty? @@ +56,3 @@ + if (this.size() === 0) { + throw "Empty Queue!!!"; + } return null if empty.
Review of attachment 285093 [details] [review]: Maybe just PoiCache and deals with the db cache as well? Tries to load from db if not in memory? And if in db add to memory.
Review of attachment 285094 [details] [review]: Do we really need sqlite? Champlain just used files, right? And if we do we need to add this as a dependency to Maps, in configure and jhbuild. ::: src/db.js @@ +27,3 @@ + +const DB_NAME = 'gnome_maps'; +const DB_LOCATION = GLib.get_user_cache_dir() +'/gnome-maps/'; GLib.build_filenamev([GLib.get_user_cache_dir(), 'gnome-maps'])
Review of attachment 285097 [details] [review]: Needs to be translatable. ::: src/poiBubble.js @@ +39,3 @@ +const displayTags = { + 'postal_code':{ + tags: new Set([ Why set? @@ +129,3 @@ + } + } +}; All these need to be translatable. So you need to use the gettext module to wrap these in _(""). Also I wonder, why not add these as properties on the place module? So that they can be used by other modules than the poiBubble?
This is a cool feature, hope we can get it in soon!
Review of attachment 285098 [details] [review]: ::: src/poiMarkerLayer.js @@ +116,3 @@ + this._setRendered(tile); + return; + } yup. will do. @@ +122,3 @@ + this._displayTile(JSON.parse(unescape(this._DBCache.get(tile)))); + this._setRendered(tile); + return; Yes, it should. missed it :P.
Review of attachment 285089 [details] [review]: Not all, but may be required later. ::: src/geoMath.js @@ +48,3 @@ + let n = (1 << zoom) * 1.0; + let latRad = latitude * Math.PI / 180.0; + return Math.floor( ( 1 - Math.log( Math.tan(latRad) + Math.sec(latRad) ) / Math.PI ) * 0.5 * n); will remove.
Review of attachment 285091 [details] [review]: For the tags we could pass an empty dict. I could write a method like placeFromGeocode in this module that can handle existing GeocodePlace ??
Review of attachment 285092 [details] [review]: There are DEQueues, but they are not GObject introspectable, at least not for GJS. I see python and vala references for them. ::: src/queue.js @@ +31,3 @@ + }, + + enQueue: function(data) { ok. @@ +38,3 @@ + }, + + deQueue: function() { ok. @@ +41,3 @@ + if (this.size() === 0) { + throw "Empty Queue!!!"; + } null would be a better option. @@ +56,3 @@ + if (this.size() === 0) { + throw "Empty Queue!!!"; + } yes.
Review of attachment 285093 [details] [review]: Something like map source chaining in libchamplain, right ? Will try to come up with something in next revision.
Review of attachment 285094 [details] [review]: Champlain uses sqlite. It has a table named "tiles" with these fields (filename, etag, popularity, size); I am using (tile, data, timestamp) where tile will be similar to filename in the above table "x/y/zoom" and the corresponding data. Yes, i will add it to configure. ::: src/db.js @@ +27,3 @@ + +const DB_NAME = 'gnome_maps'; +const DB_LOCATION = GLib.get_user_cache_dir() +'/gnome-maps/'; ok.
Review of attachment 285097 [details] [review]: ::: src/poiBubble.js @@ +39,3 @@ +const displayTags = { + 'postal_code':{ + tags: new Set([ I needed just keys, and lookup of set is faster than list. Maybe it doesn't matter here much. Just a data structure choice, nothing else. :) @@ +129,3 @@ + } + } +}; ahh yes. Yes, adding in Place module would be better I guess.
Thanks for reviewing :). Will try to come up with updated patches soon.
Review of attachment 285094 [details] [review]: How do we add packages in configure.ac ? I know somewhere here" PKG_CHECK_MODULES(GNOME_MAPS, [ gio-2.0 >= $GIO_MIN_VERSION gjs-1.0 >= $GJS_MIN_VERSION gobject-introspection-1.0 >= $GOBJECT_INTROSPECTION_MIN_VERSION ])" but whats the correct way? How do we find out what's the min version required ? (go with the latest one) and whom should I contact if I want to add dependency in jhbuild? I have no experience in this, sorry :( .
Almost all refactoring is done, I am just stuck with the above.
Review of attachment 285089 [details] [review]: Remove the functions not used, we don't want to carry around code no-one uses.
Review of attachment 285091 [details] [review]: Hmmm, maybe, needs more thought. We will need a uniform way to get image from place if and when we move the images to Maps. We will never now the tags in GeocodePlace from GeocodeGlib so that needs a function to get the image from the original place_type.
Review of attachment 285092 [details] [review]: Yeah, ok so an alternative to this would be to try to make the DEQueue from Glib into a BOXED type so that we could use it here. If you feel like trying to get that into glib that would be an option :) Look at how other boxed types are made, starting with in glib/gobject/gnoxerd.c, in that case.
Review of attachment 285096 [details] [review]: Does this has a limit on how much it stores? Should we prune it sometimes? Will we end up with everything in cache if one uses this long enough? Is there a mechanism for updating stuff in cache so we do not end up with stale data?
Review of attachment 285094 [details] [review]: I think you could add a bug on the jhbuild module and ask to add the dependency (https://bugzilla.gnome.org/enter_bug.cgi?product=jhbuild) But don't do this until we have agreed here to use sqlite. You need to find the minimum version of the library that this will still work with, I guess. Maybe what is included in the version of ubuntu that has GNOME 3.10? Or a tactic like that.
Hi! So some thoughts now that Maps have changed a bit: * We now have overpass data on the Place.js, we should use that object and the placeMarker/placeBubble modules to show the POI data. Maybe we can start with just the information/tags we currently have on the Place.js for now? We want designers to redesign the bubbles before adding more at least. * You can check the Makefile in data/icons on how to install icons in Maps theme. And we can install the icons we feel are needed there. And if we can find a nice way to get our own icons for Places we can migrate from the geocode-glib icons. But remember that the GeocodePlaces we get from geocode does not have any tags, so we need to take the GeocodePlace::PlaceType into consideration as well. * We can add an optional icon property to the Place module so the placeBubble knows what icon to use. * Caching, I dunno, I don't think I want an sql dependency. Can we do it simpler in some way? Maybe draw inspiration from how GeocodeGlib does it? Maybe we can start with a version with no caching at all? To get a feel for this?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-maps/issues/7.