Translatable text cleanup in privatemsg module

I had the same problem and added the t() function everywhere I found it appropriate...

I created a patch against HEAD, this is my first patch for a drupal module, so review it careful :)

There are two places where i changed the "text-building-logic" a bit, everywhere else I have just added the t() function to the strings

It is working fine on my site but there are probably better ways to do something

@Berdir: you'll want to roll that against 5.x-3.0 or 5.x-3.x-dev; HEAD is for D6 development.

Yes, I know.

As I am on D6 and using the HEAD-Version, I created the patch for the version I'm using.

I will create a second patch against the D5-Version, but it will take some time...

This is sort of selfish, but I notice you are using the ! placeholder instead of @ - which I think it correct, but do you have any idea why?

I have a patch fro Drupal 6 which changes a uses of @ to ! (#276174: Do not check_plain() usernames more than once) as this would allow the correct use of apostrophes in usernames but I was asked for where the other check_plain occurs. I have no idea - all I know is that other places in core also do this.

Back onto this patch - you may also want to cover the column names as changed in privatemsg_list (lines 261-276 rename the columns, but do not make them translatable) - however this may cause other problems with the current sorting code.

EDIT - oh and welcome to the HEAD development steam train.

I used ! because the names-placeholder looks like this: <a href="/user/xy">XY</a> and with a @placeholder, the html would be displayed escaped. Actually, in my first version, I had a @placeholder and replaced it with ! after i saw the result ;)

About your patch/problem... imho escaping should always happen before displaying, not in database or in a controller/model. So, instead of using ! it would be better to ensure that the string is never escaped before. But this is not always possible and I have not enough knowledge of drupal core to say if that is possible or not.

As I am writing this, I can see that this is also true for my use of !. And according to http://api.drupal.org/api/function/t/6 links should be part of the translation string. I will see if I can find a better solution and also include the column names.

Thanks, I'm glad if I can help as I really enjoy using privatemsg :)

I should not write such comments without looking at the code before :)

