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 756311 - Don’t hard-code Nominatim server as nominatim.gnome.org
Don’t hard-code Nominatim server as nominatim.gnome.org
Status: RESOLVED FIXED
Product: geocode-glib
Classification: Other
Component: general
3.18.x
Other Linux
: Normal normal
: ---
Assigned To: geocode-glib maintainer(s)
geocode-glib maintainer(s)
: 766714 (view as bug list)
Depends on:
Blocks: 774631
 
 
Reported: 2015-10-09 18:28 UTC by Philip Withnall
Modified: 2017-01-13 23:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
RFC (66.81 KB, patch)
2016-02-22 17:31 UTC, Aleksander Morgado
none Details | Review
forward: add configurable email property (6.82 KB, patch)
2016-05-20 15:20 UTC, Justin Kim
none Details | Review
RFC: Add GeocodeConfig to set configurable parameters (19.33 KB, patch)
2016-05-27 07:40 UTC, Justin Kim
needs-work Details | Review
docs: Enable --rebuild-sections option for gtk-doc (927 bytes, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
geocode-forward: Add missing ‘Returns’ line to a gtk-doc comment (1.08 KB, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
geocode-glib: Add g_autoptr() cleanup function definitions (1.49 KB, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
geocode-reverse: Fix indentation (976 bytes, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
test-gcglib: Update expected test results to match latest OSM data (1.96 KB, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
test-gcglib: Minor const-correctness fix (789 bytes, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
test-gcglib: Fix a couple of memory leaks in the tests (1.02 KB, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
test-gcglib: Add a missing error assertion (981 bytes, patch)
2016-10-13 16:32 UTC, Philip Withnall
committed Details | Review
geocode-glib: Add a backend abstraction (128.65 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
tests: Port test-gcglib to optionally use GeocodeNominatim offline (53.72 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
tests: Add copyright headers to test C files (3.07 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
tests: Rename a global variable to be less generic (2.91 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
docs: Fix multiple declarations of the enums documentation section (1.78 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
committed Details | Review
geocode-glib: Fix various minor const-correctness issues (3.82 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
committed Details | Review
geocode-place: Fix -Wswitch-enum warnings (981 bytes, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Update to latest GNOME autogen.sh (1.94 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Port from GNOME_DEBUG_CHECK to AX_CHECK_ENABLE_DEBUG (1.06 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Drop use of GNOME_MAINTAINER_MODE_DEFINES (882 bytes, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Port from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS (2.25 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Add AX_IS_RELEASE (1.10 KB, patch)
2016-10-13 16:33 UTC, Philip Withnall
none Details | Review
build: Use AM_CPPFLAGS rather than INCLUDES (1.25 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
fixup! geocode-glib: Add g_autoptr() cleanup function definitions (1.85 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
needs-work Details | Review
geocode-glib: Make some private functions static (2.64 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
geocode-backend: Add support for multiple reverse results (11.58 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
geocode-nominatim: Allow the cache path to be adjusted (18.81 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
geocode-glib: Fix documentation for list return values (3.37 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
tests: Fix some minor memory leaks in test-gcglib (1.44 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
geocode-glib: Accept a hash table for GeocodeBackend.reverse_resolve() (17.95 KB, patch)
2016-10-13 16:34 UTC, Philip Withnall
none Details | Review
build: Bump GIO dependency to 2.44 (810 bytes, patch)
2016-11-08 17:33 UTC, Philip Withnall
none Details | Review
build: Bump version to 3.23.0 (919 bytes, patch)
2016-11-08 17:33 UTC, Philip Withnall
reviewed Details | Review
geocode-glib: Add a backend abstraction (128.24 KB, patch)
2016-11-08 17:34 UTC, Philip Withnall
none Details | Review
geocode-glib: Add a backend abstraction (128.71 KB, patch)
2016-11-17 17:17 UTC, Philip Withnall
none Details | Review
build: Bump GIO dependency to 2.44 (810 bytes, patch)
2017-01-06 23:52 UTC, Philip Withnall
committed Details | Review
build: Bump version to 3.23.1 (840 bytes, patch)
2017-01-06 23:52 UTC, Philip Withnall
committed Details | Review
geocode-glib: Add a backend abstraction (128.78 KB, patch)
2017-01-06 23:52 UTC, Philip Withnall
committed Details | Review
tests: Port test-gcglib to optionally use GeocodeNominatim offline (53.72 KB, patch)
2017-01-06 23:52 UTC, Philip Withnall
committed Details | Review
build: Add perturbation variables to test environment (996 bytes, patch)
2017-01-06 23:52 UTC, Philip Withnall
committed Details | Review
tests: Add copyright headers to test C files (3.23 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
tests: Rename a global variable to be less generic (3.02 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
geocode-place: Fix -Wswitch-enum warnings (981 bytes, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
geocode-place: Simplify switch statement (2.78 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
build: Update to latest GNOME autogen.sh (1.94 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
build: Port from GNOME_DEBUG_CHECK to AX_CHECK_ENABLE_DEBUG (1.06 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
build: Drop use of GNOME_MAINTAINER_MODE_DEFINES (882 bytes, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
build: Port from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS (2.25 KB, patch)
2017-01-06 23:53 UTC, Philip Withnall
committed Details | Review
build: Add AX_IS_RELEASE (1.10 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
build: Use AM_CPPFLAGS rather than INCLUDES (1.25 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
geocode-glib: Make some private functions static (2.64 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
geocode-backend: Add support for multiple reverse results (11.69 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
geocode-nominatim: Allow the cache path to be adjusted (18.83 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
reviewed Details | Review
geocode-glib: Fix documentation for list return values (3.37 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
tests: Fix some minor memory leaks in test-gcglib (1.44 KB, patch)
2017-01-06 23:54 UTC, Philip Withnall
committed Details | Review
geocode-glib: Accept a hash table for GeocodeBackend.reverse_resolve() (18.32 KB, patch)
2017-01-06 23:55 UTC, Philip Withnall
committed Details | Review
geocode-glib: Mark UNRELEASED API as new in 3.23.1 (8.59 KB, patch)
2017-01-06 23:55 UTC, Philip Withnall
committed Details | Review
tests: Set XDG_CACHE_HOME to a temporary directory for tests (2.91 KB, patch)
2017-01-12 23:32 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2015-10-09 18:28:30 UTC
Hard-coding the server to a GNOME service is bad for a couple of reasons:
 • Non-GNOME users of geocode-glib (for example, on commercial devices) should be using their own servers
 • It makes testing geocode-glib locally against a server harder

It would be useful if the server address was configurable, and potentially didn’t have a default so that modules using it have to choose which server they want to use (or set up their own).

Ideally the fix for this would also make the server interface mockable so that it’s possible to unit test GeocodeForward and GeocodeReverse. I don’t think this is currently possible?
Comment 1 Philip Withnall 2015-10-09 18:32:27 UTC
(Also, the email parameter should be configurable, since I don’t think Zeeshan wants to be contacted about the abuses coming from other users of geocode-glib.)
Comment 2 Zeeshan Ali 2015-10-11 15:45:08 UTC
(In reply to Philip Withnall from comment #1)
> (Also, the email parameter should be configurable, since I don’t think
> Zeeshan wants to be contacted about the abuses coming from other users of
> geocode-glib.)

I really don't but what should it be then?
Comment 3 Bastien Nocera 2015-10-11 15:58:08 UTC
(In reply to Philip Withnall from comment #0)
> Hard-coding the server to a GNOME service is bad for a couple of reasons:
>  • Non-GNOME users of geocode-glib (for example, on commercial devices)
> should be using their own servers

Wouldn't they rebuild geocode-glib with a different server address and be done with it?

That might warrant a configure flag rather than anything more complicated.

>  • It makes testing geocode-glib locally against a server harder

For testing purposes (depending on the testing), wouldn't it be easier to assign nominatim.gnome.org a new IP through /etc/hosts? Or is that not what your uhttpmock is supposed to solve?

> It would be useful if the server address was configurable, and potentially
> didn’t have a default so that modules using it have to choose which server
> they want to use (or set up their own).
> 
> Ideally the fix for this would also make the server interface mockable so
> that it’s possible to unit test GeocodeForward and GeocodeReverse. I don’t
> think this is currently possible?
Comment 4 Philip Withnall 2015-10-12 07:20:20 UTC
(In reply to Bastien Nocera from comment #3)
> (In reply to Philip Withnall from comment #0)
> > Hard-coding the server to a GNOME service is bad for a couple of reasons:
> >  • Non-GNOME users of geocode-glib (for example, on commercial devices)
> > should be using their own servers
> 
> Wouldn't they rebuild geocode-glib with a different server address and be
> done with it?
> 
> That might warrant a configure flag rather than anything more complicated.

Yup, that would be fine.

> >  • It makes testing geocode-glib locally against a server harder
> 
> For testing purposes (depending on the testing), wouldn't it be easier to
> assign nominatim.gnome.org a new IP through /etc/hosts? Or is that not what
> your uhttpmock is supposed to solve?

Editing /etc/hosts for unit testing isn’t very automatable, but uhttpmock could be used (it overrides the default GResolver). It requires that the HTTP port is customisable, since otherwise you have to run the tests with root privileges to get port 80.
Comment 5 Bastien Nocera 2015-10-12 09:10:26 UTC
(In reply to Philip Withnall from comment #4)
> (In reply to Bastien Nocera from comment #3)
> > (In reply to Philip Withnall from comment #0)
> > > Hard-coding the server to a GNOME service is bad for a couple of reasons:
> > >  • Non-GNOME users of geocode-glib (for example, on commercial devices)
> > > should be using their own servers
> > 
> > Wouldn't they rebuild geocode-glib with a different server address and be
> > done with it?
> > 
> > That might warrant a configure flag rather than anything more complicated.
> 
> Yup, that would be fine.
> 
> > >  • It makes testing geocode-glib locally against a server harder
> > 
> > For testing purposes (depending on the testing), wouldn't it be easier to
> > assign nominatim.gnome.org a new IP through /etc/hosts? Or is that not what
> > your uhttpmock is supposed to solve?
> 
> Editing /etc/hosts for unit testing isn’t very automatable, but uhttpmock
> could be used (it overrides the default GResolver). It requires that the
> HTTP port is customisable, since otherwise you have to run the tests with
> root privileges to get port 80.

Which you could do by changing the URL through configure, correct?
Comment 6 Philip Withnall 2015-10-13 08:09:14 UTC
(In reply to Bastien Nocera from comment #5)
> Which you could do by changing the URL through configure, correct?

But that would mean re-running configure and rebuilding geocode-glib to run the test suite vs running against the actual server, which seems like an unnecessary faff for the sake of adding one URI property.
Comment 7 Aleksander Morgado 2016-02-22 17:31:20 UTC
Created attachment 321869 [details] [review]
RFC

I went a bit off w.r.t. the original suggestion I think, but well, let me know what you think.

Instead of just allowing to change the base URL of the nominatim server, I drafted a "GeocodeBackend" interface which has some generic actions for reverse resolving (didn't do forward yet, wanted to run it through comments first). Then, a new "GeocodeNominatim" object, which implements GeocodeBackend, can be used to do the reverse geocoding operation on a nominatim server. If no backend is explicitly specified, the code falls back to the GNOME nominatim server, implemented as a singleton.

For easy testing of the "GeocodeNominatim" object, and to avoid needing a real connection to a server during unit tests, the GeocodeNominatim object is subclassed (e.g. GeocodeNominatimTest) to just provide a predefined response for a given set of parameters.

This new logic allows creating custom backends (e.g. not nominatim-based) and also non-gnome nominatim-based backends, while keeping the gnome one as default.  Does this sound like a good approach? Or too complex?
Comment 8 Jonas Danielsson 2016-02-22 18:37:37 UTC
(In reply to Aleksander Morgado from comment #7)
> Created attachment 321869 [details] [review] [review]
> RFC
> 
> I went a bit off w.r.t. the original suggestion I think, but well, let me
> know what you think.
> 
> Instead of just allowing to change the base URL of the nominatim server, I
> drafted a "GeocodeBackend" interface which has some generic actions for
> reverse resolving (didn't do forward yet, wanted to run it through comments
> first). Then, a new "GeocodeNominatim" object, which implements
> GeocodeBackend, can be used to do the reverse geocoding operation on a
> nominatim server. If no backend is explicitly specified, the code falls back
> to the GNOME nominatim server, implemented as a singleton.
> 
> For easy testing of the "GeocodeNominatim" object, and to avoid needing a
> real connection to a server during unit tests, the GeocodeNominatim object
> is subclassed (e.g. GeocodeNominatimTest) to just provide a predefined
> response for a given set of parameters.
> 
> This new logic allows creating custom backends (e.g. not nominatim-based)
> and also non-gnome nominatim-based backends, while keeping the gnome one as
> default.  Does this sound like a good approach? Or too complex?


Hi thanks!

Too complex I think. I liked Bastiens suggestion of having a configure flag to override the nominatim URI. This is a bit like a solution looking for a problem.

What is your use-case for doing this? What application or company wants this? Right now geocode-glib is pretty much an interface to OpenStreetMap and towards Nominatim. Having backends might be fine... but then we should implement at least two of them before merging something like this. I do not want to add complexity because we can, you know?
Comment 9 Aleksander Morgado 2016-02-23 07:33:44 UTC
> 
> What is your use-case for doing this? What application or company wants
> this? Right now geocode-glib is pretty much an interface to OpenStreetMap
> and towards Nominatim. Having backends might be fine... but then we should
> implement at least two of them before merging something like this. I do not
> want to add complexity because we can, you know?

The use case here is being able to use multiple backends at the same time, so that the results can be merged into a single result later on. Just changing the base nominatim service url during configure doesn't solve this, plus it actually forces the user to need to recompile the library from scratch.

Regarding the multiple backends, well, there's already one default gnome nominatim backend, plus a generic nominatim backend (where you can assign a custom base url and requester email) plus a test nominatim backend (which doesn't need a valid internet connection to the service).

It is true that there's no non-nominatim based backend, and therefore the GeocodeBackend interface may really not be needed, but it kind of made sense at the time of writing the patch. Originally I wanted to have the test backend just implement the GeocodeBackend interface, but then the test wouldn't be testing any nominatim-specific stuff, so that's why I ended up just avoiding the HTTP request/response in the test, while leaving all the other logic testable, including the nominatim service response parsing.
Comment 10 Philip Withnall 2016-02-23 10:24:31 UTC
(In reply to Jonas Danielsson from comment #8)
> What is your use-case for doing this? What application or company wants
> this? Right now geocode-glib is pretty much an interface to OpenStreetMap
> and towards Nominatim. Having backends might be fine... but then we should
> implement at least two of them before merging something like this. I do not
> want to add complexity because we can, you know?

This is for a customer who wants to use geocode-glib in vehicles. They want the ability to use their own Nominatim instance (to avoid loading the GNOME servers), but potentially also want to use a proprietary geocoding service. More importantly, however, they want a mock backend for geocode-glib (amongst other things) which allows application developers to test their applications against different geocoding behaviour, rather than querying a real server.

For example, how do you test an application’s behaviour in the case that the geocoding server is unreachable? That’s why they’re going for a mock backend which could be controlled by another process on the developer’s machine.

This is one step towards that, and we will be implementing the mock backend itself soon (but in a separate bug, to avoid this one getting overcrowded).

Does that make sense?
Comment 11 Bastien Nocera 2016-02-23 13:13:47 UTC
(In reply to Philip Withnall from comment #10)
> (In reply to Jonas Danielsson from comment #8)
> > What is your use-case for doing this? What application or company wants
> > this? Right now geocode-glib is pretty much an interface to OpenStreetMap
> > and towards Nominatim. Having backends might be fine... but then we should
> > implement at least two of them before merging something like this. I do not
> > want to add complexity because we can, you know?
> 
> This is for a customer who wants to use geocode-glib in vehicles. They want
> the ability to use their own Nominatim instance (to avoid loading the GNOME
> servers), but potentially also want to use a proprietary geocoding service.

Given how different the code would be, and the requirements of the license, that would mean releasing the source code to that proprietary backend. Given that we'd likely not want this code in geocode-glib itself, what's the (short-term) use?
Comment 12 Philip Withnall 2016-02-23 13:40:57 UTC
(In reply to Bastien Nocera from comment #11)
> what's the (short-term) use?

The mock backend, which would ideally be upstream (and definitely not proprietary). Think like a window in Builder which allows you to set up mock geocoding responses, or a unit test which does the same, for testing your app.

(We are planning on doing this to various services, not just geocode-glib. GeoClue is on the list too.)
Comment 13 Bastien Nocera 2016-02-23 13:47:10 UTC
(In reply to Philip Withnall from comment #12)
> (In reply to Bastien Nocera from comment #11)
> > what's the (short-term) use?
> 
> The mock backend, which would ideally be upstream (and definitely not
> proprietary). Think like a window in Builder which allows you to set up mock
> geocoding responses, or a unit test which does the same, for testing your
> app.
> 
> (We are planning on doing this to various services, not just geocode-glib.
> GeoClue is on the list too.)

How is this better than mocking the results by mocking the network itself? This wouldn't need a separate backend, and would exercise the same code.
Comment 14 Philip Withnall 2016-02-23 15:33:54 UTC
(In reply to Bastien Nocera from comment #13)
> How is this better than mocking the results by mocking the network itself?
> This wouldn't need a separate backend, and would exercise the same code.

There are two separate things here: testing the existing Nominatim code, and providing mock results to geocode-glib clients to enable //them// to be tested.

For testing the Nominatim code, there is indeed uhttpmock, which would do a reasonable job. However, because it works at the HTTP layer, it’s a pain/impossible to simulate failures at lower layers, like hosts being unreachable. Testing things like timeouts means waiting for the timeout to expire, which is a pain but not the end of the world.

It also requires some way to override connections from geocode-glib out to the server — uhttpmock does this by overriding DNS lookups and needing the code under test to use a special port (not 80). So in any case, changes need to be made to geocode-glib to accommodate it.

uhttpmock could be used to provide mock results to clients, but I’m not sure it would be able to trigger the full range of error returns from geocode-glib code to the clients under test. In order to do mocking from Builder, for example, there would need to be some IPC between the mock UI and uhttpmock to provide the mock data (or between uhttpmock and geocode-glib to provide the port which uhttpmock is running on), at which point you have two bits of IPC (that, plus the HTTP between uhttpmock and geocode-glib) and it would be simpler to have a proper mock backend.

I’m not making this up. :-) This is something I tried in folks — we tried testing by mocking the D-Bus APIs of backends like EDS and Telepathy, and it was a complete pain. It was a lot easier once we wrote a mock backend for folks which integrated in the same way as the EDS backend, but just exposed all its API over D-Bus to be called by test programs.
Comment 15 Philip Withnall 2016-02-23 15:36:48 UTC
I should also say: since the client’s requirement for alternative backends is not negotiable, we have the choice of either using geocode-glib and adding backend support to it; or writing a wrapper library around geocode-glib which has backends and geocode-glib is one of them. It would be FOSS, but it seems a little pointless to have two geocoding libraries when one would do. I think it makes sense to add backends to geocode-glib, but ultimately it’s your decision. :-)
Comment 16 Bastien Nocera 2016-02-23 15:51:22 UTC
That'd be Jonas' decision actually :)
But I guess it makes sense.
Comment 17 Jonas Danielsson 2016-02-23 18:16:09 UTC
(In reply to Bastien Nocera from comment #16)
> That'd be Jonas' decision actually :)
> But I guess it makes sense.

So, yeah.

I like... 

... the fact that we can get a test backend to avoid having to update our test each time the state of the world changes.

... the idea of backends, since we might want to test out different things, here or in Maps, to try different geocoders, that we host ourself or third-party (that handle partial-search or type-ahead better than Nominatim)

So I am partial to having this get it. Thanks Phillip and Aleksander for providing more detail. I would however like to see the forward part of this before reviewing code, since I feel forward is more entwined, or how you say it, with Nominatim.

The code looks nice so I am feeling optimistic.
Comment 18 Justin Kim 2016-05-20 15:20:50 UTC
Created attachment 328267 [details] [review]
forward: add configurable email property

I tried to add an email property to make it configurable. However, I have no idea what email address should be provided as a default value for testing functions. So I just wrote a comment in 'test-gcglib.c'.
Comment 19 Justin Kim 2016-05-20 15:25:38 UTC
If I am doing this right in my patch for GeocodeFoward, I'll try to change the server parameters in a similar way. Please, review it. Thanks!
Comment 20 Justin Kim 2016-05-27 07:40:28 UTC
Created attachment 328628 [details] [review]
RFC: Add GeocodeConfig to set configurable parameters

To provide default values for server_url and email, I tried to add "GeocodeConfig" class like GeoClue does.
Comment 21 Justin Kim 2016-06-21 12:02:25 UTC
ping?
Comment 22 Jonas Danielsson 2016-06-21 18:07:31 UTC
Review of attachment 328628 [details] [review]:

Hi!

Thanks for the patch! I do not see the need for a configuration file though...
Is your use-case solved by just having a configure option that sets this permanent?


Btw:
What happened to the other approach in this bug? With the backends?
Comment 23 Emilio Pozuelo Monfort 2016-06-21 21:03:14 UTC
(In reply to Jonas Danielsson from comment #22)
> Btw:
> What happened to the other approach in this bug? With the backends?

I'm working on that. I'll attach a new patch addressing your comments (i.e. adding the forward part) soon.
Comment 24 Justin Kim 2016-06-22 03:09:21 UTC
(In reply to Jonas Danielsson from comment #22)
> Review of attachment 328628 [details] [review] [review]:
> Is your use-case solved by just having a configure option that sets this
> permanent?

A configuration option for these two parameters, url and email, is not always necessary for my use-case. However, I thought two different cases; One is to set default value when we create a package automatically. Another is to change the parameters for the purpose of testing or dynamic transitions without code modification and rebuilding.
To be done for the second case, I just did do more in configure.ac.

How about the idea of dynamic changes for the parameters, url and email?

This issue seems to get tangled by me because two different issues are posted. I'll fork another issue to handle parameter changes. Thank you.
Comment 25 Philip Withnall 2016-10-13 16:32:25 UTC
Created attachment 337622 [details] [review]
docs: Enable --rebuild-sections option for gtk-doc

Since we don’t commit the sections.txt file to git, we should explicitly
rebuild it automatically.
Comment 26 Philip Withnall 2016-10-13 16:32:29 UTC
Created attachment 337623 [details] [review]
geocode-forward: Add missing ‘Returns’ line to a gtk-doc comment

This adds a missing GIR annotation.
Comment 27 Philip Withnall 2016-10-13 16:32:34 UTC
Created attachment 337624 [details] [review]
geocode-glib: Add g_autoptr() cleanup function definitions

This allows us to use GeocodeForward and GeocodeReverse with
g_autoptr().
Comment 28 Philip Withnall 2016-10-13 16:32:39 UTC
Created attachment 337625 [details] [review]
geocode-reverse: Fix indentation
Comment 29 Philip Withnall 2016-10-13 16:32:43 UTC
Created attachment 337626 [details] [review]
test-gcglib: Update expected test results to match latest OSM data

Looks like these nodes have been edited online, so the online query
results no longer matched what we expected. Update what we expect.
Comment 30 Philip Withnall 2016-10-13 16:32:47 UTC
Created attachment 337627 [details] [review]
test-gcglib: Minor const-correctness fix
Comment 31 Philip Withnall 2016-10-13 16:32:52 UTC
Created attachment 337628 [details] [review]
test-gcglib: Fix a couple of memory leaks in the tests
Comment 32 Philip Withnall 2016-10-13 16:32:57 UTC
Created attachment 337629 [details] [review]
test-gcglib: Add a missing error assertion

This doesn’t change how the tests behave at the moment, but might catch
more errors in future.
Comment 33 Philip Withnall 2016-10-13 16:33:02 UTC
Created attachment 337630 [details] [review]
geocode-glib: Add a backend abstraction

Add an abstraction for backends, the GeocodeBackend interface, and split
the existing Nominatim code out into an implementation of that,
GeocodeNominatim. Parameterise this implementation so the default
instance uses nominatim.gnome.org, but other instances can be created
which use other Nominatim servers.

Rebase GeocodeForward and GeocodeReverse on GeocodeBackend so that they
use the new abstraction, and can have their backends changed to use a
custom GeocodeBackend instance. The default is the global GNOME
GeocodeNominatim instance.

The GeocodeNominatim class uses the same Nominatim code as before, moved
from GeocodeForward and GeocodeReverse. In order to be backend-agnostic,
the parameters accepted by GeocodeBackend methods use the XEP-0080 keys
and GValues, rather than anything specific to Nominatim, which means that
some of the parameter-handling code had to be reworked slightly.

GeocodeNominatim delegates its network requests to virtual methods — the
default implementations of these use libsoup as before, but a follow-up
commit will add a derived class which tests the Nominatim implementation
by overriding them to check the generated network requests.

This adds the following new API, but does not break backwards
compatibility:
 • GeocodeBackend
 • GeocodeNominatim
 • geocode_[forward|reverse]_set_backend()

It bumps the GLib dependency to 2.44 (from 2.34) so that new features
like g_set_object() and g_autoptr() can be used. 2.44 was released on
2015-03-23.
Comment 34 Philip Withnall 2016-10-13 16:33:07 UTC
Created attachment 337631 [details] [review]
tests: Port test-gcglib to optionally use GeocodeNominatim offline

GeocodeNominatim allows us to override its virtual methods for network
queries, so do that in a new GeocodeNominatimTest subclass, which loads
its results from a local set of results added by the test harness.

Port the existing test-gcglib tests to optionally use this new backend,
rather than the default GNOME Nominatim backend, to run the tests
offline. This means that they can reliably be run on CI or build
infrastructure with no internet access; and can be run with internet
access on developer machines (for example).
Comment 35 Philip Withnall 2016-10-13 16:33:12 UTC
Created attachment 337632 [details] [review]
tests: Add copyright headers to test C files

These have been missing for a while. Copyrights reconstructed from the
git log.
Comment 36 Philip Withnall 2016-10-13 16:33:17 UTC
Created attachment 337633 [details] [review]
tests: Rename a global variable to be less generic

Rename the commmand-line parameters variable to ‘cl_params’ so that it
isn’t shadowed by a huge number of other variables in the file.
Comment 37 Philip Withnall 2016-10-13 16:33:22 UTC
Created attachment 337634 [details] [review]
docs: Fix multiple declarations of the enums documentation section

It needs to be put in the template for the enum file, not each enum
type.
Comment 38 Philip Withnall 2016-10-13 16:33:27 UTC
Created attachment 337635 [details] [review]
geocode-glib: Fix various minor const-correctness issues

This fixes a load of compiler warnings.
Comment 39 Philip Withnall 2016-10-13 16:33:32 UTC
Created attachment 337636 [details] [review]
geocode-place: Fix -Wswitch-enum warnings

This switch is not meant to handle all of the values for the switch
(omissions are not a bug), so cast the value to an int to disable type
checking first.
Comment 40 Philip Withnall 2016-10-13 16:33:37 UTC
Created attachment 337637 [details] [review]
build: Update to latest GNOME autogen.sh

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 41 Philip Withnall 2016-10-13 16:33:42 UTC
Created attachment 337638 [details] [review]
build: Port from GNOME_DEBUG_CHECK to AX_CHECK_ENABLE_DEBUG

This is one step towards no longer depending on gnome-common.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 42 Philip Withnall 2016-10-13 16:33:46 UTC
Created attachment 337639 [details] [review]
build: Drop use of GNOME_MAINTAINER_MODE_DEFINES

It’s no longer useful for modern GNOME modules.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 43 Philip Withnall 2016-10-13 16:33:51 UTC
Created attachment 337640 [details] [review]
build: Port from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS

AX_COMPILER_FLAGS uses a standard set of warnings across all projects,
and a standard build API for enabling and disabling whether they are
warnings or errors.

Drop GNOME_CXX_WARNINGS because the project doesn’t use C++.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 44 Philip Withnall 2016-10-13 16:33:56 UTC
Created attachment 337641 [details] [review]
build: Add AX_IS_RELEASE

Automatically turn on or off various release-dependent configure options
based on whether a git directory is present. If it’s present, we assume
we’re doing development, so fatal warnings (etc.) are turned on.
Otherwise, we assume it’s a release and they are turned off.
Comment 45 Philip Withnall 2016-10-13 16:34:02 UTC
Created attachment 337642 [details] [review]
build: Use AM_CPPFLAGS rather than INCLUDES

INCLUDES is deprecated and will be removed in a future version of
automake. This fixes an automake warning.
Comment 46 Philip Withnall 2016-10-13 16:34:07 UTC
Created attachment 337643 [details] [review]
fixup! geocode-glib: Add g_autoptr() cleanup function definitions
Comment 47 Philip Withnall 2016-10-13 16:34:11 UTC
Created attachment 337644 [details] [review]
geocode-glib: Make some private functions static

These two functions are no longer needed in any files other than
geocode-nominatim.c, so they can be made private and removed from the
.symbols file.
Comment 48 Philip Withnall 2016-10-13 16:34:17 UTC
Created attachment 337645 [details] [review]
geocode-backend: Add support for multiple reverse results

Some geocoding APIs (such as the Google Geocoding API) can return
multiple results for a reverse resolution request, such as the building
at that location, the street it is on, the town it is in, etc.

While we currently only support Nominatim, other backends may be added
(potentially out of tree) in future. Change the GeocodeBackend API to
allow them to return multiple reverse results.

This does not change the GeocodeReverse API, which returns the first
result in the list, which should be the most relevant one.

https://developers.google.com/maps/documentation/geocoding/intro#ReverseGeocoding
Comment 49 Philip Withnall 2016-10-13 16:34:22 UTC
Created attachment 337646 [details] [review]
geocode-nominatim: Allow the cache path to be adjusted

Instead of always using the cache in $XDG_CACHE_HOME/geocode-glib, allow
the path to be customised, and set it to a temporary directory while
running the tests so we don’t accidentally use (or pollute) the user’s
actual cache.
Comment 50 Philip Withnall 2016-10-13 16:34:27 UTC
Created attachment 337647 [details] [review]
geocode-glib: Fix documentation for list return values

The documentation previously said that the return values were (transfer
container), but they were actually always (transfer full) (and all
callers in the tests treated them as such). Fix that in the documentation.
Comment 51 Philip Withnall 2016-10-13 16:34:33 UTC
Created attachment 337648 [details] [review]
tests: Fix some minor memory leaks in test-gcglib

It’s now memory-leak-clean when run under valgrind.
Comment 52 Philip Withnall 2016-10-13 16:34:37 UTC
Created attachment 337649 [details] [review]
geocode-glib: Accept a hash table for GeocodeBackend.reverse_resolve()

Instead of only accepting a GeocodeLocation, accept a hash table of keys
and GValues. Typically, this will contain `lat` and `lon` entries which
represent the GeocodeLocation. But in future, it could contain other
keys to look up a GeocodePlace by its backend-specific ID, for example;
or to limit the search results by place type.

This changes the API in GeocodeBackend (which is not stable yet), but
not in GeocodeReverse, so it’s not an API break.
Comment 53 Philip Withnall 2016-10-13 16:46:14 UTC
So here’s a massive patch set which does what Aleksander was doing before, but also does it for GeocodeForward. It keeps the same API as proposed by Aleksander to begin with, then mutates it a bit to allow for supporting multiple reverse results, and for things like bug #742512 in future. I also may have got a bit carried away with some build system cleanups and memory leak fixes.

Bastien has just pointed out that I forgot to squash one patch in; and I’ve just realised I need to amend the commit messages to credit Aleksander. I will do those changes locally, but won’t reupload everything now unless you really want me to. It should be OK to review like this?
Comment 54 Philip Withnall 2016-10-13 17:18:02 UTC
*** Bug 766714 has been marked as a duplicate of this bug. ***
Comment 55 Philip Withnall 2016-11-01 04:04:53 UTC
Any review comments?
Comment 56 Zeeshan Ali 2016-11-01 10:36:03 UTC
Review of attachment 337622 [details] [review]:

ack
Comment 57 Zeeshan Ali 2016-11-01 10:44:27 UTC
Review of attachment 337622 [details] [review]:

ack
Comment 58 Zeeshan Ali 2016-11-01 12:03:31 UTC
Review of attachment 337623 [details] [review]:

ACK
Comment 59 Zeeshan Ali 2016-11-01 15:48:05 UTC
Review of attachment 337624 [details] [review]:

ack but please make the shortlog fit under 50 chars since it's possible in this case.
Comment 60 Zeeshan Ali 2016-11-01 15:48:50 UTC
Review of attachment 337625 [details] [review]:

ack
Comment 61 Philip Withnall 2016-11-01 19:24:53 UTC
Attachment 337622 [details] pushed as dc87c43 - docs: Enable --rebuild-sections option for gtk-doc
Attachment 337623 [details] pushed as e4097a7 - geocode-forward: Add missing ‘Returns’ line to a gtk-doc comment
Attachment 337624 [details] pushed as 71154cd - geocode-glib: Add g_autoptr() cleanup functions
Attachment 337625 [details] pushed as a489cb4 - geocode-reverse: Fix indentation
Comment 62 Zeeshan Ali 2016-11-07 12:21:07 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 63 Zeeshan Ali 2016-11-07 12:22:55 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 64 Zeeshan Ali 2016-11-07 12:22:55 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 65 Zeeshan Ali 2016-11-07 12:22:55 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 66 Zeeshan Ali 2016-11-07 12:22:55 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 67 Zeeshan Ali 2016-11-07 12:22:55 UTC
Review of attachment 337626 [details] [review]:

ACK but shortlog seems too long.
Comment 68 Zeeshan Ali 2016-11-07 12:23:30 UTC
Review of attachment 337627 [details] [review]:

ack
Comment 69 Zeeshan Ali 2016-11-07 12:24:03 UTC
Review of attachment 337627 [details] [review]:

oops, wrong status
Comment 70 Zeeshan Ali 2016-11-07 12:26:05 UTC
Review of attachment 337628 [details] [review]:

ack, "in the tests" is redundant in shortlog IMO
Comment 71 Zeeshan Ali 2016-11-07 12:26:36 UTC
Review of attachment 337629 [details] [review]:

ack
Comment 72 Zeeshan Ali 2016-11-07 13:58:36 UTC
Review of attachment 337630 [details] [review]:

Lots of points for providing a lot of details in the commit log. :)

::: configure.ac
@@ -61,3 @@
 dnl Requires for the library
 PKG_CHECK_MODULES(GEOCODE,
-		  gio-2.0 >= 2.34

Please put it in a separate patch before this one. When writing release notes, it's harder to miss dep updates from log if they are in separate commits.

::: geocode-glib/geocode-backend.c
@@ +32,3 @@
+ * service must implement.
+ *
+ * Since: UNRELEASED

I'd bump our version (if it hasn't been done already) and set this already before it's forgotten.

@@ +45,3 @@
+ * @user_data: the data to pass to the @callback function
+ *
+ * Asynchronously performs a forward geocoding query using the backend. Use

the backend -> the backend @backend ?

@@ +332,3 @@
+
+static GeocodePlace *
+real_reverse_resolve_finish (GeocodeBackend  *backend,

nitpick: argument names off by 1 space.

::: geocode-glib/geocode-nominatim.c
@@ +145,3 @@
+get_search_uri_for_params (GeocodeNominatim  *self,
+                           GHashTable        *params,
+                           GError           **error)

extra space before arg names.

@@ +1264,3 @@
+ */
+GeocodeNominatim *
+geocode_nominatim_get_gnome (void)

This should be geocode_get_nominatim_gnome, geocode_nomimatim_gnome_get or geocode_nomimatim_gnome_get_singleton.

@@ +1275,3 @@
+		g_weak_ref_set (&backend_nominatim_gnome, backend);
+	}
+	G_UNLOCK (backend_nominatim_gnome_lock);

Why bother with multithreading in here? Rest of the API is not thread-safe.

@@ +1325,3 @@
+	/* Ensure our mandatory construction properties have been passed. */
+	g_assert (priv->base_url != NULL);
+	g_assert (priv->maintainer_email_address != NULL);

Not sure it's a good idea for a library to terminate the app. g_warn_if_fail() woudl be better imo.

@@ +1398,3 @@
+	iface->forward_search         = real_forward_search;
+	iface->forward_search_async   = real_forward_search_async;
+	iface->forward_search_finish  = real_forward_search_finish;

Don't see the point of 'real_' prefix. This is an implementation so obviously these functions are the real deal and there is nothing conflicting with static functions.

@@ +1402,3 @@
+	iface->reverse_resolve        = real_reverse_resolve;
+	iface->reverse_resolve_async  = real_reverse_resolve_async;
+	iface->reverse_resolve_finish = real_reverse_resolve_finish;

same here

@@ +1445,3 @@
+	 * Since: UNRELEASED
+	 */
+	properties[PROP_MAINTAINER_EMAIL_ADDRESS] = g_param_spec_string ("maintainer-email-address",

max 90 chars per line, if possible; otherwise, try your best to not go far beyond 80.

::: geocode-glib/geocode-reverse.h
@@ +75,3 @@
+                                    GCancellable        *cancellable,
+                                    GAsyncReadyCallback  callback,
+                                    gpointer             user_data);

splinter is not able to show what you are changing here but you shouldn't be changing anything in the public API anyway, at least in this patch.
Comment 73 Philip Withnall 2016-11-08 16:29:35 UTC
(In reply to Zeeshan Ali (Khattak) from comment #62)
> Review of attachment 337626 [details] [review] [review]:
> 
> ACK but shortlog seems too long.

I’d rather not try and rephrase the shortlogs of all the commits to make them shorter. They’re all limited to ~75 characters at the moment (as kernel style and common git practice suggests) — what’s the reason you want them shorter?
Comment 74 Philip Withnall 2016-11-08 17:25:58 UTC
(In reply to Zeeshan Ali (Khattak) from comment #72)
> Review of attachment 337630 [details] [review] [review]:
> 
> Lots of points for providing a lot of details in the commit log. :)

\o/

> ::: configure.ac
> @@ -61,3 @@
>  dnl Requires for the library
>  PKG_CHECK_MODULES(GEOCODE,
> -		  gio-2.0 >= 2.34
> 
> Please put it in a separate patch before this one. When writing release
> notes, it's harder to miss dep updates from log if they are in separate
> commits.

Done. I generally like to keep dependency changes in the same patch that introduces the need for them, so changes are self-contained (e.g. if you revert that patch, you don’t get left with a project with an arbitrarily bumped dependency due to not reverting the other patch), but I see your reasoning.

> ::: geocode-glib/geocode-backend.c
> @@ +32,3 @@
> + * service must implement.
> + *
> + * Since: UNRELEASED
> 
> I'd bump our version (if it hasn't been done already) and set this already
> before it's forgotten.

In my modules, I use UNRELEASED until release time, then change it to the release number in the commit which does the release. gtk-doc’s checks fail if UNRELEASED is present in the documentation, so it shouldn’t get forgotten.

However, your wish is my command, so I’ve changed it to 3.23.0, on the assumption that this will land in time for the 3.24 stable series; and that we shouldn’t add major new API in a stable point release (3.20.2). I’ve added a commit to bump the release version. The gnome-3-20 branch already exists, so there’s no need to branch from before this all lands.

> @@ +45,3 @@
> + * @user_data: the data to pass to the @callback function
> + *
> + * Asynchronously performs a forward geocoding query using the backend. Use
> 
> the backend -> the backend @backend ?

Changed to ‘the @backend’ to avoid repetition which would read poorly.

> @@ +332,3 @@
> +
> +static GeocodePlace *
> +real_reverse_resolve_finish (GeocodeBackend  *backend,
> 
> nitpick: argument names off by 1 space.

See below.

> ::: geocode-glib/geocode-nominatim.c
> @@ +145,3 @@
> +get_search_uri_for_params (GeocodeNominatim  *self,
> +                           GHashTable        *params,
> +                           GError           **error)
> 
> extra space before arg names.

They argument names are aligned, as per the GLib coding standard which Builder does automatically. I can change this if you really want, but it would be a pain and mostly a waste of time.

> @@ +1264,3 @@
> + */
> +GeocodeNominatim *
> +geocode_nominatim_get_gnome (void)
> 
> This should be geocode_get_nominatim_gnome, geocode_nomimatim_gnome_get or
> geocode_nomimatim_gnome_get_singleton.
> 
> @@ +1275,3 @@
> +		g_weak_ref_set (&backend_nominatim_gnome, backend);
> +	}
> +	G_UNLOCK (backend_nominatim_gnome_lock);
> 
> Why bother with multithreading in here? Rest of the API is not thread-safe.

I think the rest of the API probably is thread-safe in the sense that you can have two GeocodeBackend instances in different threads, and as long as they are only used from within the thread which owns them, there will be no races. In that case, since geocode_nominatim_get_gnome() is a singleton getter function, it *will* be accessed from multiple threads and will need locking.

I can remove it if you want, but it imposes little cost, and provides some benefit, even if it doesn’t mean we can go all the way to guaranteeing that geocode-glib is thread safe.

> @@ +1325,3 @@
> +	/* Ensure our mandatory construction properties have been passed. */
> +	g_assert (priv->base_url != NULL);
> +	g_assert (priv->maintainer_email_address != NULL);
> 
> Not sure it's a good idea for a library to terminate the app.
> g_warn_if_fail() woudl be better imo.

No. These assertions check assumptions which the rest of the code in that class depends on. If those assertions fail, the app developer has made a programming error. How can the class work if it doesn’t have a base URL to use in queries? There is no way to gracefully recover from if these assertions fail.

> @@ +1398,3 @@
> +	iface->forward_search         = real_forward_search;
> +	iface->forward_search_async   = real_forward_search_async;
> +	iface->forward_search_finish  = real_forward_search_finish;
> 
> Don't see the point of 'real_' prefix. This is an implementation so
> obviously these functions are the real deal and there is nothing conflicting
> with static functions.

Renamed to geocode_nominatim_* to be consistent with the other method implementations (*_get_property, etc.).

> @@ +1402,3 @@
> +	iface->reverse_resolve        = real_reverse_resolve;
> +	iface->reverse_resolve_async  = real_reverse_resolve_async;
> +	iface->reverse_resolve_finish = real_reverse_resolve_finish;
> 
> same here

Fixed.

> @@ +1445,3 @@
> +	 * Since: UNRELEASED
> +	 */
> +	properties[PROP_MAINTAINER_EMAIL_ADDRESS] = g_param_spec_string
> ("maintainer-email-address",
> 
> max 90 chars per line, if possible; otherwise, try your best to not go far
> beyond 80.

Wrapped to the next line.

> ::: geocode-glib/geocode-reverse.h
> @@ +75,3 @@
> +                                    GCancellable        *cancellable,
> +                                    GAsyncReadyCallback  callback,
> +                                    gpointer             user_data);
> 
> splinter is not able to show what you are changing here but you shouldn't be
> changing anything in the public API anyway, at least in this patch.

It was a whitespace fix to use spaces instead of tabs. You’re right; I’ve dropped it.
Comment 75 Philip Withnall 2016-11-08 17:33:39 UTC
Created attachment 339339 [details] [review]
build: Bump GIO dependency to 2.44

This will be needed in the following commits so that new features like
g_set_object() and g_autoptr() can be used. 2.44 was released on
2015-03-23.
Comment 76 Philip Withnall 2016-11-08 17:33:56 UTC
Created attachment 339340 [details] [review]
build: Bump version to 3.23.0

Bump to the first version in the 3.23.x unstable series, before the
following commits add a load of new API. (This commit is not itself a
release.)
Comment 77 Philip Withnall 2016-11-08 17:34:11 UTC
Created attachment 339341 [details] [review]
geocode-glib: Add a backend abstraction

Add an abstraction for backends, the GeocodeBackend interface, and split
the existing Nominatim code out into an implementation of that,
GeocodeNominatim. Parameterise this implementation so the default
instance uses nominatim.gnome.org, but other instances can be created
which use other Nominatim servers.

Rebase GeocodeForward and GeocodeReverse on GeocodeBackend so that they
use the new abstraction, and can have their backends changed to use a
custom GeocodeBackend instance. The default is the global GNOME
GeocodeNominatim instance.

The GeocodeNominatim class uses the same Nominatim code as before, moved
from GeocodeForward and GeocodeReverse. In order to be backend-agnostic,
the parameters accepted by GeocodeBackend methods use the XEP-0080 keys
and GValues, rather than anything specific to Nominatim, which means that
some of the parameter-handling code had to be reworked slightly.

GeocodeNominatim delegates its network requests to virtual methods — the
default implementations of these use libsoup as before, but a follow-up
commit will add a derived class which tests the Nominatim implementation
by overriding them to check the generated network requests.

This adds the following new API, but does not break backwards
compatibility:
 • GeocodeBackend
 • GeocodeNominatim
 • geocode_[forward|reverse]_set_backend()

This requires the recently-bumped GIO dependency of 2.44.

Largely based on a patch by Aleksander Morgado
<aleksander.morgado@collabora.co.uk>.
Comment 78 Philip Withnall 2016-11-08 17:36:22 UTC
Here is the updated backend commit, and the split out changes to configure.ac (which are ordered directly before it in the branch: https://git.collabora.com/cgit/user/pwith/geocode-glib.git/log/?h=backend-interface). Some of the following patches needed minor changes to apply correctly after my updates, but nothing which should affect reviews, hence I haven’t re-attached them all here, to save a bit of pressure on your inbox. If you want me to re-attach the entire branch, let me know.
Comment 79 Philip Withnall 2016-11-08 19:23:10 UTC
(In reply to Philip Withnall from comment #74)
> (In reply to Zeeshan Ali (Khattak) from comment #72)
> > Review of attachment 337630 [details] [review] [review] [review]:
> > 
> > ::: geocode-glib/geocode-backend.c
> > @@ +32,3 @@
> > + * service must implement.
> > + *
> > + * Since: UNRELEASED
> > 
> > I'd bump our version (if it hasn't been done already) and set this already
> > before it's forgotten.
> 
> However, your wish is my command, so I’ve changed it to 3.23.0, on the
> assumption that this will land in time for the 3.24 stable series; and that
> we shouldn’t add major new API in a stable point release (3.20.2). I’ve
> added a commit to bump the release version. The gnome-3-20 branch already
> exists, so there’s no need to branch from before this all lands.

Whoops, forgot to actually change the UNRELEASED values. I’ll do that in the next iteration of the patch (let me know if 3.23.0 is the best version number to use).
Comment 80 Zeeshan Ali 2016-11-09 09:18:04 UTC
(In reply to Philip Withnall from comment #74)
> (In reply to Zeeshan Ali (Khattak) from comment #72)
> > Review of attachment 337630 [details] [review] [review] [review]:
> > 
> > Lots of points for providing a lot of details in the commit log. :)
> 
> \o/

Even more points if you use 'Review' link/splinter for replying to inline comments. :)

> > ::: configure.ac
> > @@ -61,3 @@
> >  dnl Requires for the library
> >  PKG_CHECK_MODULES(GEOCODE,
> > -		  gio-2.0 >= 2.34
> > 
> > Please put it in a separate patch before this one. When writing release
> > notes, it's harder to miss dep updates from log if they are in separate
> > commits.
> 
> Done. I generally like to keep dependency changes in the same patch that
> introduces the need for them, so changes are self-contained (e.g. if you
> revert that patch, you don’t get left with a project with an arbitrarily
> bumped dependency due to not reverting the other patch), but I see your
> reasoning.

Thanks. Your reasoning sound good as well FWIW.

> > ::: geocode-glib/geocode-backend.c
> > @@ +32,3 @@
> > + * service must implement.
> > + *
> > + * Since: UNRELEASED
> > 
> > I'd bump our version (if it hasn't been done already) and set this already
> > before it's forgotten.
> 
> In my modules, I use UNRELEASED until release time, then change it to the
> release number in the commit which does the release. gtk-doc’s checks fail
> if UNRELEASED is present in the documentation, so it shouldn’t get forgotten.

I didn't know of this check. Let's leave it like this if `make distcheck` is going to fail.

> However, your wish is my command, so I’ve changed it to 3.23.0, on the
> assumption that this will land in time for the 3.24 stable series; and that
> we shouldn’t add major new API in a stable point release (3.20.2). I’ve
> added a commit to bump the release version. The gnome-3-20 branch already
> exists, so there’s no need to branch from before this all lands.

Yup. I'm sure this will land in time for 3.24 but let's leave it on UNRELEASED.

> > @@ +45,3 @@
> > + * @user_data: the data to pass to the @callback function
> > + *
> > + * Asynchronously performs a forward geocoding query using the backend. Use
> > 
> > the backend -> the backend @backend ?
> 
> Changed to ‘the @backend’ to avoid repetition which would read poorly.

Sure, makes sense.

> > ::: geocode-glib/geocode-nominatim.c
> > @@ +145,3 @@
> > +get_search_uri_for_params (GeocodeNominatim  *self,
> > +                           GHashTable        *params,
> > +                           GError           **error)
> > 
> > extra space before arg names.
> 
> They argument names are aligned, as per the GLib coding standard which
> Builder does automatically. I can change this if you really want, but it
> would be a pain and mostly a waste of time.

Really? Are you sure it's not a bug in Builder? Ignore me if that's the case.
 
> > @@ +1264,3 @@
> > + */
> > +GeocodeNominatim *
> > +geocode_nominatim_get_gnome (void)
> > 
> > This should be geocode_get_nominatim_gnome, geocode_nomimatim_gnome_get or
> > geocode_nomimatim_gnome_get_singleton.
> > 
> > @@ +1275,3 @@
> > +		g_weak_ref_set (&backend_nominatim_gnome, backend);
> > +	}
> > +	G_UNLOCK (backend_nominatim_gnome_lock);
> > 
> > Why bother with multithreading in here? Rest of the API is not thread-safe.
> 
> I think the rest of the API probably is thread-safe in the sense that you
> can have two GeocodeBackend instances in different threads, and as long as
> they are only used from within the thread which owns them, there will be no
> races. In that case, since geocode_nominatim_get_gnome() is a singleton
> getter function, it *will* be accessed from multiple threads and will need
> locking.
> 
> I can remove it if you want, but it imposes little cost, and provides some
> benefit, even if it doesn’t mean we can go all the way to guaranteeing that
> geocode-glib is thread safe.

Ah OK, let's keep it then.

> > @@ +1325,3 @@
> > +	/* Ensure our mandatory construction properties have been passed. */
> > +	g_assert (priv->base_url != NULL);
> > +	g_assert (priv->maintainer_email_address != NULL);
> > 
> > Not sure it's a good idea for a library to terminate the app.
> > g_warn_if_fail() woudl be better imo.
> 
> No. These assertions check assumptions which the rest of the code in that
> class depends on. If those assertions fail, the app developer has made a
> programming error. How can the class work if it doesn’t have a base URL to
> use in queries? There is no way to gracefully recover from if these
> assertions fail.

OK.
Comment 81 Zeeshan Ali 2016-11-09 09:21:00 UTC
(In reply to Philip Withnall from comment #73)
> (In reply to Zeeshan Ali (Khattak) from comment #62)
> > Review of attachment 337626 [details] [review] [review] [review]:
> > 
> > ACK but shortlog seems too long.
> 
> I’d rather not try and rephrase the shortlogs of all the commits to make
> them shorter. They’re all limited to ~75 characters at the moment (as kernel
> style and common git practice suggests) — what’s the reason you want them
> shorter?

The description part is limited to 75 characters (per line) but shortlog is supposed to be 50 chars. If you see `git log` output in a 80-chars terminal, you'll see the rationale for it. Read under "DISCUSSION" in "git commit" manpage.
Comment 82 Zeeshan Ali 2016-11-09 09:21:33 UTC
(In reply to Zeeshan Ali (Khattak) from comment #81)
> (In reply to Philip Withnall from comment #73)
> > (In reply to Zeeshan Ali (Khattak) from comment #62)
> > > Review of attachment 337626 [details] [review] [review] [review] [review]:
> > > 
> > > ACK but shortlog seems too long.
> > 
> > I’d rather not try and rephrase the shortlogs of all the commits to make
> > them shorter. They’re all limited to ~75 characters at the moment (as kernel
> > style and common git practice suggests) — what’s the reason you want them
> > shorter?
> 
> The description part is limited to 75 characters (per line) but shortlog is
> supposed to be 50 chars. If you see `git log` output in a 80-chars terminal,
> you'll see the rationale for it. Read under "DISCUSSION" in "git commit"
> manpage.

Also: https://wiki.gnome.org/Git/CommitMessages
Comment 83 Philip Withnall 2016-11-09 15:21:21 UTC
(In reply to Zeeshan Ali (Khattak) from comment #80)
> (In reply to Philip Withnall from comment #74)
> > (In reply to Zeeshan Ali (Khattak) from comment #72)
> > > Review of attachment 337630 [details] [review] [review] [review] [review]:
> > > 
> > > Lots of points for providing a lot of details in the commit log. :)
> > 
> > \o/
> 
> Even more points if you use 'Review' link/splinter for replying to inline
> comments. :)

Ah, I never knew about that feature, thanks.

> > > ::: geocode-glib/geocode-backend.c
> > > @@ +32,3 @@
> > > + * service must implement.
> > > + *
> > > + * Since: UNRELEASED
> > > 
> > > I'd bump our version (if it hasn't been done already) and set this already
> > > before it's forgotten.
> > 
> > In my modules, I use UNRELEASED until release time, then change it to the
> > release number in the commit which does the release. gtk-doc’s checks fail
> > if UNRELEASED is present in the documentation, so it shouldn’t get forgotten.
> 
> I didn't know of this check. Let's leave it like this if `make distcheck` is
> going to fail.

Indeed, it will fail with:
> /home/philip/.cache/jhbuild/build/geocode-glib/docs/geocode-glib-docs.xml doesn't appear to include "xml/api-index-UNRELEASED.xml"

> > However, your wish is my command, so I’ve changed it to 3.23.0, on the
> > assumption that this will land in time for the 3.24 stable series; and that
> > we shouldn’t add major new API in a stable point release (3.20.2). I’ve
> > added a commit to bump the release version. The gnome-3-20 branch already
> > exists, so there’s no need to branch from before this all lands.
> 
> Yup. I'm sure this will land in time for 3.24 but let's leave it on
> UNRELEASED.

OK, thanks.

> > > ::: geocode-glib/geocode-nominatim.c
> > > @@ +145,3 @@
> > > +get_search_uri_for_params (GeocodeNominatim  *self,
> > > +                           GHashTable        *params,
> > > +                           GError           **error)
> > > 
> > > extra space before arg names.
> > 
> > They argument names are aligned, as per the GLib coding standard which
> > Builder does automatically. I can change this if you really want, but it
> > would be a pain and mostly a waste of time.
> 
> Really? Are you sure it's not a bug in Builder? Ignore me if that's the case.

Yup. I’m sure it’s not a bug in Builder.

(In reply to Zeeshan Ali (Khattak) from comment #81)
> (In reply to Philip Withnall from comment #73)
> > (In reply to Zeeshan Ali (Khattak) from comment #62)
> > > Review of attachment 337626 [details] [review] [review] [review] [review]:
> > > 
> > > ACK but shortlog seems too long.
> > 
> > I’d rather not try and rephrase the shortlogs of all the commits to make
> > them shorter. They’re all limited to ~75 characters at the moment (as kernel
> > style and common git practice suggests) — what’s the reason you want them
> > shorter?
> 
> The description part is limited to 75 characters (per line) but shortlog is
> supposed to be 50 chars. If you see `git log` output in a 80-chars terminal,
> you'll see the rationale for it. Read under "DISCUSSION" in "git commit"
> manpage.

`git log` and `git shortlog` look fine for me in an 80-character terminal. What options do you pass to `git log` for it to break? With the options I commonly use (`git log --graph --decorate --pretty=oneline --abbrev-commit`), 10 characters are lost to overhead on each line, leaving ~70 for the summary.

I can find plenty of places which say ‘use 50 characters for the summary’, but nowhere which actually gives a rationale for *why*.
Comment 84 Zeeshan Ali 2016-11-10 08:37:58 UTC
(In reply to Philip Withnall from comment #83)
> (In reply to Zeeshan Ali (Khattak) from comment #80)
> (In reply to Zeeshan Ali (Khattak) from comment #81)
> > (In reply to Philip Withnall from comment #73)
> > > (In reply to Zeeshan Ali (Khattak) from comment #62)
> > > > Review of attachment 337626 [details] [review] [review] [review] [review] [review]:
> > > > 
> > > > ACK but shortlog seems too long.
> > > 
> > > I’d rather not try and rephrase the shortlogs of all the commits to make
> > > them shorter. They’re all limited to ~75 characters at the moment (as kernel
> > > style and common git practice suggests) — what’s the reason you want them
> > > shorter?
> > 
> > The description part is limited to 75 characters (per line) but shortlog is
> > supposed to be 50 chars. If you see `git log` output in a 80-chars terminal,
> > you'll see the rationale for it. Read under "DISCUSSION" in "git commit"
> > manpage.
> 
> `git log` and `git shortlog` look fine for me in an 80-character terminal.
> What options do you pass to `git log` for it to break? With the options I
> commonly use (`git log --graph --decorate --pretty=oneline
> --abbrev-commit`), 10 characters are lost to overhead on each line, leaving
> ~70 for the summary.
> 
> I can find plenty of places which say ‘use 50 characters for the summary’,
> but nowhere which actually gives a rationale for *why*.

Hm.. How about mutt (or any other cmdline email client)? There is space needed for "Subject: " but that's only 9 characters so not sure. TBH I have never questioned this and it being part of official `git commit` manpage has been good enough for me.

Having said that, I take it as a guideline and only ask people to try to adhere to it. As you can see in here: https://wiki.gnome.org/Git/CommitMessages , we say "Be sure to not exceed 72 characters. Try to not exceed 50." in the Checklist. So when it's possible, just follow the general conventions. :)
Comment 85 Philip Withnall 2016-11-10 09:58:07 UTC
(In reply to Zeeshan Ali (Khattak) from comment #84)
> Hm.. How about mutt (or any other cmdline email client)? There is space
> needed for "Subject: " but that's only 9 characters so not sure. TBH I have
> never questioned this and it being part of official `git commit` manpage has
> been good enough for me.

E-mail based transmission of patches might also need space for “[PATCH 01/20] ” in the subject, which is another 14 characters, so that might be where the recommendation comes from.

GNOME doesn’t use e-mail transmission of patches, though, so the recommendation still seems completely baseless to me. Unless you really really want me to, I don’t want to spend time rewording my shortlogs to shorten them.

What remaining issues are there to fix in the code? Most of the subsequent commits are small fixes, so should be fast and easy to review. :-)
Comment 86 Zeeshan Ali 2016-11-10 10:05:18 UTC
(In reply to Philip Withnall from comment #85)
> (In reply to Zeeshan Ali (Khattak) from comment #84)
> > Hm.. How about mutt (or any other cmdline email client)? There is space
> > needed for "Subject: " but that's only 9 characters so not sure. TBH I have
> > never questioned this and it being part of official `git commit` manpage has
> > been good enough for me.
> 
> E-mail based transmission of patches might also need space for “[PATCH
> 01/20] ” in the subject, which is another 14 characters, so that might be
> where the recommendation comes from.
> 
> GNOME doesn’t use e-mail transmission of patches, though, so the
> recommendation still seems completely baseless to me. Unless you really
> really want me to, I don’t want to spend time rewording my shortlogs to
> shorten them.
> 
> What remaining issues are there to fix in the code? Most of the subsequent
> commits are small fixes, so should be fast and easy to review. :-)

Sure fine, just keep them under 74 characters then.
Comment 87 Philip Withnall 2016-11-10 10:42:10 UTC
(In reply to Zeeshan Ali (Khattak) from comment #86)
> Sure fine, just keep them under 74 characters then.

Yup, definitely. I think all of the shortlogs so far are under 74 characters (longest is 71).
Comment 88 Lasse Schuirmann 2016-11-10 11:35:01 UTC
Hi, jumping in as suggested from Zeeshan because I'm explaining the ruling of commit message at least once a day to newcomers at coala nowadays:

From https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c :

- The 72 is just to keep the text centered when viewing in an 80 char terminal with 4 spaces indentation.
- The 50 is just a good common practice.

I really like the 50 char *rule* (not guideline, I use it as a rule at coala, 51 chars is not allowed, hard. Gets rejected by our bot): it's like tweeting. It forces you to think about redundancies and keeps your commit summary short. Compress most meaning into those 50 chars. (And yeah, according to that blog post 50 chars seems to be the best length.) What good is a summary if it's not short and easy to read? That's also why you want imperative tense there.

Cheers!
Comment 89 Philip Withnall 2016-11-10 12:05:54 UTC
(In reply to Lasse Schuirmann from comment #88)
> Hi, jumping in as suggested from Zeeshan because I'm explaining the ruling
> of commit message at least once a day to newcomers at coala nowadays:
> 
> From
> https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c :
> 
> - The 72 is just to keep the text centered when viewing in an 80 char
> terminal with 4 spaces indentation.
> - The 50 is just a good common practice.
> 
> I really like the 50 char *rule* (not guideline, I use it as a rule at
> coala, 51 chars is not allowed, hard. Gets rejected by our bot): it's like
> tweeting. It forces you to think about redundancies and keeps your commit
> summary short. Compress most meaning into those 50 chars. (And yeah,
> according to that blog post 50 chars seems to be the best length.) What good
> is a summary if it's not short and easy to read? That's also why you want
> imperative tense there.

Well, why don’t we make people summarise even better, and set the limit at 20 characters then? More succinct is obviously always better.

This is rubbish. The tools work well with a limit of ~70 characters, and reading an extra 20 characters is not going to tax anyone. The extra characters gives more scope for including relevant class or method names in the summary, which can make it a lot clearer what has changed, without forcing developers to unnecessarily abbreviate those identifiers.

“just a good common practice” is not an argument in support of 50-character limits. It’s an unsubstantiated opinion.

Can we please not sidetrack this bug any further? Zeeshan has kindly agreed to go with the shortlogs on these patches as they stand.
Comment 90 Philip Withnall 2016-11-17 17:17:38 UTC
Created attachment 340143 [details] [review]
geocode-glib: Add a backend abstraction

Add an abstraction for backends, the GeocodeBackend interface, and split
the existing Nominatim code out into an implementation of that,
GeocodeNominatim. Parameterise this implementation so the default
instance uses nominatim.gnome.org, but other instances can be created
which use other Nominatim servers.

Rebase GeocodeForward and GeocodeReverse on GeocodeBackend so that they
use the new abstraction, and can have their backends changed to use a
custom GeocodeBackend instance. The default is the global GNOME
GeocodeNominatim instance.

The GeocodeNominatim class uses the same Nominatim code as before, moved
from GeocodeForward and GeocodeReverse. In order to be backend-agnostic,
the parameters accepted by GeocodeBackend methods use the XEP-0080 keys
and GValues, rather than anything specific to Nominatim, which means that
some of the parameter-handling code had to be reworked slightly.

GeocodeNominatim delegates its network requests to virtual methods — the
default implementations of these use libsoup as before, but a follow-up
commit will add a derived class which tests the Nominatim implementation
by overriding them to check the generated network requests.

This adds the following new API, but does not break backwards
compatibility:
 • GeocodeBackend
 • GeocodeNominatim
 • geocode_[forward|reverse]_set_backend()

This requires the recently-bumped GIO dependency of 2.44.

Largely based on a patch by Aleksander Morgado
<aleksander.morgado@collabora.co.uk>.
Comment 91 Philip Withnall 2016-11-17 17:21:12 UTC
Updated version of the patch which adds the new headers to geocode-glib.h.
Comment 92 Philip Withnall 2016-11-23 10:14:33 UTC
Any chance of a review of this (and the various other patches I’ve put on other bugs) soon? :-)
Comment 93 Zeeshan Ali 2016-11-23 15:43:36 UTC
(In reply to Philip Withnall from comment #92)
> Any chance of a review of this (and the various other patches I’ve put on
> other bugs) soon? :-)

Unfortunately I'll be pretty busy in the coming weeks. But maybe I can do a live review this weekend in Berlin if you buy me a beer? :)
Comment 94 Zeeshan Ali 2016-11-23 15:44:41 UTC
(In reply to Zeeshan Ali (Khattak) from comment #93)
> (In reply to Philip Withnall from comment #92)
> > Any chance of a review of this (and the various other patches I’ve put on
> > other bugs) soon? :-)
> 
> Unfortunately I'll be pretty busy in the coming weeks. But maybe I can do a
> live review this weekend in Berlin if you buy me a beer? :)

Ah you're not there, sorry. For some reason I remembered seeing your name on the Core Apps hackfest wikipage.
Comment 95 Philip Withnall 2016-11-23 15:54:45 UTC
(In reply to Zeeshan Ali (Khattak) from comment #94)
> (In reply to Zeeshan Ali (Khattak) from comment #93)
> > (In reply to Philip Withnall from comment #92)
> > > Any chance of a review of this (and the various other patches I’ve put on
> > > other bugs) soon? :-)
> > 
> > Unfortunately I'll be pretty busy in the coming weeks. But maybe I can do a
> > live review this weekend in Berlin if you buy me a beer? :)
> 
> Ah you're not there, sorry. For some reason I remembered seeing your name on
> the Core Apps hackfest wikipage.

Indeed, I will not be there, sorry. :-(  I could buy you a beer next time I see you though, regardless.
Comment 96 Frédéric Dalleau 2017-01-03 14:01:38 UTC
Hello Philip, I have applied the patches, but it didn't apply properly in order. I have fixed that and uploaded the branch at github https://github.com/Thread974/geocode-glib/. But if you have something newer that would be nice.

I haven't checked the two additional issues yet: 756313 and 774631, so if you can point me to a branch :)
Comment 97 Philip Withnall 2017-01-03 20:52:05 UTC
(In reply to Frédéric Dalleau from comment #96)
> But if you have something newer
> that would be nice.

https://git.collabora.com/cgit/user/pwith/geocode-glib.git/log/?h=backend-interface

is what I have; but the mock-backend branch (below) is what I consider my HEAD.

> I haven't checked the two additional issues yet: 756313 and 774631, so if
> you can point me to a branch :)

Bug #756313: https://git.collabora.com/cgit/user/pwith/geocode-glib.git/log/?h=user-agent

Bug #774631: https://git.collabora.com/cgit/user/pwith/geocode-glib.git/log/?h=mock-backend
Comment 98 Bastien Nocera 2017-01-04 13:25:22 UTC
Review of attachment 337631 [details] [review]:

::: geocode-glib/Makefile.am
@@ +115,3 @@
+	G_TEST_SRCDIR="$(abs_srcdir)" \
+	G_TEST_BUILDDIR="$(abs_builddir)" \
+	G_DEBUG=gc-friendly \

Would have preferred those last 3 in a separate patch.

@@ +127,3 @@
 test_gcglib_LDADD = libgeocode-glib.la $(GEOCODE_LIBS)
 
+EXTRA_DIST += \

Should the json files be in a separate directory?

::: geocode-glib/test-gcglib.c
@@ +139,3 @@
+	g_autoptr (GError) error = NULL;
+
+	expected_response_path = g_test_build_filename (G_TEST_DIST, "tests",

This is already autofreed by the test engine, so no need to make it "g_autofree".

::: geocode-glib/tests/geocode-nominatim-test.c
@@ +1,2 @@
+/*
+   Copyright (C) 2016 Collabora Ltd.

Please put the "*" on each line too.

::: geocode-glib/tests/geocode-nominatim-test.h
@@ +1,2 @@
+/*
+   Copyright (C) 2016 Collabora Ltd.

Ditto about the "*".
Comment 99 Bastien Nocera 2017-01-04 13:26:29 UTC
Review of attachment 337632 [details] [review]:

::: geocode-glib/test-gcglib.c
@@ +1,2 @@
+/*
+   Copyright (C) 2011, 2012, 2015 Bastien Nocera

Copyright Red Hat actually.

Missing the "*"

@@ +1,3 @@
+/*
+   Copyright (C) 2011, 2012, 2015 Bastien Nocera
+   Copyright (C) 2013, 2014, 2015 Zeeshan Ali

Can be merged into the above.

::: geocode-glib/test-geouri.c
@@ +1,2 @@
+/*
+   Copyright (C) 2013, 2014 Jonas Danielsson

"*".
Comment 100 Bastien Nocera 2017-01-04 13:27:50 UTC
Review of attachment 337633 [details] [review]:

::: geocode-glib/test-gcglib.c
@@ +38,3 @@
 static GMainLoop *loop = NULL;
 static gboolean enable_network = FALSE;
+static char **cl_params = NULL;

command_line_params seems more readable to me.
Comment 101 Bastien Nocera 2017-01-04 13:28:27 UTC
Review of attachment 337634 [details] [review]:

Sure.
Comment 102 Bastien Nocera 2017-01-04 13:29:38 UTC
Review of attachment 337635 [details] [review]:

++
Comment 103 Bastien Nocera 2017-01-04 13:32:02 UTC
Review of attachment 337636 [details] [review]:

Why would it warn? There's a default handler already.

Not sure why we don't return directly from this function instead of setting a variable to return later either.
Comment 104 Bastien Nocera 2017-01-04 13:32:43 UTC
Review of attachment 337637 [details] [review]:

Sure.
Comment 105 Bastien Nocera 2017-01-04 13:33:07 UTC
Review of attachment 337638 [details] [review]:

Yep
Comment 106 Bastien Nocera 2017-01-04 13:33:33 UTC
Review of attachment 337639 [details] [review]:

Sure.
Comment 107 Bastien Nocera 2017-01-04 13:34:13 UTC
Review of attachment 337640 [details] [review]:

Sure.
Comment 108 Bastien Nocera 2017-01-04 13:36:51 UTC
Review of attachment 337641 [details] [review]:

Yep.
Comment 109 Bastien Nocera 2017-01-04 13:37:24 UTC
Review of attachment 337642 [details] [review]:

Fine by me.
Comment 110 Bastien Nocera 2017-01-04 13:39:12 UTC
Review of attachment 337643 [details] [review]:

Too late :/
Please fix the commit message to mention the ID of the original commit.
Comment 111 Bastien Nocera 2017-01-04 13:39:46 UTC
Review of attachment 337644 [details] [review]:

Yep.
Comment 112 Bastien Nocera 2017-01-04 13:43:32 UTC
Review of attachment 337645 [details] [review]:

Sure.
Comment 113 Bastien Nocera 2017-01-04 13:45:05 UTC
Review of attachment 337646 [details] [review]:

Isn't setting XDG_CACHE_HOME to another value enough in this case?
Comment 114 Bastien Nocera 2017-01-04 13:45:36 UTC
Review of attachment 337647 [details] [review]:

++
Comment 115 Bastien Nocera 2017-01-04 13:46:10 UTC
Review of attachment 337648 [details] [review]:

Nice.
Comment 116 Bastien Nocera 2017-01-04 13:49:38 UTC
Review of attachment 337649 [details] [review]:

Sure.

::: geocode-glib/geocode-backend.c
@@ +233,3 @@
+ * This is a synchronous function, which means it may block on network requests.
+ * In most situations, the asynchronous version
+ * (geocode_backend_forward_search_async()) is more appropriate. See its

Using some commas here would avoid the "))" closing.
Comment 117 Bastien Nocera 2017-01-04 13:50:37 UTC
Review of attachment 339339 [details] [review]:

Make sure to commit this early in the patchset.
Comment 118 Bastien Nocera 2017-01-04 13:51:46 UTC
Review of attachment 339340 [details] [review]:

Bump it to 3.23.1 and make sure to have a follow-up commit which changes the "UNRELEASED" tags in the docs.
Comment 119 Bastien Nocera 2017-01-04 13:57:17 UTC
Review of attachment 340143 [details] [review]:

I would rather this had been more abstract, so that implementation backend isn't propagated, but we already have backend selection in the API, so that's not really any worse...

::: geocode-glib/geocode-backend.c
@@ +1,2 @@
+/*
+  Copyright (C) 2016 Collabora Ltd.

"*".
Comment 120 Frédéric Dalleau 2017-01-05 06:30:36 UTC
Philip, Bastien, thanks for review and git branches.
Comment 121 Philip Withnall 2017-01-06 23:13:01 UTC
Attachment 337626 [details] pushed as 8607f7d - test-gcglib: Update expected test results to match latest OSM data
Attachment 337627 [details] pushed as 3bb0290 - test-gcglib: Minor const-correctness fix
Attachment 337628 [details] pushed as a5f1e32 - test-gcglib: Fix a couple of memory leaks in the tests
Attachment 337629 [details] pushed as 53b29e7 - test-gcglib: Add a missing error assertion
Attachment 337634 [details] pushed as b3cb332 - docs: Fix multiple declarations of the enums documentation section
Attachment 337635 [details] pushed as 99f6d79 - geocode-glib: Fix various minor const-correctness issues
Comment 122 Philip Withnall 2017-01-06 23:52:22 UTC
Created attachment 343041 [details] [review]
build: Bump GIO dependency to 2.44

This will be needed in the following commits so that new features like
g_set_object() and g_autoptr() can be used. 2.44 was released on
2015-03-23.
Comment 123 Philip Withnall 2017-01-06 23:52:29 UTC
Created attachment 343042 [details] [review]
build: Bump version to 3.23.1

Bump to a version in the 3.23.x unstable series, in line with the rest
of GNOME. Do this before the following commits add a load of new API.
(This commit is not itself a release.)
Comment 124 Philip Withnall 2017-01-06 23:52:37 UTC
Created attachment 343043 [details] [review]
geocode-glib: Add a backend abstraction

Add an abstraction for backends, the GeocodeBackend interface, and split
the existing Nominatim code out into an implementation of that,
GeocodeNominatim. Parameterise this implementation so the default
instance uses nominatim.gnome.org, but other instances can be created
which use other Nominatim servers.

Rebase GeocodeForward and GeocodeReverse on GeocodeBackend so that they
use the new abstraction, and can have their backends changed to use a
custom GeocodeBackend instance. The default is the global GNOME
GeocodeNominatim instance.

The GeocodeNominatim class uses the same Nominatim code as before, moved
from GeocodeForward and GeocodeReverse. In order to be backend-agnostic,
the parameters accepted by GeocodeBackend methods use the XEP-0080 keys
and GValues, rather than anything specific to Nominatim, which means that
some of the parameter-handling code had to be reworked slightly.

GeocodeNominatim delegates its network requests to virtual methods — the
default implementations of these use libsoup as before, but a follow-up
commit will add a derived class which tests the Nominatim implementation
by overriding them to check the generated network requests.

This adds the following new API, but does not break backwards
compatibility:
 • GeocodeBackend
 • GeocodeNominatim
 • geocode_[forward|reverse]_set_backend()

This requires the recently-bumped GIO dependency of 2.44.

Largely based on a patch by Aleksander Morgado
<aleksander.morgado@collabora.co.uk>.
Comment 125 Philip Withnall 2017-01-06 23:52:47 UTC
Created attachment 343044 [details] [review]
tests: Port test-gcglib to optionally use GeocodeNominatim offline

GeocodeNominatim allows us to override its virtual methods for network
queries, so do that in a new GeocodeNominatimTest subclass, which loads
its results from a local set of results added by the test harness.

Port the existing test-gcglib tests to optionally use this new backend,
rather than the default GNOME Nominatim backend, to run the tests
offline. This means that they can reliably be run on CI or build
infrastructure with no internet access; and can be run with internet
access on developer machines (for example).

Based on a patch by Aleksander Morgado
<aleksander.morgado@collabora.co.uk>.
Comment 126 Philip Withnall 2017-01-06 23:52:54 UTC
Created attachment 343045 [details] [review]
build: Add perturbation variables to test environment

These should perturb the memory allocator settings randomly so that
memory management problems are more likely to be highlighted.
Comment 127 Philip Withnall 2017-01-06 23:53:01 UTC
Created attachment 343046 [details] [review]
tests: Add copyright headers to test C files

These have been missing for a while. Copyrights reconstructed from the
git log; Bastien and Zeeshan’s copyrights were attributed to Red Hat as
part of the standard Red Hat employment contract.
Comment 128 Philip Withnall 2017-01-06 23:53:09 UTC
Created attachment 343047 [details] [review]
tests: Rename a global variable to be less generic

Rename the commmand-line parameters variable to
‘command_line_params’ so that it isn’t shadowed by a huge number
of other variables in the file.
Comment 129 Philip Withnall 2017-01-06 23:53:16 UTC
Created attachment 343048 [details] [review]
geocode-place: Fix -Wswitch-enum warnings

This switch is not meant to handle all of the values for the switch
(omissions are not a bug), so cast the value to an int to disable type
checking first.
Comment 130 Philip Withnall 2017-01-06 23:53:25 UTC
Created attachment 343049 [details] [review]
geocode-place: Simplify switch statement

We return immediately at the bottom of the switch statement, so why not
return immediately from each case?
Comment 131 Philip Withnall 2017-01-06 23:53:32 UTC
Created attachment 343050 [details] [review]
build: Update to latest GNOME autogen.sh

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 132 Philip Withnall 2017-01-06 23:53:42 UTC
Created attachment 343051 [details] [review]
build: Port from GNOME_DEBUG_CHECK to AX_CHECK_ENABLE_DEBUG

This is one step towards no longer depending on gnome-common.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 133 Philip Withnall 2017-01-06 23:53:50 UTC
Created attachment 343052 [details] [review]
build: Drop use of GNOME_MAINTAINER_MODE_DEFINES

It’s no longer useful for modern GNOME modules.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 134 Philip Withnall 2017-01-06 23:53:57 UTC
Created attachment 343053 [details] [review]
build: Port from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS

AX_COMPILER_FLAGS uses a standard set of warnings across all projects,
and a standard build API for enabling and disabling whether they are
warnings or errors.

Drop GNOME_CXX_WARNINGS because the project doesn’t use C++.

https://wiki.gnome.org/Projects/GnomeCommon/Migration
Comment 135 Philip Withnall 2017-01-06 23:54:05 UTC
Created attachment 343054 [details] [review]
build: Add AX_IS_RELEASE

Automatically turn on or off various release-dependent configure options
based on whether a git directory is present. If it’s present, we assume
we’re doing development, so fatal warnings (etc.) are turned on.
Otherwise, we assume it’s a release and they are turned off.
Comment 136 Philip Withnall 2017-01-06 23:54:12 UTC
Created attachment 343055 [details] [review]
build: Use AM_CPPFLAGS rather than INCLUDES

INCLUDES is deprecated and will be removed in a future version of
automake. This fixes an automake warning.
Comment 137 Philip Withnall 2017-01-06 23:54:20 UTC
Created attachment 343056 [details] [review]
geocode-glib: Make some private functions static

These two functions are no longer needed in any files other than
geocode-nominatim.c, so they can be made private and removed from the
.symbols file.
Comment 138 Philip Withnall 2017-01-06 23:54:28 UTC
Created attachment 343057 [details] [review]
geocode-backend: Add support for multiple reverse results

Some geocoding APIs (such as the Google Geocoding API) can return
multiple results for a reverse resolution request, such as the building
at that location, the street it is on, the town it is in, etc.

While we currently only support Nominatim, other backends may be added
(potentially out of tree) in future. Change the GeocodeBackend API to
allow them to return multiple reverse results.

This does not change the GeocodeReverse API, which returns the first
result in the list, which should be the most relevant one.

https://developers.google.com/maps/documentation/geocoding/intro#ReverseGeocoding
Comment 139 Philip Withnall 2017-01-06 23:54:36 UTC
Created attachment 343058 [details] [review]
geocode-nominatim: Allow the cache path to be adjusted

Instead of always using the cache in $XDG_CACHE_HOME/geocode-glib, allow
the path to be customised, and set it to a temporary directory while
running the tests so we don’t accidentally use (or pollute) the user’s
actual cache.
Comment 140 Philip Withnall 2017-01-06 23:54:43 UTC
Created attachment 343059 [details] [review]
geocode-glib: Fix documentation for list return values

The documentation previously said that the return values were (transfer
container), but they were actually always (transfer full) (and all
callers in the tests treated them as such). Fix that in the documentation.
Comment 141 Philip Withnall 2017-01-06 23:54:52 UTC
Created attachment 343060 [details] [review]
tests: Fix some minor memory leaks in test-gcglib

It’s now memory-leak-clean when run under valgrind.
Comment 142 Philip Withnall 2017-01-06 23:55:00 UTC
Created attachment 343061 [details] [review]
geocode-glib: Accept a hash table for GeocodeBackend.reverse_resolve()

Instead of only accepting a GeocodeLocation, accept a hash table of keys
and GValues. Typically, this will contain `lat` and `lon` entries which
represent the GeocodeLocation. But in future, it could contain other
keys to look up a GeocodePlace by its backend-specific ID, for example;
or to limit the search results by place type.

This changes the API in GeocodeBackend (which is not stable yet), but
not in GeocodeReverse, so it’s not an API break.
Comment 143 Philip Withnall 2017-01-06 23:55:08 UTC
Created attachment 343062 [details] [review]
geocode-glib: Mark UNRELEASED API as new in 3.23.1

3.23.1 has not been released yet, but this API is definitely going to be
in it when it is.
Comment 144 Philip Withnall 2017-01-07 00:00:08 UTC
(In reply to Bastien Nocera from comment #98)
> Review of attachment 337631 [details] [review] [review]:
> 
> ::: geocode-glib/Makefile.am
> @@ +115,3 @@
> +	G_TEST_SRCDIR="$(abs_srcdir)" \
> +	G_TEST_BUILDDIR="$(abs_builddir)" \
> +	G_DEBUG=gc-friendly \
> 
> Would have preferred those last 3 in a separate patch.

Split out into the next patch.

> @@ +127,3 @@
>  test_gcglib_LDADD = libgeocode-glib.la $(GEOCODE_LIBS)
>  
> +EXTRA_DIST += \
> 
> Should the json files be in a separate directory?

I don’t see any need for it; there aren’t that many of them.

> ::: geocode-glib/test-gcglib.c
> @@ +139,3 @@
> +	g_autoptr (GError) error = NULL;
> +
> +	expected_response_path = g_test_build_filename (G_TEST_DIST, "tests",
> 
> This is already autofreed by the test engine, so no need to make it
> "g_autofree".

Nope: that’s g_test_get_filename(). I’d prefer to stick with g_test_build_filename() as it makes the memory management more explicit rather than it secretly being freed by the test harness.

> ::: geocode-glib/tests/geocode-nominatim-test.c
> @@ +1,2 @@
> +/*
> +   Copyright (C) 2016 Collabora Ltd.
> 
> Please put the "*" on each line too.

Done. I copied the copyright header format from somewhere else in the project and was too scared to change it.

> ::: geocode-glib/tests/geocode-nominatim-test.h
> @@ +1,2 @@
> +/*
> +   Copyright (C) 2016 Collabora Ltd.
> 
> Ditto about the "*".

Done.

(In reply to Bastien Nocera from comment #99)
> Review of attachment 337632 [details] [review] [review]:
> 
> ::: geocode-glib/test-gcglib.c
> @@ +1,2 @@
> +/*
> +   Copyright (C) 2011, 2012, 2015 Bastien Nocera
> 
> Copyright Red Hat actually.
> 
> Missing the "*"

Both done.

> @@ +1,3 @@
> +/*
> +   Copyright (C) 2011, 2012, 2015 Bastien Nocera
> +   Copyright (C) 2013, 2014, 2015 Zeeshan Ali
> 
> Can be merged into the above.

OK.

> ::: geocode-glib/test-geouri.c
> @@ +1,2 @@
> +/*
> +   Copyright (C) 2013, 2014 Jonas Danielsson
> 
> "*".

OK.

(In reply to Bastien Nocera from comment #100)
> Review of attachment 337633 [details] [review] [review]:
> 
> ::: geocode-glib/test-gcglib.c
> @@ +38,3 @@
>  static GMainLoop *loop = NULL;
>  static gboolean enable_network = FALSE;
> +static char **cl_params = NULL;
> 
> command_line_params seems more readable to me.

Good idea, fixed.

(In reply to Bastien Nocera from comment #103)
> Review of attachment 337636 [details] [review] [review]:
> 
> Why would it warn? There's a default handler already.

That’s -Wswitch (doesn’t warn if a default case exists). -Wswitch-enums warns even if a default case exists, on the assumption that the default case probably doesn’t do the right thing for all enum values.

> Not sure why we don't return directly from this function instead of setting
> a variable to return later either.

I’ll do a follow-up commit to tidy that up.

(In reply to Bastien Nocera from comment #110)
> Review of attachment 337643 [details] [review] [review]:
> 
> Too late :/
> Please fix the commit message to mention the ID of the original commit.

I’d actually already squashed it in before pushing in the first place, so everything’s fine here. I just forgot to mark the fixup commit as obsolete, oops.

(In reply to Bastien Nocera from comment #113)
> Review of attachment 337646 [details] [review] [review]:
> 
> Isn't setting XDG_CACHE_HOME to another value enough in this case?

It would be, but I don’t like the idea of messing around in a global namespace (environment variables) in order to change configuration which should be local to one object (a GeocodeNominatim instance).

(In reply to Bastien Nocera from comment #116)
> Review of attachment 337649 [details] [review] [review]:
> 
> Sure.
> 
> ::: geocode-glib/geocode-backend.c
> @@ +233,3 @@
> + * This is a synchronous function, which means it may block on network
> requests.
> + * In most situations, the asynchronous version
> + * (geocode_backend_forward_search_async()) is more appropriate. See its
> 
> Using some commas here would avoid the "))" closing.

Done.

(In reply to Bastien Nocera from comment #117)
> Review of attachment 339339 [details] [review] [review]:
> 
> Make sure to commit this early in the patchset.

Yes, it’s the first one which is still uncommitted. Looks like the patches got a bit out of order in Bugzilla when I selectively updated some of them.

(In reply to Bastien Nocera from comment #118)
> Review of attachment 339340 [details] [review] [review]:
> 
> Bump it to 3.23.1 and make sure to have a follow-up commit which changes the
> "UNRELEASED" tags in the docs.

You want to skip 3.23.0 entirely? OK.

I’ve added the follow-up commit at the end of the patch series in this bug. I’ve marked all the patches which have not changed since you reviewed and accepted them as a_c-n, and left the rest as unreviewed. Thanks for the reviews!
Comment 145 Philip Withnall 2017-01-07 00:08:14 UTC
Since I no longer have access to the repository on git.collabora.com, I’ve pushed the latest versions of the branches here:
 • https://gitlab.com/pwithnall/geocode-glib/commits/backend-interfacehttps://gitlab.com/pwithnall/geocode-glib/commits/user-agenthttps://gitlab.com/pwithnall/geocode-glib/commits/installed-testshttps://gitlab.com/pwithnall/geocode-glib/commits/mock-backend

They’ve all been rebased on top of the changes here, but I will not upload new versions of the patches in bug 756313, bug 772928 or bug 774631 until all the patches here have landed, or they’ll just end up being rebased again and again.

Frédéric, it would be helpful if you could delete those stale branches from git.collabora.com (or get a sysadmin to do so, if you don’t have write access to my old home directory), so they don’t end up confusing someone. Thanks.
Comment 146 Bastien Nocera 2017-01-10 13:46:56 UTC
Review of attachment 343041 [details] [review]:

As mentioned earlier: Make sure to commit this early in the patchset
Comment 147 Bastien Nocera 2017-01-10 13:47:58 UTC
Review of attachment 343042 [details] [review]:

++
Comment 148 Bastien Nocera 2017-01-10 13:49:17 UTC
Review of attachment 343043 [details] [review]:

Yep.
Comment 149 Bastien Nocera 2017-01-10 13:50:15 UTC
Review of attachment 343044 [details] [review]:

Yep.
Comment 150 Bastien Nocera 2017-01-10 13:51:40 UTC
Review of attachment 343045 [details] [review]:

++
Comment 151 Bastien Nocera 2017-01-10 13:52:21 UTC
Review of attachment 343046 [details] [review]:

Yes.
Comment 152 Bastien Nocera 2017-01-10 13:52:58 UTC
Review of attachment 343047 [details] [review]:

Easier to read.
Comment 153 Bastien Nocera 2017-01-10 13:53:41 UTC
Review of attachment 343048 [details] [review]:

As discussed.
Comment 154 Bastien Nocera 2017-01-10 13:54:15 UTC
Review of attachment 343049 [details] [review]:

Nice cleanup.
Comment 155 Bastien Nocera 2017-01-10 13:56:59 UTC
Review of attachment 343058 [details] [review]:

I still don't see why this is generally useful. It might be interesting to test the cache of nominatim, in which case it only affects the tests, and we can use the big hammer, or it's generally useful, and I'd like to know what the use cases are.
Comment 156 Bastien Nocera 2017-01-10 13:59:18 UTC
Review of attachment 343061 [details] [review]:

> Instead of only accepting a GeocodeLocation, accept a hash table of keys
> and GValues

GValues is a type, but "keys" isn't. Maybe strings? static strings?

Looks good otherwise.
Comment 157 Bastien Nocera 2017-01-10 13:59:48 UTC
Review of attachment 343062 [details] [review]:

Thanks.
Comment 158 Philip Withnall 2017-01-10 23:02:36 UTC
Review of attachment 343058 [details] [review]:

There would be a use case for maintaining privacy if an application needs to do geocoding lookups in several different privacy domains (for example, a web browser doing geocoding lookups in different private browsing tabs). The caches for such domains would have to be kept separate, but since it’s all in the same process, XDG_CACHE_HOME could not be changed for all of them.

Another use case would be a utility which pre-downloads the geocoding results for a set of places or locations, and stores the cached data somewhere for later use offline.

Note that when I added this, I was only thinking about the unit tests (and my hatred of environment variables), so these use cases are reverse engineered.
Comment 159 Philip Withnall 2017-01-10 23:04:39 UTC
(In reply to Bastien Nocera from comment #156)
> Review of attachment 343061 [details] [review] [review]:
> 
> > Instead of only accepting a GeocodeLocation, accept a hash table of keys
> > and GValues
> 
> GValues is a type, but "keys" isn't. Maybe strings? static strings?
> 
> Looks good otherwise.

The keys are strings, and it’s up to the allocator of the GHashTable as to whether they are static (no key free function passed to g_hash_table_new_full()) or non-static (free function).

I’ll update the commit message before pushing.
Comment 160 Bastien Nocera 2017-01-11 10:44:48 UTC
(In reply to Philip Withnall from comment #158)
> Review of attachment 343058 [details] [review] [review]:
> 
> There would be a use case for maintaining privacy if an application needs to
> do geocoding lookups in several different privacy domains (for example, a
> web browser doing geocoding lookups in different private browsing tabs).

In this case we'd need a way to avoid doing lookups altogether, or not to save as caches, not a way to save elsewhere.

> The
> caches for such domains would have to be kept separate, but since it’s all
> in the same process, XDG_CACHE_HOME could not be changed for all of them.
> 
> Another use case would be a utility which pre-downloads the geocoding
> results for a set of places or locations, and stores the cached data
> somewhere for later use offline.

You'd need to instrument the sources of that utility anyway.

> Note that when I added this, I was only thinking about the unit tests (and
> my hatred of environment variables), so these use cases are reverse
> engineered.

Yes, it looks like they are. Still not convinced. Can you please stick this patch in a separate bug, and if Jonas likes it better than me, or better use cases appear, we can reassess. Thanks!
Comment 161 Philip Withnall 2017-01-12 23:32:40 UTC
Created attachment 343393 [details] [review]
tests: Set XDG_CACHE_HOME to a temporary directory for tests

We don’t want to pollute the user’s actual cache, or to re-use cached
results between tests.
Comment 162 Philip Withnall 2017-01-12 23:37:46 UTC
Comment on attachment 343058 [details] [review]
geocode-nominatim: Allow the cache path to be adjusted

The new patch goes in place of this one in the patch series. Some of the subsequent patches needed minor rebasing to apply correctly, but not enough to require re-review.

Filed https://bugzilla.gnome.org/show_bug.cgi?id=777196 with the dropped cache patch.
Comment 163 Bastien Nocera 2017-01-13 10:53:49 UTC
Review of attachment 343393 [details] [review]:

Good.
Comment 164 Philip Withnall 2017-01-13 23:12:18 UTC
Boom, done. Thanks for the reviews Bastien.

Let’s start on bug #756313 next? :-)

Attachment 343041 [details] pushed as 591b0ee - build: Bump GIO dependency to 2.44
Attachment 343042 [details] pushed as 4b6bb1f - build: Bump version to 3.23.1
Attachment 343043 [details] pushed as af4bac3 - geocode-glib: Add a backend abstraction
Attachment 343044 [details] pushed as 65dc183 - tests: Port test-gcglib to optionally use GeocodeNominatim offline
Attachment 343045 [details] pushed as 14b9de6 - build: Add perturbation variables to test environment
Attachment 343046 [details] pushed as 8816352 - tests: Add copyright headers to test C files
Attachment 343047 [details] pushed as 5bca002 - tests: Rename a global variable to be less generic
Attachment 343048 [details] pushed as b088742 - geocode-place: Fix -Wswitch-enum warnings
Attachment 343049 [details] pushed as 7d9130a - geocode-place: Simplify switch statement
Attachment 343050 [details] pushed as bd97dc6 - build: Update to latest GNOME autogen.sh
Attachment 343051 [details] pushed as eaeb4cb - build: Port from GNOME_DEBUG_CHECK to AX_CHECK_ENABLE_DEBUG
Attachment 343052 [details] pushed as 2e925be - build: Drop use of GNOME_MAINTAINER_MODE_DEFINES
Attachment 343053 [details] pushed as e5fc19d - build: Port from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS
Attachment 343054 [details] pushed as 0e2efdb - build: Add AX_IS_RELEASE
Attachment 343055 [details] pushed as 491dcd4 - build: Use AM_CPPFLAGS rather than INCLUDES
Attachment 343056 [details] pushed as a2306cb - geocode-glib: Make some private functions static
Attachment 343057 [details] pushed as 2a8d8e4 - geocode-backend: Add support for multiple reverse results
Attachment 343059 [details] pushed as c4efd69 - geocode-glib: Fix documentation for list return values
Attachment 343060 [details] pushed as f97421f - tests: Fix some minor memory leaks in test-gcglib
Attachment 343061 [details] pushed as 4c27e04 - geocode-glib: Accept a hash table for GeocodeBackend.reverse_resolve()
Attachment 343062 [details] pushed as 1f2766a - geocode-glib: Mark UNRELEASED API as new in 3.23.1
Attachment 343393 [details] pushed as 6e8d348 - tests: Set XDG_CACHE_HOME to a temporary directory for tests