After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 651672 - Individual should have a display-name property
Individual should have a display-name property
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other Linux
: Normal normal
: gnome-3.6
Assigned To: folks-maint
folks-maint
Depends on: 685039
Blocks: 681391
 
 
Reported: 2011-06-02 00:09 UTC by Travis Reitter
Modified: 2013-11-08 00:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
New display_name property (2.52 KB, patch)
2012-07-03 13:15 UTC, Laurent Contzen
needs-work Details | Review
Updated the previous patch from Laurent (3.12 KB, patch)
2012-07-25 19:15 UTC, Jeremy Whiting
none Details | Review
Updated to build (3.12 KB, patch)
2012-07-25 19:37 UTC, Jeremy Whiting
needs-work Details | Review
Add display_name property to Individual. (4.05 KB, patch)
2012-08-01 14:13 UTC, Laurent Contzen
needs-work Details | Review
Add display_name property to Individual. (4.66 KB, patch)
2012-08-04 09:35 UTC, Laurent Contzen
needs-work Details | Review
Add display-name property to Individual (10.95 KB, patch)
2012-09-28 13:27 UTC, Philip Withnall
needs-work Details | Review
Bug 651672 — Individual should have a display-name property (17.51 KB, patch)
2013-09-29 16:08 UTC, Philip Withnall
reviewed Details | Review
Bug 651672 — Individual should have a display-name property (21.43 KB, patch)
2013-11-07 17:21 UTC, Philip Withnall
committed Details | Review

Description Travis Reitter 2011-06-02 00:09:30 UTC
This property is what most applications should display as a simple display name for any given NameDetails-implementing object.

It will iterate through a priority list of field types and be set to the first non-empty match.

I think the priority list should look something like:

