After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705112 - Reverse geocoding functions should return GeocodePlace object
Reverse geocoding functions should return GeocodePlace object
Status: RESOLVED FIXED
Product: geocode-glib
Classification: Other
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: geocode-glib maintainer(s)
geocode-glib maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-07-29 23:36 UTC by Zeeshan Ali
Modified: 2013-07-30 13:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
lib: Separate function to determine place type (1.64 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
committed Details | Review
lib: One dedicated function to create GeocodePlace object (3.41 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
committed Details | Review
lib: Export create_place_from_attributes library-wide (2.55 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
committed Details | Review
lib: Add new place getter/setters to symbols file (1.05 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
committed Details | Review
lib: geocode_reverse_resolve*() now returns GeocodePlace (8.43 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
needs-work Details | Review
lib: Remove now redundant code (4.70 KB, patch)
2013-07-29 23:36 UTC, Zeeshan Ali
committed Details | Review
lib: geocode_reverse_resolve*() now returns GeocodePlace (8.52 KB, patch)
2013-07-30 12:50 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2013-07-29 23:36:01 UTC
Instead of returning a hashtable, reverse geocoding functions should return a GeocodePlace instance.
Comment 1 Zeeshan Ali 2013-07-29 23:36:03 UTC
Created attachment 250417 [details] [review]
lib: Separate function to determine place type

Separate out code to determine place type from attributes in to a
separate function: get_place_type_from_attributes().
Comment 2 Zeeshan Ali 2013-07-29 23:36:06 UTC
Created attachment 250418 [details] [review]
lib: One dedicated function to create GeocodePlace object

We already have a function to create the GeocodePlace instance from
Nominatim attributes, create_place_from_attributes(). We should
initialize all props of these instances from within that function,
rather than doing it soon after calling it.
Comment 3 Zeeshan Ali 2013-07-29 23:36:10 UTC
Created attachment 250419 [details] [review]
lib: Export create_place_from_attributes library-wide

Make create_place_from_attributes available to other files by exporting
it library-wide as _geocode_create_place_from_attributes().
Comment 4 Zeeshan Ali 2013-07-29 23:36:13 UTC
Created attachment 250420 [details] [review]
lib: Add new place getter/setters to symbols file
Comment 5 Zeeshan Ali 2013-07-29 23:36:17 UTC
Created attachment 250421 [details] [review]
lib: geocode_reverse_resolve*() now returns GeocodePlace

Instead of returning a hashtable, reverse geocoding functions now return
a GeocodePlace instance.
Comment 6 Zeeshan Ali 2013-07-29 23:36:21 UTC
Created attachment 250422 [details] [review]
lib: Remove now redundant code

Remove now redundant code to translate Nominatim to XEP attributes.
Comment 7 Bastien Nocera 2013-07-30 12:38:05 UTC
Review of attachment 250417 [details] [review]:

Looks fine.
Comment 8 Bastien Nocera 2013-07-30 12:39:12 UTC
Review of attachment 250418 [details] [review]:

Looks good.
Comment 9 Bastien Nocera 2013-07-30 12:39:49 UTC
Review of attachment 250419 [details] [review]:

Looks good.
Comment 10 Bastien Nocera 2013-07-30 12:40:31 UTC
Review of attachment 250420 [details] [review]:

Yes.
Comment 11 Bastien Nocera 2013-07-30 12:44:54 UTC
Review of attachment 250421 [details] [review]:

::: geocode-glib/geocode-reverse.c
@@ +323,3 @@
+                GeocodePlace *place;
+
+                place = _geocode_create_place_from_attributes (result);

are you leaking the result hashtable here?

@@ +509,3 @@
 	g_object_unref (query);
 
+        place = _geocode_create_place_from_attributes (result);

Same comment about leaking the hashtable.
Comment 12 Bastien Nocera 2013-07-30 12:45:42 UTC
Review of attachment 250422 [details] [review]:

Success.
Comment 13 Zeeshan Ali 2013-07-30 12:50:44 UTC
Created attachment 250463 [details] [review]
lib: geocode_reverse_resolve*() now returns GeocodePlace

v2: Don't leak hashtables.
Comment 14 Bastien Nocera 2013-07-30 12:53:51 UTC
Review of attachment 250463 [details] [review]:

Looks good.
Comment 15 Zeeshan Ali 2013-07-30 13:07:14 UTC
Attachment 250417 [details] pushed as d72aee7 - lib: Separate function to determine place type
Attachment 250418 [details] pushed as 8ba11ad - lib: One dedicated function to create GeocodePlace object
Attachment 250419 [details] pushed as 26093f2 - lib: Export create_place_from_attributes library-wide
Attachment 250420 [details] pushed as 40d2d51 - lib: Add new place getter/setters to symbols file
Attachment 250422 [details] pushed as d64d4f1 - lib: Remove now redundant code
Attachment 250463 [details] pushed as fad7155 - lib: geocode_reverse_resolve*() now returns GeocodePlace