The username links are generated by the theme_username function so there is no way around using !. And the column names are already translated, but with a dynamic call (t($col), this should be avoided so that potx can generate a .pot file (see #194962: How to write Drupal translatable interfaces)

I've updated my patch a bit, here are the changes:

- Added some more t() calls
- replaced some occurrences of @placeholder and !placeholder with %placeholder
- changed some t() calls to be static instead of using variables. There is still one there, but it is probably not even used at all (line 280, see comment)

Again, the patch seems to work fine on my website.

I've seen that #311541: Freeze privatemsg db schema + msg delete functionality + changes to query builder does change much of the code and obviously has some conflicts with my patch.

As translation stuff isn't that important in a dev release, I'll wait until #311541: Freeze privatemsg db schema + msg delete functionality + changes to query builder is resolved and the attached patch(es) are committed. I will create a new version of my patch after that against HEAD and re-submit it.

In the meantime, I'll work on the promised 5.x-3.0 version.

As #311541: Freeze privatemsg db schema + msg delete functionality + changes to query builder has been commited and #321090: Code review already fixed some translation issues, I've finally updated my patch.

I haven't tested the patch on my site as I am waiting with upgrading because I had no time to look at the schema changes and I'm not sure if the schema is already freezed or not. But my IDE doesn't find any syntax errors...

I've created a first version of a german translation as well.

Please provide some feedback if I forgot something or it doesn't work...

Berdir

PS: I'm not sure if I should change the version to HEAD because the original bug report is about 5.x-3.0. I had a short look at this version and it seems that this is the only translation problem in this version so creating a patch for it is not really useful... (and I have no drupal5 setup to test it...)

PS 2: the translation directory is now called translations and not po anymore... so add it at the right location :)

+        case 'author':
+      	  $col = t('Author');
+      	  break;
+        case 'recipient':
+          $col = t('Recipient');
+          break;
+        case 'subject':
+          $col = t('Subject');
+          break;

That is no longer needed.

-        $status = 'Unread';
+        $status = t('unread');
       }
       else {
-        $status = 'Read';
+        $status = t('read');

Lack capitalisations.

Apart from that, seems good.

> That is no longer needed.

If I read privatemsg_privatemsg_list_alter correctly, subject is still there, but the other two are gone. Imho it does make sense to keep the subject-case to have a static t()-call so that the translation string can easier be extracted.

> Lack capitalisations.

$status is only used as part of another translation, does it really needs to be capitalised then ?

      if ($new == 1) {
        $status = t('unread');
      }
      else {
        $status = t('read');
      }
      $total_marked = db_affected_rows();
      drupal_set_message(format_plural($total_marked, "1 message marked as %status", '@count messages marked as %status', array('%status' => $status)));
    

yes, you're right - oops. my quick review was from just reading the patch.

No problem, english is a foreign language for me so I wasn't sure either...

Attached is a new version with the two unecessary cases removed. I'll try to upgrade my installation as described in #321171: unknown column pmi.uid to be able to test it but I think it should be ok now...

Applies cleanly and it works, so setting to rtbc.

but... there are (existing) strings I am unhappy with:

1. I would prefer the send button having the text "Send message" instead of just "Send".
2. "Create private message" to either "create new message" or "write new message".
3. The Inbox is not really the inbox but "all messages".
4. The column titles of sent/received are too granular and do not make sense in the threaded conversation world. where most conversations will have messages that have been sent AND messages received. Maybe change it to "last message", "last update" or "last reply"?

None of those need to impact this patch though.

Scope of this patch seem to have outgrown it's original title. lets fix all text that we thing needs translation.

Also HEAD may need the same ?

oops - the current patch was already for HEAD.

@ #14, 15

Yes my patch is against HEAD... but the original issue still remains open and is about 5.x-3.0.

@ #13

I'm fine with updating my patch and adding your ideas...

> 1. I would prefer the send button having the text "Send message" instead of just "Send".
Changed. What about Preview message, to keep it similiar ?

> 2. "Create private message" to either "create new message" or "write new message".
Does make sense. I don't care what text is used but imho it should be a similiar phrase in all 3 occurences (block, user page and the messages page) => No mix of new/private, create/write and so on. I've changed the block and messages page to Write new message, not sure about the user page... perhabs "'Send this user a message'"? (Write sounds wrong there and new seems unecessary.. you can't send him a old message :) )

> 3. The Inbox is not really the inbox but "all messages".
Changed in the block and messages page

>4. The column titles of sent/received are too granular and do not make sense in the threaded conversation world. where most conversations
> will have messages that have been sent AND messages received. Maybe change it to "last message", "last update" or "last reply"?
What about "last updated" ?

As my site is now updated to latest HEAD, i noticed something "strange" about the "Between ... and ..." String. There is some code to ensure that the active user is always the last one but does this actually make sense? why not just output them in the order the messages were sent?

Example:
User A sends user B a message. When reading this message it states "Between B and A". But it would be more logical to me if it would state "Between A and B", because A startet the conversation.

I'm waiting for some feedback before uploading a new version of my patch..

Argh... didn't wanted to change Version, just startet to comment before it changed...

@ Birdir - sounds good.

1: yes, add it to preview too.

2: yup, but remember that they are all also private too, so no need to over emphasise that either. I would not use the word "private" ever (even the main menu title - but I guess I will be alone on this.) as messages can be assumed to be private.

4: yup, "last updated" also get rid of the query in the code as the same title should be used in both cases.

As for the "Between xxxx and xxxx" string, it is functional, but I do not like it. Not the name ordering as we will need to use some sort of order (or not) which may or may not fit all circumstances (and the current logic is from when the viewers name was not used, it instead said " and you" - I nixed that as I like to have the username of "You" and the string could cause confusion.), but the actual wording. Simplest solution is to replace the Between with Participants:

1: done

2: Leaving the private in the menu title for now. I think it does make sense there as it is a common tense and after all, the module is called privatemsg :)

4: Done

About the participants... I tried to simplify the function a bit but I'm still not really happy with it, it has to many special cases imho (I *think* it is technically impossible to have a message without participants and the only way to have only one is when I am sending myself a message => Should this actually be allowed?) Or am I wrong about this? I have replaced Between so it is now Participants: A, B and C

Anyway, attached is a new patch, feedback is welcome :)

I like it.

However, you got rid of "Sent Messages" from the block. I also agree with that decision, but I guess that decision is above my pay grade, so leaving as CNR for Litwol.

I removed it because of #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering. and I personally don't think that it is useful.

My Idea was to display a list of (newest/most active/whatever) tags instead. But the implementation of that obviously depends on #314327: Tagging architecture.. Then the "All" in "All messages" would actually make sense...

Updated the patch and I will be presumptuous enough to set it to rtbc :P

I added back the link to sent messages - while we both do not like it, the decision to remove it is not ours, and would complicate this patch further. I want this in ASAP.

As for the tagging stuff, I am just about to break that out into a submodule - no idea if there is a way to still get the most active tags into the block in a reasonable manner.

Good work - this patch vastly improves the feel of the module for me - makes it feel much more substantial, polished.

With some logic fix in privatemsg_preprocess_privatemsg_to() :http://drupal.org/cvs?commit=148145

Good job guys :). keep 'em comin'

As the patch is commited, I'm attaching a new version of de.po and general.pot extracted by potx. Both do currently only contain translations of privatemsg.module, privatemsg.install is not yet translated..