* Alias (most-preferred since it's user-settable)
* Full Name
* Nickname (defined as ~contact-set, with the possible exception of user-set address book Nickname values)
* ...
* email address
* IM address
* ...
* _("Unnamed person")

I guess it's up to debate whether Alias or Full Name would tend to be more useful as a display name, but Alias is slightly more-easily set by the user (since it can be set within Empathy, etc.). Full Name (and any of the other fields) are settable in EDS, which somewhat breaks our "alias is user-set, full name and nickname are contact-set" assumption, but I think alias still tends to be slightly preferred as the first option.

Email addresses tend to be more human-readable than IM addresses, hence the ordering. In part, people tend to have at least one professional-sounding email address in case they need to hand it out and not look silly. It's slightly less-so for IM addresses.
Comment 1 Philip Withnall 2011-06-14 16:32:48 UTC
Will one static priority list of proprerties to try suffice for all applications? I wonder if different client applications will want different priority lists, given the applications' different use cases, making this property redundant.
Comment 2 Travis Reitter 2011-06-14 17:48:42 UTC
(In reply to comment #1)
> Will one static priority list of proprerties to try suffice for all
> applications? I wonder if different client applications will want different
> priority lists, given the applications' different use cases, making this
> property redundant.

I'm leaning toward "we're all better off with all clients showing consistent names".

A phone-calling client might want to prefer showing a phone number instead of a Jabber address, but they should probably show both the display name and the set of phone numbers that you might start a call with.

An argument for limiting the fall-back list to only names and not addresses (structured name components, full name, alias) is that you wouldn't want to show the display name "+156788567" in the phone app for a contact with phone numbers "+156788567" and "+113298798". But now that I've written that out, it doesn't sounds as bad as I thought. And in most cases, we're probably better off showing a phone number or jabber ID than a UID or "<unnamed>".
Comment 3 Philip Withnall 2011-06-17 08:37:18 UTC
Sounds reasonable to me.
Comment 4 Travis Reitter 2011-08-01 18:39:26 UTC
Punting bugs that won't be fixed by Folks 0.6.0.
Comment 5 Travis Reitter 2011-08-23 16:21:34 UTC
(Setting target bugs for Folks 0.6.1. Search for this phrase to deal with bug mail spam.)
Comment 6 Raul Gutierrez Segales 2011-08-29 12:46:50 UTC
Didn't make this release; punting to 0.6.2.
Comment 7 Philip Withnall 2011-08-30 20:48:39 UTC
I wonder if it might make more sense to just add a display-name property to Individual and Persona separately from the NameDetails interface. Tpf.Persona doesn't implement NameDetails, and Edsf.Persona doesn't implement AliasDetails; it would be a bit awkward to implement a display-name property on NameDetails which assumed the implementing class also implemented AliasDetails.
Put another way, taken to extremes a Persona could implement zero interfaces. Even in this case, we'd probably still want a display-name property on it.
Comment 8 Alexander Larsson 2011-08-31 06:41:07 UTC
Yeah, I don't see why the display name has to be on NameDetails. Its just an Individual thing. Also, i think full name should have priority over alias. At least its what you assume when displaying something in e.g. gnome-contacts (and thus also in the shell search to match).
Comment 9 Travis Reitter 2011-09-05 16:49:33 UTC
(Punting bugs to 0.6.4)
Comment 10 Travis Reitter 2011-10-11 20:36:57 UTC
(mass bump to gnome-3.4 milestone in our new milestone scheme; search this string mass move bug mail)
Comment 11 Travis Reitter 2012-06-13 17:00:09 UTC
This should be fairly easy to fix and useful. I just tested out:

* add a new contact in gnome-contacts named "Foo"
* link it to 2 un-aliased Telepathy contacts

The effect in Empathy is that the first Telepathy contact linked to Foo above disappears. In reality, there's a new Individual with both personas linked in with the second Telepathy contact's ID as its display string (not "Foo", as you might expect).

This is pretty confusing from a user standpoint - it even took me a moment to figure out what happened.

However, if Empathy (and all other Folks clients) could just show a display-name property, it'd help keep things consistent.
Comment 12 Travis Reitter 2012-06-13 17:13:27 UTC
I'm not sure this should just be added to NameDetails, though. I think the fallback list should look something like:

* primary persona Alias
* primary persona Full Name
* _("%s %s" % (primary persona name.given_name, primary persona name.family_name))
** (ie, adjusted for locale)
* service id (eg, foo@example.org)
* postal address
* _("Unnamed Person")
* (not the local id, which isn't much better than an empty string) 
* same sequence above, but for non-primary personas

This is mainly to stick with the principal "user-set values win, service IDs only as a last-ditch effort"

We should probably just implement this in Individual, not in NameDetails (since it relies upon the AliasDetails and other interfaces, and we don't want to pull those in as dependencies). It means we won't have a display-name for Personas, but applications should only care about the Individual value anyhow.
Comment 13 Guillaume Desmottes 2012-06-14 06:47:08 UTC
(In reply to comment #12)
> I'm not sure this should just be added to NameDetails, though. I think the
> fallback list should look something like:
> 
> * primary persona Alias
> * primary persona Full Name
> * _("%s %s" % (primary persona name.given_name, primary persona
> name.family_name))
> ** (ie, adjusted for locale)
> * service id (eg, foo@example.org)
> * postal address
> * _("Unnamed Person")
> * (not the local id, which isn't much better than an empty string) 
> * same sequence above, but for non-primary personas

Agreed.

> This is mainly to stick with the principal "user-set values win, service IDs
> only as a last-ditch effort"
> 
> We should probably just implement this in Individual, not in NameDetails (since
> it relies upon the AliasDetails and other interfaces, and we don't want to pull
> those in as dependencies). It means we won't have a display-name for Personas,
> but applications should only care about the Individual value anyhow.

Agreed; having on Individual is best.

My new roster view just use the alias atm, I'm not going to change that now,  I prefer to wait for this new API.
Comment 14 Laurent Contzen 2012-07-03 13:15:56 UTC
Created attachment 217930 [details] [review]
New display_name property
Comment 15 Philip Withnall 2012-07-03 20:38:37 UTC
Review of attachment 217930 [details] [review]:

::: folks/individual.vala
@@ +1457,3 @@
+
+      if (this.alias != null)
+        new_display_name = this.alias;

Please put braces around the ‘if’ blocks (even though they’re only one line long).

@@ +1460,3 @@
+      else if (this.full_name != null)
+        new_display_name = this.full_name;
+      else if (this.structured_name.to_string() != null)

You should check if (this.structured_name != null) first. Then instead of checking whether (.to_string () != null), check whether (.is_empty() == false).

@@ +1461,3 @@
+        new_display_name = this.full_name;
+      else if (this.structured_name.to_string() != null)
+        new_display_name = this.structured_name.to_string();

Missing a space before the ‘()’ (and in the line above).

Note that StructuredName.to_string() outputs names in quite a different format from what Travis suggested in comment #12. It might not be what we want.

Also, what about a fallback to the primary persona’s Persona.display_id before falling back to “Unnamed Person”?

Additionally, this patch doesn’t implement the fallbacks in the order Travis proposed (i.e. checking all fallbacks against the primary persona first, then falling back to non-primary personas). What’s the reason for this? (Arguments as to why this is better than what Travis proposed are welcome!) :-)

@@ +1463,3 @@
+        new_display_name = this.structured_name.to_string();
+      else
+        new_display_name = _("Unnamed Person");

This should probably have a translator comment explaining what the purpose of the string is.

@@ +1465,3 @@
+        new_display_name = _("Unnamed Person");
+
+      if (new_display_name != this.display_name)

It’ll be slightly faster to check against this._display_name, rather than this.display_name.

@@ +1468,3 @@
+        {
+          this._display_name = new_display_name;
+          this.notify_property ("display_name");

For consistency with the rest of the file, please use “display-name” (with a hyphen rather than an underscore) in the notify_property() call. :-)
Comment 16 Jeremy Whiting 2012-07-18 18:58:15 UTC
Didn't make gnome-3.4 release, punting to gnome-3.6 milestone
Comment 17 Philip Withnall 2012-07-22 07:45:45 UTC
(In reply to comment #14)
> Created an attachment (id=217930) [details] [review]
> New display_name property

Laurent, are you going to come up with a new version of the patch, or should someone else take this on?
Comment 18 Jeremy Whiting 2012-07-25 19:15:14 UTC
Created attachment 219651 [details] [review]
Updated the previous patch from Laurent

Philip,

I updated based on your comments, but am not sure if there's an easy way to get the primary persona from an individual.  Do you mean I should use primary_compare_func from _update_single_valued_property (or use that method itself maybe)?  If not what's the best way to get the primary persona?
Comment 19 Jeremy Whiting 2012-07-25 19:37:11 UTC
Created attachment 219654 [details] [review]
Updated to build

Oops, apparently string doesn't have an is_empty function.  Updated it to use string == "" instead.
Comment 20 Philip Withnall 2012-07-26 15:44:04 UTC
Review of attachment 219654 [details] [review]:

> I updated based on your comments, but am not sure if there's an easy way to get
> the primary persona from an individual.  Do you mean I should use
> primary_compare_func from _update_single_valued_property (or use that method
> itself maybe)?  If not what's the best way to get the primary persona?

A persona, A, is a ‘primary persona’ if A.store.is_primary_store is true.

::: folks/individual.vala
@@ +1465,3 @@
+        }
+      else if (this.structured_name != null &&
+               this.structured_name.to_string () == "")

Isn’t the second part of this condition the wrong way round? As it is, the code will always set new_display_name to "" if this branch is taken.

@@ +1471,3 @@
+      else
+        {
+          /* Iterate over all personas and sum up their IM interaction counts*/

This comment is wrong.

@@ +1482,3 @@
+          }
+          
+          if (new_display_name == "")

I think this branch should be moved outside of the ‘else’ block so that it clearly acts as a catch-all if everything else fails.
Comment 21 Laurent Contzen 2012-08-01 14:13:02 UTC
Created attachment 220060 [details] [review]
Add display_name property to Individual.

Fixes bug 651672

Based on Jeremy Whiting's update of my previous patch
Comment 22 Philip Withnall 2012-08-03 20:31:22 UTC
Review of attachment 220060 [details] [review]:

Getting there. :-)

::: folks/individual.vala
@@ +288,3 @@
+
+  /**
+   * {@inheritDoc}

You can’t use the inheritDoc taglet here because display_name isn’t inherited from any interface or superclass. A full documentation comment needs to be written for it, including a “@since” line.

@@ +1454,3 @@
+  private string _lookup_for_display_name (Persona p)
+    {
+      var alias = ((AliasDetails) p).alias;

This assumes the Persona implements AliasDetails, which it might not. In that case, “(AliasDetails) p)” will evaluate to null and folks will crash. Try:

var a = p as AliasDetails;
if (a != null && a.alias != null)
  {
    return a.alias;
  }

@@ +1460,3 @@
+        }
+
+      var full_name = ((NameDetails) p).full_name;

Same for the NameDetails interface here and below.

@@ +1470,3 @@
+        {
+          var name = structured_name.given_name + " " +
+		    structured_name.family_name;

The second line should only be indented 4 spaces past the first one here.

Also, it would be good to get input from the i18n team about whether this is the best way of doing things or whether, for example, the string format should be translatable. Would you mind sending an e-mail to the gnome-i18n@gnome.org mailing list about it?

@@ +1485,3 @@
+          foreach (var pa_fd in ((!) address_details).postal_addresses)
+            {
+              var pa = ((PostalAddress) pa_fd);

This cast to PostalAddress is wrong. ‘pa_fd’ is of type PostalAddressFieldDetails. You should use pa_fd.value, and a cast shouldn’t be necessary.

@@ +1498,3 @@
+  private void _update_display_name ()
+    {
+      Persona primary_personna = null;

This should have type ‘Persona?’ (i.e. a _nullable_ Persona) rather than just ‘Persona’. Also, s/personna/persona/.

@@ +1522,3 @@
+                {
+                  new_display_name = _lookup_for_display_name (p);
+                }

Instead of checking if “new_display_name == ""” each time, why not check after calling _lookup_for_display_name() and break out of the loop if “new_display_name != ""”?

@@ +1528,3 @@
+      if (new_display_name == "")
+        {
+          new_display_name = _("Unnamed Person");

This should have a comment to translators explaining the string. e.g.:

/* Translators: This is the default name for an Individual when displayed in the UI if no personal details are available for them. */
Comment 23 Laurent Contzen 2012-08-04 08:21:00 UTC
(In reply to comment #22)
> Review of attachment 220060 [details] [review]:
> 
> Getting there. :-)
Thanks! I'm going to fix my patch according to your comments.

> Also, it would be good to get input from the i18n team about whether this is
> the best way of doing things or whether, for example, the string format should
> be translatable. Would you mind sending an e-mail to the gnome-i18n@gnome.org
> mailing list about it?

Done!
Comment 24 Laurent Contzen 2012-08-04 09:35:48 UTC
Created attachment 220304 [details] [review]
Add display_name property to Individual.

Fixes bug 651672

Based on Jeremy Whiting's update of my previous patch
Comment 25 Laurent Contzen 2012-08-04 09:40:24 UTC
(In reply to comment #24)
> Created an attachment (id=220304) [details] [review]
> Add display_name property to Individual.
> 
> Fixes bug 651672
> 
> Based on Jeremy Whiting's update of my previous patch

Here's the corrected version. Two little notes :

- I set the @since version to 0.7.4 because the last tag I saw in the git branches was 0.7.3
- I left the structured name formatting as it was in the last patch, we'll see what the i18n team says about it.
Comment 26 Philip Withnall 2012-08-08 19:38:34 UTC
Review of attachment 220304 [details] [review]:

Looking much better! Once a conclusion is reached on gnome-i18n about the best way to format the names, I guess you can submit one final version of the patch for review with the necessary changes, then it can be committed.

::: folks/individual.vala
@@ +290,3 @@
+   * The name of this Individual to display in the UI.
+   *
+   * This value is set according to the following list of possibilities :

s/ :/:/

@@ +299,3 @@
+   * - _("Unnamed Person")
+   *
+   * @since 0.7.4

We have a convention in libfolks where we use “@since UNRELEASED” for new symbols. The “UNRELEASED” is then replaced by the version number during the build process.

@@ +1481,3 @@
+            {
+              var name = n.structured_name.given_name + " " +
+                  n.structured_name.family_name;

From the replies on gnome-i18n, this should be translatable. Perhaps a new method in StructuredName which uses the glibc formatting functions for names.
Comment 27 Travis Reitter 2012-08-16 23:25:08 UTC
(In reply to comment #26)
> Review of attachment 220304 [details] [review]:
> 
> Looking much better! Once a conclusion is reached on gnome-i18n about the best
> way to format the names, I guess you can submit one final version of the patch
> for review with the necessary changes, then it can be committed.

Was there ever a conclusion? Can this be finalized and committed?
Comment 28 Laurent Contzen 2012-08-17 07:47:30 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > Review of attachment 220304 [details] [review] [details]:
> > 
> > Looking much better! Once a conclusion is reached on gnome-i18n about the best
> > way to format the names, I guess you can submit one final version of the patch
> > for review with the necessary changes, then it can be committed.
> 
> Was there ever a conclusion? Can this be finalized and committed?

Hello,

As the discussion on the i18n ML stated (https://mail.gnome.org/archives/gnome-i18n/2012-August/msg00037.html) we could use the glibc's name formatting section. I'll continue looking how to do that in Vala as soon as possible, but I've got quite some work to do now.
Comment 29 Philip Withnall 2012-08-25 19:44:59 UTC
Also, we should probably add NameDetails.nickname into the fallback list somewhere (probably just before the display ID).

Prompted by looking at get_name() in https://github.com/openshine/gnome-sms/blob/master/src/helper.vala, which is an actual program which actually uses folks, like, for real.
Comment 30 Philip Withnall 2012-09-27 15:14:29 UTC
(In reply to comment #28)
> As the discussion on the i18n ML stated
> (https://mail.gnome.org/archives/gnome-i18n/2012-August/msg00037.html) we could
> use the glibc's name formatting section. I'll continue looking how to do that
> in Vala as soon as possible, but I've got quite some work to do now.

This would be the best way to do it. I’ve looked into how to use name_fmt, and I’ve come up blank. Ideally we need to find a glibc-provided helper function like strftime(), but I can’t find one for names. The best I can come up with is retrieving the localised name_fmt using:
    nl_langinfo (_NL_NAME_NAME_FMT)
and then implementing the substitution of field descriptors ourselves, but that looks painful.
See: http://www.gnu.org/savannah-checkouts/gnu/libc/manual/html_node/The-Elegant-and-Fast-Way.html#The-Elegant-and-Fast-Way

I’ve asked on the mailing list about how to do this. Until I get an answer there (or here!), this patch isn’t going anywhere. :-(
Comment 31 Philip Withnall 2012-09-28 13:27:06 UTC
Created attachment 225334 [details] [review]
Add display-name property to Individual

https://www.gitorious.org/folks/folks/commits/651672-display-name

Here’s an updated patch which implements parsing of glibc’s name_fmt in folks itself, since no function exists in glibc to do that[1].

Unfortunately, this can’t be committed yet because:
 • it doesn’t compile because I can’t get Vala to include the right header for nl_langinfo(); and because of this
 • it hasn’t been tested yet. I need to write some unit tests for StructuredName.to_string().

So this patch can sit here for a while.

[1]: I’ve filed http://sourceware.org/bugzilla/show_bug.cgi?id=14641 about that.
Comment 32 Alexander Larsson 2012-10-02 13:42:08 UTC
Is using the name format for the current locale really right? I mean, any particular contact in your address book might not have a name that is in the format of the local you're using. For instance if you live in different countries.

I'm not sure having a locale-dependent name parser makes much sense in general.
Comment 33 Philip Withnall 2012-10-03 12:31:15 UTC
(In reply to comment #32)
> Is using the name format for the current locale really right? I mean, any
> particular contact in your address book might not have a name that is in the
> format of the local you're using. For instance if you live in different
> countries.
> 
> I'm not sure having a locale-dependent name parser makes much sense in general.

Hum. We prefer NameDetails.full_name over this parsed-and-reassembled name, since that’s a simple string which can be formatted however the contact prefers.

What are the alternatives?
 • Not using NameDetails.structured_name at all in generating a display name. It seems silly to waste that information if it’s available — even a name which is formatted in the wrong locale is better than a nickname, service ID or “Unnamed Person”.
 • Using NameDetails.structured_name and formatting it in the contact’s locale rather than the user’s locale. This requires almost all of the same code, but also requires that we know the locale of each contact. I’m not sure any of our backends can give us that.

What do you suggest?
Comment 34 Alexander Larsson 2012-10-04 13:35:48 UTC
I guess you got to display *something*, even if you only have the structured name. And since whatever you do might be wrong, it might as well be the users locale, as that has the highest chance of being right.
Comment 35 Travis Reitter 2013-09-24 15:46:47 UTC
Review of attachment 225334 [details] [review]:

Could we clean this up and merge it? The core display-name functionality is something we should really have in place. And we can improve heuristics later if we need to (though they seem fine now).

The patch mostly looks good to me except for the initialization and name_fmt portions because I think we should err on the side of "if we can't support it well, don't claim to support it". That way, clients can do their own implementation if they're satisfied with the quality they can achieve. And they can do it internally without having to worry about API/ABI breaks.

We could think of the name portions -> given format function as "safe" since we can just provide a string based upon whatever decomposed elements we've been given (and have confident are accurate). But the main use case for this type of function is to display and sort a list based on, eg, last name. If only 5% of your contact list can be listed accurately like this, it's not very useful (it'd probably have to be 95% filled in to be terribly useful).

And I think we can all agree that parsing arbitrary names is just as problematic as parsing mailing addresses. In the vast majority of cases, they should just be left as-is for the user to make sense of.

As a side note, https://bugzilla.gnome.org/show_bug.cgi?id=685039 has been fixed.
Comment 36 Philip Withnall 2013-09-29 16:08:05 UTC
Created attachment 256028 [details] [review]
Bug 651672 — Individual should have a display-name property

Based on work by Jeremy Whiting and Philip Withnall.

New API:
 • Individual.display_name

Closes:
Comment 37 Philip Withnall 2013-09-29 16:15:21 UTC
This new version fixes nl_langinfo(), but there’s a tiny fix needed in bug #685039 (and then a Vala release and dependency bump) before it’ll compile properly.

With that fix applied, I’ve compiled and tested it, and it works OK using folks-inspect. The new patch also includes a small test case for the nl_langinfo() stuff.
Comment 38 Simon McVittie 2013-11-06 12:26:54 UTC
Review of attachment 256028 [details] [review]:

I think the commit message deserves either a trailing [modified to use nl_langinfo() -pwithnall] or something, or re-attributing to yourself with Laurent in the "based on...", depending how much you rewrote. I tend to apply the general principle that anything not written by the attributed author should be noted, to avoid blaming them for any errors in the rewrite.

::: docs/Makefile.am
@@ +74,3 @@
 	folks-generics \
 	build-conf \
+	posix \

Does Folks aim to be portable? If it does, this would need to be conditional on "we're on Unix".

::: folks/individual.vala
@@ +317,3 @@
+   *  # Primary persona postal address
+   *  # Same sequence, but for non-primary personas
+   *  # _("Unnamed Person")

This comment is no longer true. The actual implementation appears to go like:

# Primary persona alias
# Non-primary persona alias
# Primary persona full name
# Primary persona structured name
# Primary persona nickname
# Non-primary persona full name
# Non-primary persona structured name
# Non-primary persona nickname
# Primary persona display_id (is this the service ID,
  e.g. foo@example.com?)
# Non-primary persona display_id
# Primary persona postal address
# Non-primary persona postal address
# _("Unnamed Person")

which also seems reasonable, tbh, but isn't the same.

There was some mention of preferring email addresses (even from non-primary personas?) over IM addresses - did you decide against that?

@@ +1688,3 @@
+  private string _look_up_name_details_for_display_name (Persona? p)
+    {
+      var n = p as NameDetails;

A gap in my Vala knowledge: does "null as Foo" gracefully return ((Foo?) null)?

::: folks/name-details.vala
@@ +203,3 @@
+      while (names.get_next_char (ref index, out c) == true)
+        {
+          if (c.isspace () || c == '-')

Is the intention here that if we're turning "Mary-Jane Watson" into "%G %f" form, we return "MJ Watson"?

@@ +225,3 @@
     {
+      /* FIXME: Extracted from langinfo.h and locale.h because it's partially
+       * private there, and definitely not bound in Vala. */

How portable is this? Should this whole block be #ifdef __GLIBC__ or something, with (e.g.) the en_US version used as a fallback for other libcs?
Comment 39 Simon McVittie 2013-11-06 12:41:59 UTC
(In reply to comment #38)
> How portable is this? Should this whole block be #ifdef __GLIBC__ or something,
> with (e.g.) the en_US version used as a fallback for other libcs?

Actually, if it was me, I'd do something like this pseudocode:

#ifdef __GLIBC__
  ... hopelessly glibc-specific runes here ...
#else
  /* Translators: format string to construct names, similar to glibc's
   * name_fmt. Only the following format specifiers are supported: ... */
  name_fmt = _("%d%t%g%t%m%t%f");
#endif

... or even ditch the nl_langinfo() path altogether, and just reuse the syntax (despite what my locale says, I'd expect to see "Fred Smith" in my address book, not "Mr Fred James Smith").
Comment 40 Philip Withnall 2013-11-07 17:21:32 UTC
Created attachment 259204 [details] [review]
Bug 651672 — Individual should have a display-name property

Based on work by Jeremy Whiting and Philip Withnall.

New API:
 • Individual.display_name
 • StructuredName.to_string_with_format()
Comment 41 Philip Withnall 2013-11-07 17:23:56 UTC
Thanks for the review.

(In reply to comment #38)
> Review of attachment 256028 [details] [review]:
> 
> I think the commit message deserves either a trailing [modified to use
> nl_langinfo() -pwithnall] or something, or re-attributing to yourself with
> Laurent in the "based on...", depending how much you rewrote. I tend to apply
> the general principle that anything not written by the attributed author should
> be noted, to avoid blaming them for any errors in the rewrite.

Whoops, I’ll fix that before pushing.

> ::: docs/Makefile.am
> @@ +74,3 @@
>      folks-generics \
>      build-conf \
> +    posix \
> 
> Does Folks aim to be portable? If it does, this would need to be conditional on
> "we're on Unix".

All removed now. I gave up on glibc.

> ::: folks/individual.vala
> @@ +317,3 @@
> +   *  # Primary persona postal address
> +   *  # Same sequence, but for non-primary personas
> +   *  # _("Unnamed Person")
> 
> This comment is no longer true. The actual implementation appears to go like:

Fixed.

> There was some mention of preferring email addresses (even from non-primary
> personas?) over IM addresses - did you decide against that?

Still need to look at this.

> @@ +1688,3 @@
> +  private string _look_up_name_details_for_display_name (Persona? p)
> +    {
> +      var n = p as NameDetails;
> 
> A gap in my Vala knowledge: does "null as Foo" gracefully return ((Foo?) null)?

Yup. Whereas casts (‘(Foo) p’) abort if they fail.

> ::: folks/name-details.vala
> @@ +203,3 @@
> +      while (names.get_next_char (ref index, out c) == true)
> +        {
> +          if (c.isspace () || c == '-')
> 
> Is the intention here that if we're turning "Mary-Jane Watson" into "%G %f"
> form, we return "MJ Watson"?

Yes.

> @@ +225,3 @@
>      {
> +      /* FIXME: Extracted from langinfo.h and locale.h because it's partially
> +       * private there, and definitely not bound in Vala. */
> 
> How portable is this? Should this whole block be #ifdef __GLIBC__ or something,
> with (e.g.) the en_US version used as a fallback for other libcs?

Die, glibc, die.
Comment 42 Simon McVittie 2013-11-07 18:02:16 UTC
+      /* Translators: This is a format string used to convert structured names
+       * to a single string. It should be translated to the predominant
+       * semi-formal name format for your locale, using the placeholders
+       * documented here: http://lh.2xlibre.net/values/name_fmt/. You may be
+       * able to re-use the existing glibc format string for your locale on that
+       * page if it’s suitable.

I think it'd be worth saying explicitly:

    The supported placeholders are %f, %F, %g, %G, %m, %M, %t.
    The romanisation modifier (e.g. %Rf) is recognized but ignored.
    %s, %S and %d are all replaced by the same thing (the "Honorific
    Prefixes" from vCard) so please avoid using more than one.

Other than that, it looks good apart from:

(In reply to comment #41)
> > [...] the commit message [...]
> Whoops, I’ll fix that before pushing.

and

> > There was some mention of preferring email addresses (even from non-primary
> > personas?) over IM addresses - did you decide against that?
> 
> Still need to look at this.
Comment 43 Philip Withnall 2013-11-08 00:42:11 UTC
Comment on attachment 259204 [details] [review]
Bug 651672 — Individual should have a display-name property

Committed with the suggested changes. Thanks again for your reviews!

commit fded410fd72c290596fb2b2fb2929fa92339b1c6
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Wed Jul 25 13:10:02 2012 -0600

    Bug 651672 — Individual should have a display-name property
    
    Based on work by Jeremy Whiting and Laurent Contzen.
    
    New API:
     • Individual.display_name
     • StructuredName.to_string_with_format()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=651672

 NEWS                          |   3 +
 folks/individual.vala         | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 folks/name-details.vala       | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 tests/folks/Makefile.am       |   5 ++
 tests/folks/name-details.vala | 137 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 559 insertions(+), 12 deletions(-)