There are some context sensitive issues where translators have no idea what it should mean.

Note to committer: Please apply patch #51.

CommentFileSizeAuthor
#85 views-translatable_strings_not_self_explanatory_D7-1779658-85.patch9.75 KBAndrew Answer
#69 Views_Translatable+strings+not+self-explanatory-D7.patch10.61 KBhass
#56 drupal-translatable_strings_not_self_speaking-1779658-55.patch29.74 KBmitron
#51 drupal-translatable_strings_not_self_speaking-1779658-51.patch10.35 KBmitron
#51 interdiff-46-51.txt296 bytesmitron
#47 drupal-translatable_strings_not_self_speaking-1779658-47.patch10.32 KBmitron
#47 interdiff-41-46.txt1.16 KBmitron
#41 drupal-translatable_strings_not_self_speaking-1779658-41.patch9.79 KBYesCT
#41 interdiff-31-41.txt1.3 KBYesCT
#35 drupal-translatable_strings_not_self_speaking-1779658-35.txt1.58 KBesoteric1
#33 drupal-translatable_strings_not_self_speaking-1779658-33.patch1.58 KBesoteric1
#31 drupal-translatable_strings_not_self_speaking-1779658-31.patch9.62 KBesoteric1
#18 commentapprove_after.png10.6 KBirunflower
#18 commentapprove_before.png9.36 KBirunflower
#18 usercancel_after.png9.09 KBirunflower
#18 usercancel_before.png8.58 KBirunflower
#18 userlink_after.png7.83 KBirunflower
#18 userlink_before.png7.18 KBirunflower
#16 drupal-translatable_strings_not_self_speaking-1779658-16.patch9.73 KBdisasm
#11 1779658+Translatable+strings+not+self+speaking++context+independent5-D8.patch11.19 KBhass
#10 1779658+Translatable+strings+not+self+speaking++context+independent4-D8.patch10.97 KBhass
#9 1779658+Translatable+strings+not+self+speaking++context+independent4-D7.patch12.07 KBhass
#8 1779658+Translatable+strings+not+self+speaking++context+independent3.patch12.05 KBhass
#7 1779658+Translatable+strings+not+self+speaking++context+independent2.patch11.67 KBhass
#5 2012-09-23_143707.png26.37 KBhass
#1 1779658+Translatable+strings+not+self+speaking++context+independent.patch1014 byteshass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Status: Active » Needs review
FileSize
1014 bytes

Patch attached

dawehner’s picture

Status: Needs review » Postponed (maintainer needs more info)

I really don't get that: if they don't know how a certain link works they can search for the code and somehow are able to see what it does...
Remember: A module has string freezes since it's stable release as well, why should this be changed "just" to get better meta-translatability.

hass’s picture

Title: Translatable strings not self speaking / not context independent » Translatable strings not self speaking / context independent

Since we have localize.d.o and l10n_update we can change buggy strings and fix them very easy every day.

