GNOME Bugzilla – Bug 719585
Add support for OSM viewbox and bound parameters
Last modified: 2013-12-05 15:08:09 UTC
I want to add support for the viewbox and bound paramters from OSM to geocode forward search. The viewbox parameter is a bounding box that specifies a geographic area to search. If the bounded paramater is 1 then the search is restricted to only items in the area. If the bounded parameter is 0 then the area is treated as "preferred". I will attach three patches. The first will make the private variable answer_count to a property. The second will add the search-area property and a new type GeocodeBoundingBox and will have the bounded parameter to osm as always 1 if a search-area is set. The third will add a bounded property to control the bounded parameter of osm. Thanks Jonas
Created attachment 263173 [details] [review] forward: make answer_count a property
Created attachment 263174 [details] [review] forward: add search area property Nominatim allows searches to be limited to a viewbox. In order to take advantage of this we need a new property. The search area is specified as a bounding box of latitude and longitude.
Created attachment 263175 [details] [review] forward: add bounded property The bounded boolean property decides if the search-area property sets an hard limit or not. If false the search-area is simply the prefered area for search. If true it restricts results to only items within the search-area.
Review of attachment 263173 [details] [review]: ACK w/ the corrected. ::: geocode-glib/geocode-forward.c @@ +994,3 @@ +geocode_forward_get_answer_count (GeocodeForward *forward) +{ + g_return_if_fail (GEOCODE_IS_FORWARD (forward)); you need to use g_return_val_if_fail here since function has a return value.
Review of attachment 263174 [details] [review]: FWIW, I would have divided this into two patches, one to add the new BoundingBox type and the other that makes use of it. ::: geocode-glib/geocode-bounding-box.c @@ +25,3 @@ +G_DEFINE_BOXED_TYPE (GeocodeBoundingBox, geocode_bounding_box, + geocode_bounding_box_copy, + geocode_bounding_box_free) Lets have a proper GObject here, makes it much easier for higher-level-languages/GIR to handle it. Besides location is a simpler type and its a GObject. IMHO Boxed types are only good for laziness now a days mostly unless you intend to create thousands or millions of instances at the same time. ::: geocode-glib/geocode-forward.c @@ +1035,3 @@ + box->left, box->top, box->right, box->bottom); + + geocode_forward_add (forward, "viewbox", area); this call creates its own copy for key and value so you need to g_free (area)
Review of attachment 263175 [details] [review]: Looks good otherwise. ::: geocode-glib/geocode-forward.c @@ +182,3 @@ + * GeocodeForward:bounded: + * + * If set to #TRUE then only results in the search-area bounding box are search-area -> #GeocodeForward:search-area to make it a link. @@ +1121,3 @@ +geocode_forward_get_bounded (GeocodeForward *forward) +{ + g_return_if_fail (GEOCODE_IS_FORWARD (forward)); You need to use g_return_val_if_fail .
Created attachment 263425 [details] [review] forward: make answer_count a property
Created attachment 263426 [details] [review] Add new object GeocodeBoundingBox Add an object representing a geographical area.
Created attachment 263427 [details] [review] forward: add search area property Nominatim allows searches to be limited to a viewbox. In order to take advantage of this we need a new property. The search area is specified as a GeocodeBoundingBox.
Created attachment 263428 [details] [review] forward: add bounded property The bounded boolean property decides if the search-area property sets an hard limit or not. If false the search-area is simply the prefered area for search. If true it restricts results to only items within the search-area.
Created attachment 263439 [details] [review] forward: add bounded property The bounded boolean property decides if the search-area property sets an hard limit or not. If false the search-area is simply the prefered area for search. If true it restricts results to only items within the search-area.
Created attachment 263447 [details] [review] forward: add search area property Nominatim allows searches to be limited to a viewbox. In order to take advantage of this we need a new property. The search area is specified as a GeocodeBoundingBox.
Review of attachment 263425 [details] [review]: Sorry still needs a bit more work :( ::: geocode-glib/geocode-forward.c @@ +136,3 @@ + G_MAXINT, + DEFAULT_ANSWER_COUNT, + G_PARAM_WRITABLE | So property is only writable (not readable) and you are only providing a getter (no setter). I think you want the prop to be readable as well (G_PARAM_READWRITE) and provide a setter too.
Review of attachment 263426 [details] [review]: Looks good otherwise. ::: geocode-glib/geocode-bounding-box.c @@ +187,3 @@ + 90, + 0.0, + G_PARAM_READWRITE | Since setters are not public, I guess you don't want these props to be writable after creation so you also want G_PARAM_CONSTRUCT_ONLY here.
Review of attachment 263447 [details] [review]: Looks good.
(In reply to comment #13) > Review of attachment 263425 [details] [review]: > > Sorry still needs a bit more work :( > It's me that should be sorry :) I've worked on these patches on stolen moments when the kid sleeps or I otherwise find time, I am super grateful for review. > ::: geocode-glib/geocode-forward.c > @@ +136,3 @@ > + G_MAXINT, > + DEFAULT_ANSWER_COUNT, > + G_PARAM_WRITABLE | > > So property is only writable (not readable) and you are only providing a getter > (no setter). I think you want the prop to be readable as well > (G_PARAM_READWRITE) and provide a setter too. Yes, it should be READWRITE. The setter is there tho, only I did not provide it, it was there from before.
(In reply to comment #14) > Review of attachment 263426 [details] [review]: > > Looks good otherwise. > > ::: geocode-glib/geocode-bounding-box.c > @@ +187,3 @@ > + 90, > + 0.0, > + G_PARAM_READWRITE | > > Since setters are not public, I guess you don't want these props to be writable > after creation so you also want G_PARAM_CONSTRUCT_ONLY here. Agreed, thanks.
Created attachment 263538 [details] [review] forward: make answer_count a property
Created attachment 263539 [details] [review] Add new object GeocodeBoundingBox Add an object representing a geographical area.
Created attachment 263540 [details] [review] Add new object GeocodeBoundingBox Add an object representing a geographical area.
Review of attachment 263439 [details] [review]: Looks good. Please push w/ this corrected. ::: geocode-glib/geocode-forward.c @@ +1091,3 @@ + * #GeocodeForward:search-area bounding box. + * + * Set the bounded property. If you are not going to document inline, at least to link the to the property. :) @@ +1134,3 @@ + * @forward: a #GeocodeForward representing a query + * + * Gets the bounded property. same here.
Review of attachment 263538 [details] [review]: ACK
Review of attachment 263540 [details] [review]: ack
Attachment 263439 [details] pushed as eca1275 - forward: add bounded property Attachment 263447 [details] pushed as f153ec7 - forward: add search area property Attachment 263538 [details] pushed as 7d5da03 - forward: make answer_count a property Attachment 263540 [details] pushed as aed6c80 - Add new object GeocodeBoundingBox