GNOME Bugzilla – Bug 756311
Don’t hard-code Nominatim server as nominatim.gnome.org
Last modified: 2017-01-13 23:14:43 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?
(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.)
(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?
(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?
(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.
(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?
(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.
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?
(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?
> > 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.
(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?
(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?
(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.)
(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.
(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.
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. :-)
That'd be Jonas' decision actually :) But I guess it makes sense.
(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.
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'.
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!
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.
ping?
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?
(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.
(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.
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.
Created attachment 337623 [details] [review] geocode-forward: Add missing ‘Returns’ line to a gtk-doc comment This adds a missing GIR annotation.
Created attachment 337624 [details] [review] geocode-glib: Add g_autoptr() cleanup function definitions This allows us to use GeocodeForward and GeocodeReverse with g_autoptr().
Created attachment 337625 [details] [review] geocode-reverse: Fix indentation
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.
Created attachment 337627 [details] [review] test-gcglib: Minor const-correctness fix
Created attachment 337628 [details] [review] test-gcglib: Fix a couple of memory leaks in the tests
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.
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.
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).
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.
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.
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.
Created attachment 337635 [details] [review] geocode-glib: Fix various minor const-correctness issues This fixes a load of compiler warnings.
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.
Created attachment 337637 [details] [review] build: Update to latest GNOME autogen.sh https://wiki.gnome.org/Projects/GnomeCommon/Migration
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
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
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
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.
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.
Created attachment 337643 [details] [review] fixup! geocode-glib: Add g_autoptr() cleanup function definitions
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.
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
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.
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.
Created attachment 337648 [details] [review] tests: Fix some minor memory leaks in test-gcglib It’s now memory-leak-clean when run under valgrind.
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.
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?
*** Bug 766714 has been marked as a duplicate of this bug. ***
Any review comments?
Review of attachment 337622 [details] [review]: ack
Review of attachment 337623 [details] [review]: ACK
Review of attachment 337624 [details] [review]: ack but please make the shortlog fit under 50 chars since it's possible in this case.
Review of attachment 337625 [details] [review]: ack
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
Review of attachment 337626 [details] [review]: ACK but shortlog seems too long.
Review of attachment 337627 [details] [review]: ack
Review of attachment 337627 [details] [review]: oops, wrong status
Review of attachment 337628 [details] [review]: ack, "in the tests" is redundant in shortlog IMO
Review of attachment 337629 [details] [review]: ack
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.
(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?
(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.
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.
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.)
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>.
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.
(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).
(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.
(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.
(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
(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*.
(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. :)
(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. :-)
(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.
(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).
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!
(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.
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>.
Updated version of the patch which adds the new headers to geocode-glib.h.
Any chance of a review of this (and the various other patches I’ve put on other bugs) soon? :-)
(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? :)
(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.
(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.
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 :)
(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
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 "*".
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 "*".
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.
Review of attachment 337634 [details] [review]: Sure.
Review of attachment 337635 [details] [review]: ++
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.
Review of attachment 337637 [details] [review]: Sure.
Review of attachment 337638 [details] [review]: Yep
Review of attachment 337639 [details] [review]: Sure.
Review of attachment 337640 [details] [review]: Sure.
Review of attachment 337641 [details] [review]: Yep.
Review of attachment 337642 [details] [review]: Fine by me.
Review of attachment 337643 [details] [review]: Too late :/ Please fix the commit message to mention the ID of the original commit.
Review of attachment 337644 [details] [review]: Yep.
Review of attachment 337645 [details] [review]: Sure.
Review of attachment 337646 [details] [review]: Isn't setting XDG_CACHE_HOME to another value enough in this case?
Review of attachment 337647 [details] [review]: ++
Review of attachment 337648 [details] [review]: Nice.
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.
Review of attachment 339339 [details] [review]: Make sure to commit this early in the patchset.
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.
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. "*".
Philip, Bastien, thanks for review and git branches.
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
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.
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.)
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>.
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>.
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.
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.
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.
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.
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?
Created attachment 343050 [details] [review] build: Update to latest GNOME autogen.sh https://wiki.gnome.org/Projects/GnomeCommon/Migration
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
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
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
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.
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.
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.
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
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.
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.
Created attachment 343060 [details] [review] tests: Fix some minor memory leaks in test-gcglib It’s now memory-leak-clean when run under valgrind.
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.
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.
(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!
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-interface • https://gitlab.com/pwithnall/geocode-glib/commits/user-agent • https://gitlab.com/pwithnall/geocode-glib/commits/installed-tests • https://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.
Review of attachment 343041 [details] [review]: As mentioned earlier: Make sure to commit this early in the patchset
Review of attachment 343042 [details] [review]: ++
Review of attachment 343043 [details] [review]: Yep.
Review of attachment 343044 [details] [review]: Yep.
Review of attachment 343045 [details] [review]: ++
Review of attachment 343046 [details] [review]: Yes.
Review of attachment 343047 [details] [review]: Easier to read.
Review of attachment 343048 [details] [review]: As discussed.
Review of attachment 343049 [details] [review]: Nice cleanup.
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.
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.
Review of attachment 343062 [details] [review]: Thanks.
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.
(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.
(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!
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 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.
Review of attachment 343393 [details] [review]: Good.
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