As you are also german... The original string is "Cancel link". What should this mean? A link that cancel something? Well, in this case it means the link to cancel the user account. It was translated to German as "Link zum Abbrechen"... Heck - what??? :-((( it need to mean "Link zum Löschen des Benutzeraccounts" or similar, but this is NOT what the English string says, too. Feel free to find any other better and self speaking wording. There are more of these type of strings in views we need to fix :-)

You can find this string in field selection of views if you add a new user field.

hass’s picture

Title: Translatable strings not self speaking / context independent » Translatable strings not self speaking / not context independent
Status: Postponed (maintainer needs more info) » Needs review
hass’s picture

Title: Translatable strings not self speaking / context independent » Translatable strings not self speaking / not context independent
FileSize
26.37 KB

Just for the visual review of the problem, here is a screenshot that shows the problematic strings. The string User: Link to contact page is a good string and is self speaking all others not.

hass’s picture

Status: Needs review » Needs work

Huh, there are more strings that need to be changed.

hass’s picture

Status: Needs work » Needs review
FileSize
11.67 KB

New patch attached.

hass’s picture

Made some small changes to the wording as it may cause misunderstandings.

hass’s picture

hass’s picture

Version: 7.x-3.x-dev » 8.x-3.x-dev
FileSize
10.97 KB

D8 version of this patch.

hass’s picture

One of the strings from D7 was missing in the D8 patch. Fixed version attached.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/modules/user.views.incundefined
@@ -330,7 +330,7 @@
+      'title' => t('Link to cancel user'),

"Link to cancel user" isn't proper english. Add a/an/the or whatever to the sentences. Happens in multiple places.

In general I agree that some strings can be improved. I'm not sure about all the "status" changes.

xjm’s picture

Title: Translatable strings not self speaking / not context independent » Translatable strings not self-explanatory / not context-independent
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: User interface » views.module
Issue tags: +VDC

Like translators, I think I'd need to see some screenshots of these in context to understand the change.

hass’s picture

See #5, please.

xjm’s picture

Issue tags: +Needs screenshots

Thanks @hass. Could we get before-and-after screenshots of all the changed strings?

disasm’s picture

Status: Needs review » Needs work
FileSize
9.73 KB

attached is a re-roll for VDC. locale.views.inc and new.html do not exist, so it contains all the changes in the previous patch except those two remaining.

disasm’s picture

Status: Needs work » Needs review
irunflower’s picture

Status: Needs work » Needs review
FileSize
7.18 KB
7.83 KB
8.58 KB
9.09 KB
9.36 KB
10.6 KB

Patch #16 applied successfully. Below is the one of the changes made. Attached are some more examples.

Before Patch:
Comment Approval Before Patch

After Patch:
Comment Approval After Patch

*I am not sure why the after image is not showing up..

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok agree this is an improvement. Good to go.

hass’s picture

Issue tags: +Needs backport to D7

Just for the notes. Patch in #9 is for D7 to solve the issues there, too.

xjm’s picture

Issue tags: -Needs backport to D7

Changing this string at this point in the release cycle is likely to break more translations than it helps, so removing the backport tag. See: http://drupal.org/node/1527558

The strings are a little more verbose, but also a bit clearer. We talked a bit about ideas to redesign the handler lists a little so we don't have "Comment: foo for comment" repeated over and over and over again, but probably out of scope for this issue. I think this is probably an improvement as it is.

hass’s picture

Issue tags: +Needs backport to D7

Heck, how often do I need to explain this. This are critical bugs for translators like me. We are NOT *able* to translate the strings properly in D7. This are bugs and therefore requires a backport! Views has no complete translation. It's clearly wrong that this will break anything. I guess this strings are not yet translated and if they are, they are wrong (like in german) and not fixable. The patch only repairs tons of broken strings.

xjm’s picture

@hass, http://localize.drupal.org/translate/downloads?project=views shows all the translations that have already been created. If we change these strings now, any of these existing translations of the Views UI will suddenly be broken on production sites using those translations. I checked and found several translations that include "Approve link," for example; Spanish, Swedish, Dutch, Chinese... And this doesn't even take into account translations that were not contributed on l.d.o.

I agree that it is a bug that should be fixed, but backporting it is a much less clear-cut decision to make. It has other ramifications.

xjm’s picture

Also, @dawehner pointed out that these are potentially not only admin-facing strings because they are used as default values for field labels.

hass’s picture

See #3, please. The meaning of these ultra short strings is not unique. We have translations of these strings in German too, but very many of them are simply wrong, missleading and cannot understood. #3 is the extreme case example.

In general, views is a very large module and has a lot of strings. But with 6 years drupal translation expierence I know how bad these are and how extremly difficult to translate. Views is not ready for core if it comes to translatable strings! It's far away from core string quality. There is a lot of outstanding UI review work required to make views admin/user friendly and translatable. If the maintainer likes it or not. Translators are reading Views strings and they can often not guess what a string means. I'm not alone with this problems. This is why views is very incomplete in German and I really tried to translate all top 10 modules to be 100% complete. Strings must be long enough and need to be translatable without the context they are used in.

Strings that are broken must be fixed. String freeze or not - does not matter. We can translate them on d.o within seconds and have the benefit that these are ready then D8 comes out of the door.

xjm’s picture

Again, it is being fixed in D8. :) The issue is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/node.views.incundefined
@@ -159,18 +159,18 @@ function node_views_data() {
-    'title' => t('Published or admin'),

Isn't this one "published status or admin user"? There's no 'admin status'.

+++ b/core/modules/node/node.views.incundefined
@@ -203,7 +203,7 @@ function node_views_data() {
-      'label' => t('Sticky'),

Sticky is more of a flag than a status?

+++ b/core/modules/user/user.views.incundefined
@@ -285,7 +285,7 @@ function user_views_data() {
   $data['users']['status'] = array(
-    'title' => t('Active'), // The item it appears as on the UI,

While we're touching this line could we put the comments above the line per coding standards?

xjm’s picture

Sticky is more of a flag than a status?

Well, all three are under "publishing options," and flag might be confused with the flag module. I think status is fine so that it is parallel with published and promoted.

While we're touching this line could we put the comments above the line per coding standards?

And, for that matter, fix the grammar:
// The item as it appears in the UI.

But actually, that doesn't make any sense to me either. The comment is superfluous and misleading; let's just delete it.

xjm’s picture

Tagging novice for the cleanup.

esoteric1’s picture

cleaning it up at the moment. will post a patch.

esoteric1’s picture

esoteric1’s picture

Status: Needs work » Needs review
esoteric1’s picture

The interdiff between 31 and 16:

Status: Needs review » Needs work

The last submitted patch, drupal-translatable_strings_not_self_speaking-1779658-33.patch, failed testing.

esoteric1’s picture

The above comment with the interdiff shouldn't have had a .patch extension. oops.

esoteric1’s picture

Status: Needs work » Needs review
YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I checked the recent work by esoteric1: looking good!
The interdiff looks correct and the new patch applies when I try it locally, and the testbot likes it too.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/user.views.incundefined
@@ -117,7 +117,7 @@ function user_views_data() {
-    'title' => t('E-mail'), // The item it appears as on the UI,
+    'title' => t('Mail'),

All of these changes make sense, except for this one. Didn't we just *remove* valuable context to what "type" of mail we're talking about here?

hass’s picture

Do we have a core rule that says it need to be written as "email" or "e-mail" or "mail". I had no idea about such a rule and just sychronized them insude of views, but I think the "e-" schould be added everywhere in core.

xjm’s picture

Agreed with #38. Our text standards indicate that "e-mail" is the correct form.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
9.79 KB

re-rolled as patch did not apply any more.
no conflicts, it did a 3 way merge and it looked ok to me.

then, changed the published or admin status to published status or admin user to cover the feedback from @catch in #27 and Mail to E-mail according to #39 and #40.

hass’s picture

There is one help text that is written as "Email" and not "E-mail". We should change this one, too

YesCT’s picture

+++ b/core/modules/user/user.views.incundefined
@@ -116,7 +116,7 @@ function user_views_data() {
     'help' => t('Email address for a given user. This field is normally not shown to users, so be cautious when using it.'), // The help that appears on the UI,

I think the suggestion is to change this line.

Probably also would mean removing the comment on that line.

YesCT’s picture

If there is not already, a follow-up could be opened to clean up comment standards in core/modules/user/user.views.inc

hass’s picture

We do not need a follow up. Let's make it in one patch, please.

YesCT’s picture

Status: Needs review » Needs work

hass, for that one line, yes, we can use this issue.

But the whole file needs clean up, that should definitely be a separate follow-up issue. We do not want to delay getting this functionality in for you.

needs work to address #43

mitron’s picture

Fix for #43 plus one extra change from Email to E-mail.

mitron’s picture

Status: Needs work » Needs review
YesCT’s picture

Status: Needs review » Reviewed & tested by the community

looks good, if the bot says green.

hass’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.views.incundefined
@@ -119,7 +119,7 @@ function comment_views_data() {
   $data['comment']['mail'] = array(
     'title' => t('Mail'),

E-mail

mitron’s picture

Status: Needs work » Needs review
FileSize
296 bytes
10.35 KB

@hass: Good catch.

dawehner’s picture

Thanks for pushing this forward!

+++ b/core/modules/comment/comment.views.incundefined
@@ -258,46 +258,46 @@ function comment_views_data() {
+  // Link to approve comment.

If we are here already, please let's us remove this silly and more or less useless comments everywhere.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

everywhere, or just surrounding the lines we are changing?
I say leave that for follow-up of generally cleaning up this file *everywhere*.

@mitron I'm not sure why, but that interdiff looks a little strange. How are you generating the interdiff?

mitron’s picture

Assigned: Unassigned » mitron

This may take a little while. I'm working on cleanup of all three files.

YesCT’s picture

mitron’s picture

Assigned: Unassigned » mitron
Status: Reviewed & tested by the community » Needs review
FileSize
29.74 KB

First pass.

@YesCT: Was making interdiff with diff old.patch new.patch. Read Creating an interdiff after the second patch was already written so no interdiff included. Plus there are really too many changes here for an interdiff to be useful.

Edit: Please ignore this patch. It belongs in #1912946: Clean up comments in comment.views.inc node.views.inc user.views.inc

mitron’s picture

Status: Reviewed & tested by the community » Needs review

@YesCT: Too late. What do I do now?

mitron’s picture

Assigned: mitron » Unassigned
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's ignore the patch in 56 for now?

@mitron
I hope you will be able to rerole the patch in 56 once that one is on, back to RTBC.

mitron’s picture

YesCT’s picture

So, to help the committer that comes to look at this issue: update the issue summary to point to which patch we want them to commit (51).

actually, in addition, a little more detail using the issue summary template might be nice here anyway. (http://drupal.org/node/1155816)

thanks for posting the patch on the other issue.

Plus there are really too many changes here for an interdiff to be useful.

yep. thats why we want it be a separate issue too. :)

mitron’s picture

Updated issue summary requesting application of patch in #51.

Can someone else add more complete information about the issue to the summary? The "action items" at the end of the comment stream were clear enough to update the patch without understanding the motivation, so I'm not sure I can do this.

webchick’s picture

Assigned: mitron » Unassigned
Status: Needs review » Fixed

These look like good clean-ups to me, thanks!

Committed and pushed #51 to 8.x.

hass’s picture

Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 7.x-3.x-dev
Component: views.module » Code
Status: Fixed » Patch (to be ported)

Thanks for fixing these bugs in core. Now we just need to backport this bugfixes.

YesCT’s picture

Status: Needs review » Active

@hass I'm sorry to say it looks like we wont be backporting this. because it will break too many live sites.

String freeze has long been in effect for 7.x

I dont know what compromise we might be able to make.

hass’s picture

Status: Patch (to be ported) » Active
Issue tags: -Novice

Critical string bugs have always been fixed and this is only contrib. Most of these strings are not translated yet or are completly wrong translated. Some of these wrong strings can cause major harm (data loss) to site owners. l10n_update will manage the update. There is no blocking reason not to fix these strings in views D7. They are also only in admin section and not in the public area.

dawehner’s picture

They are also only in admin section and not in the public area.

That's technical wrong. If you have a field shown with the default label, it get's exposed to the user, so if you have a view with default labels, these kind of patches would change it.

Hopefully you don't argue against that, because then you care more about strings then users, sorry but you sometimes kind of overreact :(

hass’s picture

I may forgotten this small detail. However, the example is still valid for what I opened the case. This is critical as the translation of t('Cancel link') and others is completely wrong and cannot fixed by translators as this string requires a context that it do not have - or only have now in D8.

I don't understand why we need to discuss this again and again just for a minor reason that is named string freeze. We have also fixed bad and broken core strings post final release. There is zero reason to hold such patches back. I fixed thousands of strings in contrib POST final releases because they are not translatable, have bad usability or other reasons. I do not like translate a bad string and later a good string. I fix strings first and then translate them.

Views administration for beginners (and others) is bad UX (also in English), if you like it or not - doesn't change the UX and usability issues in views. Users do not understand views administration. I provided some patches for D7 in the beginning of this case, but we should re-role the last D8 patch against D7 to get these strings ready and fixed and translated in D7 times so they are ready when D8 comes out and not to forget the usability in D7 can only *win*.

hass’s picture

Status: Active » Needs review
FileSize
10.61 KB

Here is the D7 version of #51.

mitron’s picture

If all the translation of these strings are wrong, then we should be able to break string freeze. It does not matter since the translations are all wrong. However, it they are wrong in two languages out of 100, then no way should we break string freeze. How can we know?

hass’s picture

These strings are not translated in 100 languages. We are talking about guessed 5-10 at very maximum because they are not translatable without the context. There is no views translation around that is complete... None! German is the most complete as I know.

mitron’s picture

So then does that not make a strong argument that we CAN apply this patch to D7?

YesCT’s picture

Status: Active » Needs review

thanks for the d7 patch.

This is just contrib in D7. But it's views, which is in a lot of sites.
Is there any way to show data for how many translations there are for these (seems that might be posible), and how many are correct or incorrect (I have a doubt)?

Maybe we can grep and get some data.
It would be nice to get another translator or admin of a D7 site to weigh in here.

is there another issue number we can look at that was a string fix in contrib (maybe in views)?

hass’s picture

@YesCT: Try this ~100 cases or Translatable string review from my random search.

Views has also not received a string review before a release. It's not worth do discuss this from my point of view; if this can be committed or not. It must be committed.

tobiasb’s picture

That are just strings, no big api-changes, so lets fix the few strings in views. ;-)

mitron’s picture

If this is committed and included in the next Views release, these strings will appear in English once the new version of views is installed on a site until
1) These new strings are translated.
2) A new translation is released.
3) The translation is downloaded and installed on the site.

A quick survey of http://localize.drupal.org/translate/language drilling into views looks like these strings are already in English for most languages because the .po files are incomplete and do not include these strings.

YesCT’s picture

xjm's last objection was in comment #23

Let's see see if those objections still exist.

It's more work, but a compromise (where no side is really happy) might be to locate which strings are not translated in translations yet, and do the patch for those.

David_Rothstein’s picture

I'm not sure if this helps, but note that you don't need to change the user-visible string itself in order to provide context for translators.

For example, instead of this:

-      'title' => t('Edit link'),
+      'title' => t('Link to edit comment'),
       'help' => t('Provide a simple link to edit the comment.'),

You can just do this:

-      'title' => t('Edit link'),
+      'title' => t('Edit link', array(), array('context' => 'Link to edit comment')),
       'help' => t('Provide a simple link to edit the comment.'),

And not change the string at all.

(I actually wonder if this should be reconsidered for D8 as well, since some of the new strings that were added here look unnecessarily verbose.)

In any case, the reason it might not help the current discussion is that I think it still gets treated as a string breakage; if there's no translation for t('Edit link', array(), array('context' => 'Link to edit comment')) it looks like the Locale module will not fall back on t('Edit link') but rather just treat it as untranslated. But I'm mentioning it anyway, in case I'm wrong.

Someday we really need to find a way to make the whole system smarter, so that backporting string changes isn't so contentious... For example, I heard a while ago that Wordpress can actually keep track of the mapping between string changes, so if t('Edit link') got changed to t('Link to edit comment') at some point and if there is no translation for 'Link to edit comment' available, it knows that it should look up the translation for 'Edit link' as a backup... or something like that. We should seriously think about doing that - anyone want to create an issue and look into it? :)

mitron’s picture

I don't see how WordPress could link a prior phrase to translate with an updated phrase. The function to translate in Wordpress is __('message'). If 'message' is found, it is translated. Otherwise, 'message' is returned. If in the prior version, it was __('old') and in the new version it is __('new') there is no way for the function to connect 'new' with 'old.'

http://codex.wordpress.org/Translating_WordPress

hass’s picture

Note that we should not use "context" if there is any other way to solve issues. Context does not scale.

hass’s picture

Found one more bug, but made a new case as it's easier to commit. See #1974208: Rename "Cancel" to "Cancel account" to help translators., please.

dawehner’s picture

Thanks for creating a new issue!

heddn’s picture

tagging

heddn’s picture

Issue summary: View changes

Tell committer which patch to apply

Chris Matthews’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The 6 year old D7 patch in #69 does not apply to the latest views 7.x-3.x-dev.

Checking patch modules/comment.views.inc...
error: while searching for:

  // mail
  $data['comment']['mail'] = array(
    'title' => t('Mail'),
    'help' => t('Email of user that posted the comment. Will be empty if the author is a registered user.'),
    'field' => array(
      'handler' => 'views_handler_field',
      'click sortable' => TRUE,

error: patch failed: modules/comment.views.inc:135
error: modules/comment.views.inc: patch does not apply
Checking patch modules/node.views.inc...
error: while searching for:

  // published status
  $data['node']['status'] = array(
    'title' => t('Published'),
    'help' => t('Whether or not the content is published.'),
    'field' => array(
      'handler' => 'views_handler_field_boolean',

error: patch failed: modules/node.views.inc:144
error: modules/node.views.inc: patch does not apply
Checking patch modules/user.views.inc...
error: while searching for:
  // mail
  // Note that this field implements field level access control.
  $data['users']['mail'] = array(
    'title' => t('E-mail'), // The item it appears as on the UI,
    'help' => t('Email address for a given user. This field is normally not shown to users, so be cautious when using it.'), // The help that appears on the UI,
    'field' => array(
      'handler' => 'views_handler_field_user_mail',
      'click sortable' => TRUE,

error: patch failed: modules/user.views.inc:133
error: modules/user.views.inc: patch does not apply
Andrew Answer’s picture

hass’s picture

Status: Needs review » Reviewed & tested by the community

  • DamienMcKenna committed e10d893 on 7.x-3.x
    Issue #1779658 by hass, mitron, esoteric1, YesCT, disasm, Andrew Answer...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
Parent issue: » #2960871: Plan for Views 7.x-3.23 release

Committed. Thanks everyone.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.