Change button.editFeed.label to e.g. button.updateFeed.label

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Ton, Assigned: aceman)

Tracking

Trunk
Thunderbird 51.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.84 KB, patch
Magnus Melin
: review+
Ian Neal
: review+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
For some reason, button.editFeed.label is used and was introduced by bug 716706. The text is fine (Update) since it’s about updating the feed, but a number of locales including mine seem to have localized it as Edit, probably as a result of looking at the entity (or other locales) a bit too much.

https://transvision.mozfr.org/string/?entity=mail/chrome/messenger-newsblog/feed-subscriptions.dtd:button.editFeed.label&repo=aurora

Could the entity be updated so other locales can fix it accordingly?
(Assignee)

Comment 1

a year ago
The text may be wrong because when you look at the referenced bug you can see in the patch that burton.editFeed.label has changed value from Edit to Update but the entity name was not changed (as single case in feed-subscriptions.dtd).

That is incorrect in current process. Entity name must be changed if the value changes (and the change is relevant for all locales). But this wasn't done so localizers didn't notice the change and you see old text there.
Assignee: nobody → acelists
Blocks: 716706
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8776305 [details] [diff] [review]
patch
Attachment #8776305 - Flags: review?(alta88)
(Reporter)

Comment 3

a year ago
Created attachment 8776312 [details] [diff] [review]
19799.patch

When discussed on IRC, I didn't realize aceman was already on it, but this patch also replaces other internal occurrences that may need to be changed along.
(Reporter)

Updated

a year ago
Attachment #8776312 - Flags: review?(alta88)

Comment 4

a year ago
Both 'Edit' and 'Update' mean 'change', but 'Edit' implies an intermediate step while 'Update' is immediate.  The original UI opened a dialog, thus 'Edit'; when these fields were brought inline, 'Update' became more correct. I didn't change the string key then because, as you see, it adds to code confusion, and felt is was unjustified and Bad Practice to make numerous such code changes.

We should not use code change to notify localizers, as that is the Most Risky method. Instead, I will ask Ton to send an email to Tb and Sm localizers noting that they should review the key button.editFeed.label and make sure its localized value is equivalent in meaning to the en-US 'Update'. This is the Least Risky way.
(Assignee)

Comment 5

a year ago
I'm not sure why it would be the most risky method. It seems currently it is the most reliable one to notify localizers and is standard practice at all string changes. But if you can reach to localizers in another way, I have no problem with it. After you do it, please close the bug.
Thanks.

Comment 6

a year ago
Unfortunately, the standard practice is a Bad Practice, many people have complained for a long time that not all localizer tools key off a value change. The fatal yellow screen of death is the major problem.  As for the code changes patch, you really should understand why that's certainly the most risky method. Now, key changes may have been true years ago and tools are better now; there is a major effort to fix this[1] once and for all with l20n.

Posting to a localizers list is (was) certainly common, ie for late string changes etc. so that's what I've asked Ton to do.

[1] https://blog.mozilla.org/l10n/2016/07/25/l20n-in-firefox-a-summary-for-developers/

Updated

a year ago
Attachment #8776305 - Flags: review?(alta88) → review-

Updated

a year ago
Attachment #8776312 - Flags: review?(alta88) → review-
(Reporter)

Comment 7

a year ago
@alta88:
To clarify: my intention was not to inform localizers only (which is not my job either) by requesting a code change, but merely to catch 2 birds with one stone. Coding may not be my cup of tea but I can tell the current code is confusing because of the "old" internal references like you elaborated on in comment 4. I would however have wished you had changed the keys at the time.

I know Update and Change can be/mean the same and understand the old code referred to opening the dialog to Edit a value - now it’s a button "to update/apply the changed URL field’s content after editing it", which may raise questions about whether or not the button should be displayed otherwise or elsewhere for UI reasons only, but I won’t go that far. Looking at what may have caused our l10n error including those for other locales, it turned out the code probably is, or (more obvious) locales just haven’t noticed the change because of unchanged entities. That alone raises questions, so this is just a chance to fix both issues. Please also keep in mind that it’s not for en-US to decide if l10n strings should change or not in case en-US gets some slight rewordings.

Hope this clarifies. If it was up to me, I’d change the code to avoid future confusion in code changes in the first place and regardless of l10n - but be glad l10n issues would be solved along - but the point is probably clear.
Speaking with my l10n driver hat on, please stick to the standard process for now, which means changing the entity name (and possibly related code) if the meaning of the string changes in a way that localizers should be aware of. As we are not pushing this code to aurora or beta, there is the same low risk as with other patches. If a regression exists it will be found until the next release.

This has been established practice for a long time, at least for the last 10 years. The yellow screen of death is not an issue any longer because missing strings are merged with the en-US strings during the build process.

I would therefore suggest to re-request review for that patches, possibly making sure the patches are complete.
(Assignee)

Comment 9

a year ago
Magnus, please decide how to proceed here.
Flags: needinfo?(mkmelin+mozilla)

Comment 10

a year ago
I agree with Philipp, changing the entity name is standard practice, annoying as it may be.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 11

a year ago
Alta, which patch will you choose?
Flags: needinfo?(alta88)

Comment 12

a year ago
This bug is a localization process fail; some localizers got it and some didn't. An l10n driver should focus on fixing whatever tools or methods are lacking in the didn't get it group.

I have no intention of approving this, or making some lesser of two evils choice. Please find someone else.
Flags: needinfo?(alta88)
What tool could there possibly be to tell a localizer that a string has changed meaning, when neither the string nor the entity has changed? The only way to do this *is* a code change…
(Assignee)

Comment 14

a year ago
Well, in the original bug, that caused this problem, the string itself DID change, just not the entity. So a proper tool should have noticed it. And even many tools do. They even use that the entity didn't change, only the string and flag the string for review, but preserving the original translation. If the entity changes, they have mostly no choice than to just drop the previous translation. So I agree with alta88 in this. But I don't know why the mozilla translators use such improper tools. Maybe there is a technical reason. So until that is solved, we have to follow the process, however bad.

Magnus, will you choose the less evil patch? :)
Flags: needinfo?(mkmelin+mozilla)

Updated

a year ago
Attachment #8776312 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)

Comment 15

a year ago
Comment on attachment 8776305 [details] [diff] [review]
patch

Review of attachment 8776305 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go with this one. No real need to update ids and such.
Attachment #8776305 - Flags: review- → review+
(Assignee)

Comment 16

a year ago
Thanks.
Keywords: checkin-needed

Comment 17

a year ago
Comment on attachment 8776305 [details] [diff] [review]
patch

r/a=me for SM part
Attachment #8776305 - Flags: review+
https://hg.mozilla.org/comm-central/rev/ccca3341d88f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
You need to log in before you can comment on or make changes to this bug.