Comments

hass’s picture

Status: Active » Needs review
hass’s picture

StatusFileSize
new1.66 KB

small mistake in the above patch. review this one, please.

hass’s picture

StatusFileSize
new4.88 KB

found another \r\n issue like in project module... rerolled patch

hass’s picture

StatusFileSize
new2.04 KB

This patch fixes some translation bugs in the "issue.inc" file.

dww’s picture

Status: Needs review » Needs work

Help text with complex HTML shouldn't be 1 giant t() like this. Instead, please fix it like my latest patches at http://drupal.org/node/57667 and http://drupal.org/node/159334.

Also, if you're *just* fixing translation problems, please include all the changes in a single patch. That makes it easier to review, instead of separate patches for each file in a given module's directory. Putting other changes unrelated to translation problems in a patch/issue about translation problems makes it harder to review. Does that make sense?

For example, this isn't a translation change, you're proposing a change to the default UI (e.g. on drupal.org):

-  $header[] = array('data' => t('Pri'), 'field' => 'p.priority');
+  $header[] = array('data' => t('Priority'), 'field' => 'p.priority');

That belongs in a separate issue and patch. You're not "fixing" the translatability of the module, you're proposing to change the UI for English sites. Yes, "Priority" is easier to translate than just "Pri", but it might also make the UI look worse and require some additional .css changes or other things to get the spacing right.

Also, please verify that the fields in the mailhandler input could be translated. Otherwise, those keywords inside the help text should be handled outside of t() and passed in via placeholders or something, since people shouldn't translate them. See what I mean? Actually, since I was curious, I just looked myself and they can be translated. Though that raises weird questions about how mailhandler works on a multilingual site -- users send in email with the keywords in their own language, but how does hook_mailhandler() know what language to use for looking up the keywords? It's running via cron with the site's default language, it seems. Evil -- however, this belongs in a separate issue.

hass’s picture

Status: Needs work » Needs review
StatusFileSize
new6.08 KB

Updated patch

hass’s picture

Aside, let me say, Gabor haven't said we must split *all* strings. He only said it might be better for translators, but he missed to say that some stings are context sensitive and it is MORE difficult to translate them if i cannot read the context of the string.

Therefor i thing what we have done here is no good idea in general. Partly it makes sense here, but partly not.

dww’s picture

Status: Needs review » Needs work

Great, almost there. Now, for readability and ease of maintaining this in the future, and since it's not inside a t() anymore where it was causing problems, let's keep some of the newlines in between the various parts. This way, if we ever need to change anything in the future, we only have to modify the appropriate line, not the entire message (from the CVS or patch/diff perspective -- the separate t() already defends translators from having to retranslate the whole thing if we change a few words). For example:

$help = '<p>'. t('Use this page to add new status options for project issues or to change or delete existing options.') .'</p>'.
  '<dl><dt>'. t('Adding') .'</dt>'.
    '<dd>'. t('To add a new status option, put its name in one of the blank places at the bottom of the form and assign it a weight.') .'</dd>'.
  '<dt>'. t('Updating') .'</dt>'.
    '<dd>'. t('When renaming existing issues, keep in mind that issues with the existing name will receive the new one.') .'</dd>'.
  '<dt>'. t('Deleting') .'</dt>'.
  ...

Note, also, your </p> is in the wrong place in the patch, should be as I list here. (Though, I see that's just a bug in the original code, not your fault, but you shouldn't propagate bugs like this).

hass’s picture

Status: Needs work » Needs review
StatusFileSize
new6.74 KB

hopefully this patch gets your love. Additional i found and added the untranslatable "priorities".

dww’s picture

Additional i found and added the untranslatable "priorities".

That's going to require much more careful review and testing, since it's entirely possible those strings are being stored in the db, where they need to be English to keep consistent behavior. That should go to a new issue, since it requires more thought and care. The other stuff is basically RTBC now.

dww’s picture

Status: Needs review » Fixed

Committed a modified version of the 2 changes not include the "priorities" thing to HEAD, and backported the help text change to DRUPAL-4-7--2 and DRUPAL-4-7.

Updated the translation template to find you had committed a new copy based on your hacked workspace, not the official code. Please, never do this, or I'll make sure you don't have permission to commit to the po directories in any of my modules. It's nice if you help keep the .pot files up to date, but you should *never* commit changes based on your local modifications.

Thankfully, soon this won't involve CVS at all, and the translation templates will always be extracted automatically.

-Derek

Anonymous’s picture

Status: Fixed » Closed (fixed)