GNOME Bugzilla – Bug 757144
Add support for mapbox simplestyle for GeoJSON
Last modified: 2018-03-26 12:59:06 UTC
As specified here: https://github.com/mapbox/simplestyle-spec/ Should be investiged if we can do it in geoJSONSource.js or if we have to get into the geojson-vt/ code from mapbox. Maybe we can just look in the tags object?
I looked into it, I think we can accommodate title,description,stroke,stroke-width,stroke-opacity,fill and fill-opacity in geoJSONSource.js inside _renderTile function. Marker-size,symbol and color will be a problem because we use UI-images.
(In reply to Alaf from comment #1) > I looked into it, I think we can accommodate > title,description,stroke,stroke-width,stroke-opacity,fill and fill-opacity > in geoJSONSource.js inside _renderTile function. > Marker-size,symbol and color will be a problem because we use UI-images. Agreed. And all the properties will end up in feature.tags: for (key in feature.tags) { log('tag[' + key + '] = ' + feature.tags[key]); } We should be able to do this nicely and map the style to cairo. A question is we should do this abstract enough to accommodate other styles?
We should support all styles that comes under intersection of "basic svg styles" and styles supported by cairo. basic svg style: http://www.w3.org/TR/SVG/styling.html How will we display description on hover or click?
(In reply to Alaf from comment #3) > We should support all styles that comes under intersection of "basic svg > styles" and styles supported by cairo. > Why do you think so? We will not create our own GeoJSON files so we should support the styles that are common in GeoJSON I feel. Do you have a link that states that the basic SVG styles are used with GeoJSON? My limited Googling has only shown the Mapbox simple style. I do not want to add un-needed complexity. Starting with the Mapbox simple style seems enough. And if someone requests more or if we find out some other style is used a lot then that can be the next. > basic svg style: > http://www.w3.org/TR/SVG/styling.html > > How will we display description on hover or click?
Okay, that seems a good strategy.
Created attachment 316789 [details] [review] Add support for mapbox simplestyle for GeoJSON
sample GeoJson for testing https://gist.github.com/Alafazam/6a4aeb7a3c345dd44521
Review of attachment 316789 [details] [review]: Thanks! Looks great! But I do not want to change to much in the geojson-vt library, I want to treat it as more of a blackbox. Can't we operate just on the tags in renderTile?
Created attachment 316809 [details] [review] Add support for mapbox simplestyle for GeoJSON
Review of attachment 316809 [details] [review]: Thanks! I have some comments on this, but it is a nice start! I think we should treat the style better. Maybe add a new class, geoJSONStyle.js? And we can have mapboxSimpleStyle as a subclass maybe? I am not sure. What do you think? But I want to aboid having this file grow for each new style we add. The style object can keep stuff like stroke-width, stroke color, fill color and stroke/fill opacity. To start with? So this file can just instatiate: let simpleStyle = new MapboxSimpleStyle.MapboxSimpleStyle({ tags: tags }); And later set the color/stroke-width/opacity from the style? Maybe you can ask Andreas in #gnome-maps how he feels description should be shown? Thanks! ::: src/geoJSONSource.js @@ +221,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i); + + // handlign #abc color format Maybe we can just detect if the first char is '#' or not? And send whole string or the substring to the parsing function? @@ +248,3 @@ + return [0,0,0]; + } + }, Maybe this can return a { red: , green:, blue: } object?
(In reply to Jonas Danielsson from comment #10) > Review of attachment 316809 [details] [review] [review]: > > Thanks! > > I have some comments on this, but it is a nice start! > I think we should treat the style better. Maybe add a new class, > geoJSONStyle.js? And we can have mapboxSimpleStyle as a subclass maybe? > I am not sure. What do you think? But I want to aboid having this file grow > for each new style we add. > > The style object can keep stuff like stroke-width, stroke color, fill color > and stroke/fill opacity. To start with? > So this file can just instatiate: > > let simpleStyle = new MapboxSimpleStyle.MapboxSimpleStyle({ tags: tags }); > > And later set the color/stroke-width/opacity from the style? > > Maybe you can ask Andreas in #gnome-maps how he feels description should be > shown? > > Thanks! > > ::: src/geoJSONSource.js > @@ +221,3 @@ > + m = colorString.match(/([0-9a-f]{3})$/i); > + > + // handlign #abc color format > > Maybe we can just detect if the first char is '#' or not? > And send whole string or the substring to the parsing function? > > @@ +248,3 @@ > + return [0,0,0]; > + } > + }, > > Maybe this can return a { red: , green:, blue: } object? Or maybe just a file geoJSONStyle.js that also have static functions? Like parseSimpleStyle that returns NULL or a geoJSONStyle class with the properties needed? color, opacity, width, fillColor, fillOpacity etc
Thanks for the review. Should I start with a new file geoJSONStyle.js, which will have a class named geoJSONStyle. This class will have all the methods required to parse styles and assign them properties of GeoJSONStyle object. And we can do somthing like -: let simpleStyle = new GeoJSONStyle({ tags: tags });
Review of attachment 316809 [details] [review]: Yeah sure, I will ask Andreas about how should we display description and title for geoJSON. ::: src/geoJSONSource.js @@ +221,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i); + + // handlign #abc color format Actually style support 4 kinds of color values in #abc, abc, #abcdef and abcdef format. I used different match for each of them so as to also validate them. I found them here http://stackoverflow.com/questions/11068240/what-is-the-most-efficient-way-to-parse-a-css-color-in-javascript @@ +248,3 @@ + return [0,0,0]; + } + m = colorString.match(/^#([0-9a-f]{6})$/i)[1]; yes, you are right, that would be better. okay i.ll do it.
Review of attachment 316809 [details] [review]: ::: src/geoJSONSource.js @@ +221,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i); + + // handlign #abc color format Yes, I see that. But parsing #abc and abc to color is pretty much the same, right? If you remove the '#' so my thinking was a have something like: if (string.startsWith('#')) color = _parseHex(string.splice(1)); else color = _parseHex(string); @@ +248,3 @@ + return [0,0,0]; + } + }, Maybe this can return a { red: , green:, blue: } object?
(In reply to Alaf from comment #12) > Thanks for the review. > Should I start with a new file geoJSONStyle.js, which will have a class > named geoJSONStyle. > This class will have all the methods required to parse styles and assign > them properties of GeoJSONStyle object. > > And we can do somthing like -: > > let simpleStyle = new GeoJSONStyle({ tags: tags }); I think so, but better would be to have a geoJSONStyle class/file that is a class that contains data. And have static methods in that file geoJSOnStyle.js that constructs the object for different styles. Like: let simpleStyle = GeoJSONStyle.parseSimpleStyle(tags); if (simpleStyle) { ... } Also, remember cr.setLineWidth() ...
Review of attachment 316809 [details] [review]: ::: src/geoJSONSource.js @@ +221,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i); + + // handlign #abc color format okay now I get it. @@ +248,3 @@ + return [0,0,0]; + } + m = colorString.match(/^#([0-9a-f]{6})$/i)[1]; yeah okay.
sorry to write it here. In reply to. > @@ +248,3 @@ > + return [0,0,0]; > + } > + }, > > Maybe this can return a { red: , green:, blue: } object? Yeah okay.
(In reply to Jonas Danielsson from comment #15) > (In reply to Alaf from comment #12) > > Thanks for the review. > > Should I start with a new file geoJSONStyle.js, which will have a class > > named geoJSONStyle. > > This class will have all the methods required to parse styles and assign > > them properties of GeoJSONStyle object. > > > > And we can do somthing like -: > > > > let simpleStyle = new GeoJSONStyle({ tags: tags }); > > I think so, but better would be to have a geoJSONStyle class/file that is a > class that contains data. And have static methods in that file > geoJSOnStyle.js that constructs the object for different styles. > > Like: > > let simpleStyle = GeoJSONStyle.parseSimpleStyle(tags); > if (simpleStyle) { ... } > > Also, remember cr.setLineWidth() ... Okay.
Created attachment 316982 [details] [review] Add support for mapbox simplestyle for GeoJSON
Review of attachment 316982 [details] [review]: Thanks! Looking good! We will soon merge this! ::: src/geoJSONStyle.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Alaf Azam <alafazam@gmail.com> Could this also be your git user/e-mail? @@ +25,3 @@ + Name: 'GeoJSONStyle', + + _init: function() { I want this to take a parameters argument, sort of like how place.js does it. _init: function(params) { this.lineWidth = params.lineWidth || 1; [...] this.color = this._parseColor(params.color); this.fillColor = [...] }, @@ +57,3 @@ + + return styleObj; +} I wwuld like this function to construct the object more like this: return new GeoJSONStyle({ lineWidth: tags.strokeWidth, color: tags.stroke, [...] }); @@ +59,3 @@ +} + +GeoJSONStyle.parseColorString = function(colorString) { I think this function could be a member of the class. And called maybe "_hexToColor" or something like that? @@ +60,3 @@ + +GeoJSONStyle.parseColorString = function(colorString) { + let m = ''; Why is this needed? @@ +62,3 @@ + let m = ''; + + if (colorString.startsWith('#')){ Missing space before { @@ +64,3 @@ + if (colorString.startsWith('#')){ + colorString = colorString.slice(1); + print(colorString); Left over debug print. @@ +68,3 @@ + + if (colorString.length === 3) { + Remove blank line @@ +69,3 @@ + if (colorString.length === 3) { + + m = colorString.match(/([0-9a-f]{3})$/i)[1]; Either add a comment here or move all regex to the top of the file and name them in some way, maybe look at how Polari does it: https://git.gnome.org/browse/polari/tree/src/utils.js @@ +71,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i)[1]; + return { + red: (parseInt(m.chatAt(0),16)*0x11)/255, Some spaces would be nice here, and also format a bit different: return { red: (parseInt(m.charAt(0), 16) * 0x11) / 255, green: ... blue: ... }; Btw: Why * 0x11 ? I do not understand that. @@ +77,3 @@ + + } else if(colorString.length === 6) { + Remove empty line @@ +83,3 @@ + green: parseInt(m.substr(2,2),16)/255, + blue: parseInt(m.substr(4,2),16)/255 + }; Same comments as above. @@ +90,3 @@ + green: 0, + blue: 0 + }; Format as specified above.
Review of attachment 316982 [details] [review]: ::: src/geoJSONSource.js @@ +219,3 @@ _renderTile: function(tile) { let tileJSON = this._tileIndex.getTile(tile.zoom_level, tile.x, tile.y); + let geoJSONStyleObj = new GeoJSONStyle.GeoJSONStyle(); There is no need to instantiate this, right?
Review of attachment 316982 [details] [review]: ::: src/geoJSONSource.js @@ +244,3 @@ + if (feature.tags){ + geoJSONStyleObj = GeoJSONStyle.GeoJSONStyle.parseSimpleStyle(feature.tags); If there are no tags then we need to have default values. So if geoJSONStyleObj is present it gives default values. Is this not the right way??
Review of attachment 316982 [details] [review]: ::: src/geoJSONSource.js @@ +244,3 @@ + if (feature.tags){ + geoJSONStyleObj = GeoJSONStyle.GeoJSONStyle.parseSimpleStyle(feature.tags); I think this should give default values if tags is NULL instead.
Review of attachment 316982 [details] [review]: ::: src/geoJSONStyle.js @@ +16,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Alaf Azam <alafazam@gmail.com> yup. my name is pretty unique. I am the only 'alafazam' present online. @@ +25,3 @@ + Name: 'GeoJSONStyle', + + _init: function() { okay. @@ +59,3 @@ +} + +GeoJSONStyle.parseColorString = function(colorString) { oh, yes. you are right. @@ +62,3 @@ + let m = ''; + + if (colorString.startsWith('#')){ Sorry. :) @@ +64,3 @@ + if (colorString.startsWith('#')){ + colorString = colorString.slice(1); + print(colorString); Oops. @@ +71,3 @@ + m = colorString.match(/([0-9a-f]{3})$/i)[1]; + return { + red: (parseInt(m.chatAt(0),16)*0x11)/255, in three-character format, each value is multiplied by 0x11 to give an even scale from 0x00 to 0xff.
Created attachment 317127 [details] [review] Add support for mapbox simplestyle for GeoJSON
Created attachment 317129 [details] [review] Add support for mapbox simplestyle for GeoJSON
Review of attachment 317129 [details] [review]: Thanks some more nits! Sorry :) About the patch, how about going $ git config --global user.name "Alaf Azam" To get your name the same way as in the copyright in our commit log? ::: src/geoJSONSource.js @@ +219,3 @@ _renderTile: function(tile) { let tileJSON = this._tileIndex.getTile(tile.zoom_level, tile.x, tile.y); + let geoJSONStyleObj = null; Not needed, right? ::: src/geoJSONStyle.js @@ +27,3 @@ + _init: function(params) { + this.lineWidth = params.lineWidth || 1; + delete params.lineWidth; No need to delete the params. The reason why we do this for other classes is because they are subclasses of GObjects and when they pass the params along to the parent with this.parent(params); there can not be any properties on the object that do not belong to the parent class. @@ +29,3 @@ + delete params.lineWidth; + + this.strokeAlpha = params.strokeAlpha || 1; just alpha is ok I feel, no need for stroke. this.alpha. @@ +37,3 @@ + this.strokeStyle = this._hexToColor(params.strokeStyle) || { red: 0, + green: 0, + blue: 0 }; I prefer params.color and params.fillColor, since style is the whole thing and this is just color. @@ +48,3 @@ + + _hexToColor: function(colorString) { + let m = null; no need for this right? m is set and used in both if and else. Instead maybe have let color = null; And assign color in both === 3 and else below, and then return color at the end of the method? @@ +74,3 @@ + +GeoJSONStyle.parseSimpleStyle = function(tags) { + Remove blank line @@ +79,3 @@ + strokeStyle: tags["stroke"], + fillStyle: tags["fill"], + lineWidth: tags["stroke-width"] }); I think I prefer the "tags.stroke_width ... tags.stroke ... tags.fill" approach instead of tags['stroke-width'] ... Both should work right? And also: when qouting use single qoute marks : ' not " if not dealing with translations.
Created attachment 317183 [details] [review] Add support for mapbox simplestyle for GeoJSON
Review of attachment 317183 [details] [review]: Thanks! Awesome work! Some more comments below, I could fix them up when I merge tho. Thank you! ::: src/geoJSONStyle.js @@ +20,3 @@ + +const Lang = imports.lang; +const Champlain = imports.gi.Champlain; Champlain is not actually used, right? @@ +28,3 @@ + this.lineWidth = params.lineWidth || 1; + this.alpha = params.alpha || 1; + this.fillAlpha = params.fillAlpha || 0.25; I realised there is a problem with this, we can not have lineWidth, alpha or fillAlpha as 0, they will be converted to default in that case. @@ +49,3 @@ + if (colorString.length === 3) { + // validate and match color format abc + colorString = colorString.match(/([0-9a-f]{3})$/i)[1]; What happens if the string does not match? @@ +55,3 @@ + } else { + // validate and match color format abcdef + colorString = colorString.match(/([0-9a-f]{6})$/i)[1]; What happens if the string does not match here? We have a random string that is not 3 letters.
Review of attachment 317183 [details] [review]: ::: src/geoJSONStyle.js @@ +20,3 @@ + +const Lang = imports.lang; +const Champlain = imports.gi.Champlain; yup. my bad. @@ +28,3 @@ + this.lineWidth = params.lineWidth || 1; + this.alpha = params.alpha || 1; + this.fillAlpha = params.fillAlpha || 0.25; hm, yes that is a problem. but I can fix it. I tried a few things, and (this might sound unusual/silly ) but it is some JavaScript magic that accessing tags.fill_opacity when it is 0 gives undefined, and accessing it like tags['fill-opacity'] gives 0. Have u known this behavior? Should I report it as a bug ? > print('tags["fill_opacity"] : ' + tags['fill-opacity']); > print('tags.fill_opacity : ' + tags.fill_opacity); prints tags["fill_opacity"] : 0 tags.fill_opacity : undefined Should we get back to using tags['fill-opacity'] ? @@ +49,3 @@ + if (colorString.length === 3) { + // validate and match color format abc + colorString = colorString.match(/([0-9a-f]{3})$/i)[1]; maybe we can do something like this? if (colorString.length === 3) { // validate and match color format abc colorString = colorString.match(/([0-9a-f]{3})$/i); if (colorString) { color = { red: (parseInt(colorString[0].chatAt(0), 16) * 0x11)/255, green: (parseInt(colorString[0].chatAt(1), 16) * 0x11)/255, blue: (parseInt(colorString[0].chatAt(2), 16) * 0x11)/255 }; }; and same in second condition, so if string don't match color (= null) gets returned and gets assigned default values later.
Review of attachment 317183 [details] [review]: Thanks! ::: src/geoJSONStyle.js @@ +28,3 @@ + this.lineWidth = params.lineWidth || 1; + this.alpha = params.alpha || 1; + this.fillAlpha = params.fillAlpha || 0.25; Hmm, that is weird yes, sure if that is the case you can do it for this one. @@ +49,3 @@ + if (colorString.length === 3) { + // validate and match color format abc + colorString = colorString.match(/([0-9a-f]{3})$/i)[1]; Sure, sounds reasonable. We are then tolerant. We do not give error on syntax error, just silently ignore it, seems reasonable it the light of missing standardization here.
Created attachment 317207 [details] [review] Add support for mapbox simplestyle for GeoJSON
Review of attachment 317207 [details] [review]: Thanks! Great work!
Comment on attachment 317207 [details] [review] Add support for mapbox simplestyle for GeoJSON Attachment 317207 [details] pushed as e25c994 - Add support for mapbox simplestyle for GeoJSON
So items left in the simplestyle specification are: // OPTIONAL: default "" // A title to show when this item is clicked or // hovered over "title": "A title", // OPTIONAL: default "" // A description to show when this item is clicked or // hovered over "description": "A description", // OPTIONAL: default "medium" // specify the size of the marker. sizes // can be different pixel sizes in different // implementations // Value must be one of // "small" // "medium" // "large" "marker-size": "medium", // OPTIONAL: default "" // a symbol to position in the center of this icon // if not provided or "", no symbol is overlaid // and only the marker is shown // Allowed values include // - Icon ID from the Maki project at http://mapbox.com/maki/ // - An integer 0 through 9 // - A lowercase character "a" through "z" "marker-symbol": "bus", // OPTIONAL: default "7e7e7e" // the marker's color // // value must follow COLOR RULES "marker-color": "#fff", Reference: https://github.com/mapbox/simplestyle-spec/tree/master/1.1.0 I think the marker ones are hard, but maybe title and description could be added? Some art from leaflet that shows how they do it can be found here: http://leafletjs.com/examples/geojson.html
okay. I think why not have have 3 markers for different sizes? or maybe have marker in svg or geoJSON(crazy) itself. or something like that.
(In reply to Alaf from comment #36) > okay. > I think why not have have 3 markers for different sizes? or maybe have > marker in svg or geoJSON(crazy) itself. or something like that. I am not sure. Maybe we just do not allow styling markers. The markers denote a place. And the way we show a place in Maps is with our markers. For us having geojson style something as core as the marker feels like a big deal and maybe not good ux? Maybe confusing?
I'm no expert here nor have a good idea about how to show labels in general but I'd like to load aeronautical chart layers in Maps and one important bit is labels on the restricted aerspaces. You can see how they're usually rendered here: http://maps.openaip.net/?destination=http://www.openaip.net/ This particular implementation show different labels on different zoom levels (airspace class on low zoom-levels and boundary info on higher zooms) actually but that's not a requirement at all and actually I find it a bit annoying. :)
-- 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/28.