Wrong wording in activity

[chouf1]

%s new need to be changed to %s OR should have a plural form for translation
This could actually be 1 new or x new in english, but needs a s if more as 1 in other languages

This number is shown on the profile subnav bar where other items shows ie, site(2) or Friends (56), so IMO because the word # new doesn't follow the style of these items, it can be removed, by default

<span>%s new</span>
bp-themes/bp-default/activity/index.php:77
bp-templates/bp-legacy/activity/index.php:57

[johnjamesjacoby]

This makes sense; we should probably use _n() here anyways. Moving to 1.8 since we're in string freeze for 1.7.

[boonebgorges]

Thanks for the patch, merty. I'm going to switch it to use _nx(), as it's pretty unclear what this string means out of context.

[boonebgorges]

In 7004:

Use _nx() to make 'x new' activity mentions gloss translatable

Languages other than English require distinguishing between the singular and
plural adjectival forms of "new".

Fixes #4916

Props johnjamesjacoby, merty

[nacin]

Excellent use of _nx() here.

One thought: Generally numbers should be %d. But %s is actually correct here, because it is possible (even probable, over time) for this number to be in the thousands, which means it'll need a thousands separator. At which point we need a number_format_i18n() in there. (Note only for the sprintf() argument, not for the _nx() argument.)

It would probably make sense for bp_get_total_mention_count_for_user() to only be called once. I also noticed that it defaulted to $user_id = 0, but 0 will end up just returning false down the stack in get_metadata() — so it should either not be a default value, or a default of 0 should mean the current user at some point before it reaches get_user_meta().

Final question — what is the purpose of bp_loggedin_user_id() and how is it different from get_current_user_id()? As someone who knows WordPress fairly well but BuddyPress not so well, it is not obvious why there are wrapper functions such as this, and it makes it pretty difficult to wrap your head around things. $bp->loggedin_user should probably just *be* the $current_user object, and a function like this should probably just be get_current_user_id(). I do see that there is a filter in there, but that seems suspect.

[boonebgorges]

Thanks very much for the comment, nacin.

At which point we need a number_format_i18n()

Good call. I've opened a new ticket for it, as it's an issue we should address wherever we display similar counts.

It would probably make sense for bp_get_total_mention_count_for_user() to only be called once

Yes. I was going to fix this, but I'd have to introduce a variable into the global scope in the template, which we normally do not do. Thus, I'd have to abstract the whole bit into a standalone function. But we have other similar content counts (groups, friends, etc) that would have to get similar treatments in order for the template to be consistent. As so often happens, fixing a small thing would require big changes, so I've left well enough alone for the moment :)

I also noticed that it defaulted to $user_id = 0, but 0 will end up just returning false down the stack in get_metadata() — so it should either not be a default value, or a default of 0 should mean the current user at some point before it reaches get_user_meta().

I agree that it'd make sense for this function, and others like it, to default to the displayed user when no value is passed. But it's possible that this'll break backward compatibility with plugin/themes in some edge cases, so it needs more consideration. #4998

what is the purpose of bp_loggedin_user_id() and how is it different from get_current_user_id()?

As far as I know, the main purpose is that "this is the way we have always done it". We can't just switch over to the $current_user object, because there are certain other pieces of data that BP adds to the loggedin_user global ("domain", which is the profile URL; whether he's a super admin; the "fullname", which is loaded in different ways depending on how BP is configured and which therefore may be different from $current_user->display_name). As for the filter, it may indeed be "suspect" to make such a value filterable, but it *is* filterable, and we can't just tear it out without breaking something. I totally agree with the general sentiment that it'd make BP more approachable to a WP dev if this object were the same as $current_user and if we used WP functions in place of the BP wrappers. But it'd require a large amount of work to make the necessary changes in a way that didn't break backward compatibility, for what is, relatively speaking, a marginal benefit. Which is to say that there are places in BP where we could make changes in a similar spirit - better use of WP's tools, better parity with WP syntax, and so on - which would have a much higher impact on newcomers to BP.

Thanks again for the thoughtful comments.