(In reply to comment #0)
> Related to the following bug against "Damned lies":
> https://bugzilla.gnome.org/show_bug.cgi?id=725578
> 
> There is one string marked as translateable in geocode-glib/geocode-reverse.c
> 
> Address formats are country-specific rather than language-dependent.
> Maybe it would be possible to derive this returned address string from the
> LC_ADDRESS locale component?

Sure. How do you use that?

Created attachment 271367 [details] [review]
Determine addressformat automatically

I added a patch that uses nl_langinfo to determine the addressformat.
I have manually tested running gnome-maps against the locally compiled library (using LD_LIBRARY_PATH). Using the patched library I get the streetnumber after the streename in a lookup using an sv_SE.UTF-8 locale, and before the streetname using en_US.UTF-8.
I have also pushed the changes to my github account here:
https://github.com/mlundblad/geocode-glib.git

Created attachment 271419 [details] [review]
Detect nl_langinfo support

Created attachment 271420 [details] [review]
Use nl_langinfo to format address

Created attachment 271421 [details] [review]
Remove from POTFILES.in

Review of attachment 271419 [details] [review]:

ack (. is redundant in commit).

Review of attachment 271420 [details] [review]:

::: geocode-glib/geocode-reverse.c
@@ +28,3 @@
 #include <libsoup/soup.h>
 #include <config.h>
+#ifdef HAVE_LANGINFO_H

I wonder if we really need this? Which platforms langinfo isn't available for/on?

@@ +120,3 @@
 
+static gboolean
+_geocode_is_number_after_street (void)

'_geocode_' prefix is used only for functions that are not static but not part of public API.

@@ +132,3 @@
+
+  if (s != NULL && h != NULL) {
+    /* return true if %h comes after %s in the format string */

The code is almost as readable/explanatory as these comments so I wouldn't include them.

@@ +186,3 @@
+				char *name = g_strdup_printf ("%s %s", 
+					number_after ? value : house_number,
+					number_after ? house_number : value);

you want to align the 'number_after' to first arg here.

Review of attachment 271421 [details] [review]:

ACK otherwise but please try to follow these guidelines for commit logs: https://wiki.gnome.org/Git/CommitMessages

Created attachment 271474 [details] [review]
reverse: clean up patch for locale-dependent address format

Remove unnessesary #ifdef for <langinfo.h>.
Remove redundant comments. Use proper function name for local function.
Some indentation fixes.

Review of attachment 271474 [details] [review]:

Ouch, you don't want an additional patch to correct your previous patch but rather provide new version of the previous patch.

Created attachment 271475 [details] [review]
Determine address format using nl_langinfo.

Created attachment 271476 [details] [review]
Remove file no longer using translated strings.

Created attachment 271477 [details] [review]
reverse: clean up patch for locale-dependent address format

Remove unnessesary #ifdef for <langinfo.h>.
Remove redundant comments. Use proper function name for local function.
Some indentation fixes.

Created attachment 271479 [details] [review]
Add detection of nl_langinfo support.

Determine address format using nl_langinfo.

https://bugzilla.gnome.org/show_bug.cgi?id=725591

Remove file no longer using translated strings.

https://bugzilla.gnome.org/show_bug.cgi?id=725591

reverse: clean up patch for locale-dependent address format

Remove unnessesary #ifdef for <langinfo.h>.
Remove redundant comments. Use proper function name for local function.
Some indentation fixes.

Review of attachment 271479 [details] [review]:

<zeenix> ah, you marked "Remove file no longer using translated strings." obsolete too
<zeenix> mlundblad: thats not obsolete afaict
<zeenix> oh
<zeenix> you did squash everything together :(
<zeenix> <zeenix> mlundblad: you first gotta use `git rebase -i origin/master` and then squash the appropriate patches
<zeenix> appropriate -> the old and new version of the patch
<zeenix> mlundblad: and you also need to merge the commit log rather than appending them all together

Created attachment 271483 [details] [review]
reverse: derive address format from locale.

Determine address format using nl_langinfo.
Remove unnessesary #ifdef for <langinfo.h>.
Remove redundant comments. Use proper function name for local function.
Some indentation fixes.

Patches pushed with minor changes.

Unconditional use of (private!) _NL_ADDRESS_POSTAL_FMT broke the build on FreeBSD.

<desrt> zeenix: it doesn't build on !linux
<zeenix> ah :)
<desrt> and i think it may even break on linux some day
<desrt> since you're using a private symbol
<zeenix> desrt: mlundblad told me its supposed to at least work on posix
<desrt> definitely not...
<desrt> here is the list of nl_langinfo queries that posix supports: http://pubs.opengroup.org/onlinepubs/007904875/basedefs/langinfo.h.html

Created attachment 271611 [details] [review]
reverse: only use _NL_ADDRESS_POSTAL_FMT on glibc

Conditionally check for GNU libc for the _NL_ADDRESS_FMT constant.
Short-circuite to the default address format of housenumber before
street name otherwise.

Review of attachment 271611 [details] [review]:

Well we are still using internal API so we can't be sure it'll even keep working against glibc. Also this means that this will not work at all for non-glibc cases.

I'd rather we just revert the previous patch and get translators to translate this one string.

I mentioned on IRC that I'm not sure that this is internal API on account of the fact that nothing seems to use it internally, aside from the commandline tool.

It could be that this is meant to be a proper GNU extension -- I can't really find documentation about (the consumer side of) it anywhere, though.

Created attachment 271662 [details] [review]
Revert "reverse: Derive address format from locale"

This reverts commit 029c421d32647e4ab6fbbfa669947c24a27e69fe.

Comment on attachment 271662 [details] [review]
Revert "reverse: Derive address format from locale"

Attachment 271662 [details] pushed as 55d7e80 - Revert "reverse: Derive address format from locale"

Reverted with Zeeshan's ACK (via IRC).

Created attachment 272204 [details] [review]
Revert "l10n: Update POTFILES.in"

This reverts commit 684555176c33091a336268870c19a436a1be35bb.

We also need to revert the removal of geocode-reverse from POTFILES.in.

Attachment 272204 [details] pushed as a95f1ba - Revert "l10n: Update POTFILES.in"

(Ack from Zeeshan via chat)

So I am re-opening this. 

I would like this functionality, either if we can find a proper way of getting the postal_fmt field or if we can do something purely based on a list of LOCALE?

Like looking through this list and see if it there could be easy classification: http://lh.2xlibre.net/values/postal_fmt/

(In reply to comment #29)
> So I am re-opening this. 
> 
> I would like this functionality, either if we can find a proper way of getting
> the postal_fmt field or if we can do something purely based on a list of
> LOCALE?

I don't understand the question here. _NL_ADDRESS_POSTAL_FMT should contain all the information necessary to format the address, but the metadata given back by the service might not exactly match the fields used by _NL_ADDRESS_POSTAL_FMT...

What exactly do you expect to happen? Full support for the spec? It looks doable although it probably needs to be cleaned up (look at the number of address formats that don't include a country name for example).

(In reply to comment #31)
> (In reply to comment #29)
> > So I am re-opening this. 
> > 
> > I would like this functionality, either if we can find a proper way of getting
> > the postal_fmt field or if we can do something purely based on a list of
> > LOCALE?
> 
> I don't understand the question here. _NL_ADDRESS_POSTAL_FMT should contain all
> the information necessary to format the address, but the metadata given back by
> the service might not exactly match the fields used by
> _NL_ADDRESS_POSTAL_FMT...
> 
> What exactly do you expect to happen? Full support for the spec? It looks
> doable although it probably needs to be cleaned up (look at the number of
> address formats that don't include a country name for example).

See above, we can not use _NL_ADDRESS_POSTAL_FMT, since it is private API, and I cannot find a proper way to get that postal format string. Only the locale tool uses it, it seemed, and they reference the private API.

This broke FreeBSD. So what I was thinking was getting a list of locales that have the number before the street, or the other way around, whichever is the shorter list. And from that have logic to format the <house number> <street name> pair.

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > So I am re-opening this. 
> > > 
> > > I would like this functionality, either if we can find a proper way of getting
> > > the postal_fmt field or if we can do something purely based on a list of
> > > LOCALE?
> > 
> > I don't understand the question here. _NL_ADDRESS_POSTAL_FMT should contain all
> > the information necessary to format the address, but the metadata given back by
> > the service might not exactly match the fields used by
> > _NL_ADDRESS_POSTAL_FMT...
> > 
> > What exactly do you expect to happen? Full support for the spec? It looks
> > doable although it probably needs to be cleaned up (look at the number of
> > address formats that don't include a country name for example).
> 
> See above, we can not use _NL_ADDRESS_POSTAL_FMT, since it is private API, and
> I cannot find a proper way to get that postal format string. Only the locale
> tool uses it, it seemed, and they reference the private API.
> 
> This broke FreeBSD. So what I was thinking was getting a list of locales that
> have the number before the street, or the other way around, whichever is the
> shorter list. And from that have logic to format the <house number> <street
> name> pair.

All I want, for now is the correct ordering of <house number> and <street name>. Number before street or number after street.

This became personal since I added my own address and want it formatted in a way that is familiar for me :)

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > So I am re-opening this. 
> > > 
> > > I would like this functionality, either if we can find a proper way of getting
> > > the postal_fmt field or if we can do something purely based on a list of
> > > LOCALE?
> > 
> > I don't understand the question here. _NL_ADDRESS_POSTAL_FMT should contain all
> > the information necessary to format the address, but the metadata given back by
> > the service might not exactly match the fields used by
> > _NL_ADDRESS_POSTAL_FMT...
> > 
> > What exactly do you expect to happen? Full support for the spec? It looks
> > doable although it probably needs to be cleaned up (look at the number of
> > address formats that don't include a country name for example).
> 
> See above, we can not use _NL_ADDRESS_POSTAL_FMT, since it is private API, and
> I cannot find a proper way to get that postal format string. Only the locale
> tool uses it, it seemed, and they reference the private API.

I don't think it's private API, it's a GNU only API most likely. Given that the information from the page you linked above is from glibc's locale data, I'm pretty sure using the data from the glibc locale information is fine.

> This broke FreeBSD.

That's because FreeBSD doesn't use the glibc.

#ifndef _NL_ADDRESS_POSTAL_FMT
  // Do default FreeBSD thing
#else
  // Get info
#endif

> So what I was thinking was getting a list of locales that
> have the number before the street, or the other way around, whichever is the
> shorter list. And from that have logic to format the <house number> <street
> name> pair.

Which is information that you can already have when using the glibc. If something exists on FreeBSD to do that, it can be implemented later, in the meanwhile, I'm not sure it's interesting to start copying data from glibc's locales just for FreeBSD's benefit.

(In reply to comment #34)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #29)
> > > > So I am re-opening this. 
> > > > 
> > > > I would like this functionality, either if we can find a proper way of getting
> > > > the postal_fmt field or if we can do something purely based on a list of
> > > > LOCALE?
> > > 
> > > I don't understand the question here. _NL_ADDRESS_POSTAL_FMT should contain all
> > > the information necessary to format the address, but the metadata given back by
> > > the service might not exactly match the fields used by
> > > _NL_ADDRESS_POSTAL_FMT...
> > > 
> > > What exactly do you expect to happen? Full support for the spec? It looks
> > > doable although it probably needs to be cleaned up (look at the number of
> > > address formats that don't include a country name for example).
> > 
> > See above, we can not use _NL_ADDRESS_POSTAL_FMT, since it is private API, and
> > I cannot find a proper way to get that postal format string. Only the locale
> > tool uses it, it seemed, and they reference the private API.
> 
> I don't think it's private API, it's a GNU only API most likely. Given that the
> information from the page you linked above is from glibc's locale data, I'm
> pretty sure using the data from the glibc locale information is fine.
> 
> > This broke FreeBSD.
> 
> That's because FreeBSD doesn't use the glibc.
> 
> #ifndef _NL_ADDRESS_POSTAL_FMT
>   // Do default FreeBSD thing
> #else
>   // Get info
> #endif
> 
> > So what I was thinking was getting a list of locales that
> > have the number before the street, or the other way around, whichever is the
> > shorter list. And from that have logic to format the <house number> <street
> > name> pair.
> 
> Which is information that you can already have when using the glibc. If
> something exists on FreeBSD to do that, it can be implemented later, in the
> meanwhile, I'm not sure it's interesting to start copying data from glibc's
> locales just for FreeBSD's benefit.

Sure that works for me. Thanks!

Created attachment 293920 [details] [review]
reverse: Derive street address format from locale

Determine street address format using nl_langinfo.

The patch is Marcus' slightly refactored. And also uses the function for set_street_address.

Created attachment 293922 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Created attachment 293924 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Review of attachment 293924 [details] [review]:

::: geocode-glib/geocode-glib.c
@@ +192,3 @@
+
+gboolean
+_geocode_object_is_number_after_street (void)

This whole function could be cached, as the locale isn't likely to change during runtime (actually, it can't change).

You can use g_once() for that.

@@ +201,3 @@
+  gchar *h;
+
+  addr_format = nl_langinfo (_NL_ADDRESS_POSTAL_FMT);

Check if addr_format is NULL, just in case (C locale?)

@@ +202,3 @@
+
+  addr_format = nl_langinfo (_NL_ADDRESS_POSTAL_FMT);
+  s = g_strstr_len (addr_format, -1, "%s");

Can you add a link to the definition of those values (%s and %h)?

Created attachment 293978 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Created attachment 293979 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Review of attachment 293979 [details] [review]:

Looks good otherwise.

::: geocode-glib/geocode-glib.c
@@ +194,3 @@
+is_number_after_street (gpointer data)
+{
+	static gboolean retval;

No need to make it static, is_number_after_street() should only get called once.

::: geocode-glib/geocode-reverse.c
@@ +155,3 @@
                         } else if (house_number != NULL && g_strcmp0 (members[i], "road") == 0) {
+                                gboolean number_after;
+                                char * name;

char *name, not char * name

Review of attachment 293979 [details] [review]:

::: geocode-glib/geocode-glib.c
@@ +194,3 @@
+is_number_after_street (gpointer data)
+{
+	static gboolean retval;

Well, yeah.

The reason I did this is because g_once wants to return gpointer and I want to return boolean. So by returning the address of the gboolean I get a pointer. But I do not want to return the address of a local variable, and I didn't want to allocate a boolean. So I thought that if I use a static local variable then the scoping would allow me to return the address.

Do you have a nicer way of me of doing this? Should I use GINT_TO_GPOINTER in some way?

The g_once test in glib/tests/once.c does sort of a mix :)

static gpointer
do_once (gpointer data)
{
    static gint i = 0;

    i++;

    return GINT_TO_POINTER (i);
}

But that might be static just to verify it gets called only once.

I see that gedit has a GBOOLEAN_TO_GPOINTER macro in gedit-utils.h:

#define GBOOLEAN_TO_POINTER(i) (GINT_TO_POINTER ((i) ? 2 : 1))

Should I use something like that?

Created attachment 294005 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Created attachment 294006 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Review of attachment 294006 [details] [review]:

::: geocode-glib/geocode-glib.c
@@ +216,3 @@
+
+ out:
+	return GINT_TO_POINTER (retval ? 2 : 1);

GINT_TO_POINTER(retval) will be enough.

@@ +228,3 @@
+
+	g_once (&once, is_number_after_street, NULL);
+	return (GPOINTER_TO_INT (once.retval) == 2);

And the "== 2" won't be necessary.

(In reply to comment #44)
> Review of attachment 293979 [details] [review]:
> 
> ::: geocode-glib/geocode-glib.c
> @@ +194,3 @@
> +is_number_after_street (gpointer data)
> +{
> +    static gboolean retval;
> 
> Well, yeah.
> 
> The reason I did this is because g_once wants to return gpointer and I want to
> return boolean. So by returning the address of the gboolean I get a pointer.
> But I do not want to return the address of a local variable, and I didn't want
> to allocate a boolean. So I thought that if I use a static local variable then
> the scoping would allow me to return the address.
> 
> Do you have a nicer way of me of doing this? Should I use GINT_TO_GPOINTER in
> some way?

GINT_TO_POINTER() is the right way to do things, yes.

> The g_once test in glib/tests/once.c does sort of a mix :)

It does that to ensure that do_once is only called once :)
<snip>
> But that might be static just to verify it gets called only once.

Yes :)

> I see that gedit has a GBOOLEAN_TO_GPOINTER macro in gedit-utils.h:
> 
> #define GBOOLEAN_TO_POINTER(i) (GINT_TO_POINTER ((i) ? 2 : 1))
> 
> Should I use something like that?

That's not necessary. In gedit's case, it's used to avoid possible problems with the inability to differentiate between FALSE (0 or NULL in pointer form) with a missing item in a hashtable:
ret = GPOINTER_TO_INT(g_hash_table_lookup (ht, key));
cannot differentiate between "FALSE" being in the hashtable and the key not being in the hashtable.

Created attachment 294053 [details] [review]
Derive street address format from locale

Determine street address format using nl_langinfo.
This is only possible on the GNU C library atm.

Review of attachment 294053 [details] [review]:

Looks good, you just need a test case for test-gcglib now!

Created attachment 294074 [details] [review]
test-gcglib: Add test of address format

Test the ordering of house number and street name.

Review of attachment 294074 [details] [review]:

Looks good!

Attachment 294074 [details] breaks on FreeBSD because of LC_ADDRESS.

Created attachment 294594 [details] [review]
test: Fix compilation on non-glibc platforms

Comment on attachment 294594 [details] [review]
test: Fix compilation on non-glibc platforms

See bug 742957

Does not the patch in bug 742957 cover this?

Yes, it is fixed now.