463869 - Remove obsolete/unused entities in Editor string files

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Simon Paquet [:sipaq]]

Attachment: Obsolete strings in editor/ui

+++ This bug was initially created as a clone of Bug #461112 +++

Using an optimized version (from Philipp Kewisch) of the script mentioned in http://www.chrisfinke.com/2008/04/22/finding-unused-entities-in-your-firefox-extensions/ I was able to identify lots of unused strings in editor/ui/locales

Those should be removed to lessen the localization burden for new localizers and to reduce the build size. 

CC'ing philor and KaiRo as likely reviewers, since this patch also touches a SeaMonkey file. The attached patch removes 98 strings on the Thunderbird side and 74 on the SeaMonkey side.

I'll attach a patch that only deals with .dtd files first. A .properties patch will follow later.

Comment 1

[Simon Paquet [:sipaq]]

Comment on attachment 347121 [details] [diff] [review]
Obsolete strings in editor/ui

Asking for a second review from KaiRo, as this patch also touches one SeaMonkey file.

Comment 2

[Phil Ringnalda (:philor)]

Comment on attachment 347121 [details] [diff] [review]
Obsolete strings in editor/ui

Looks like the copy-paste into debugQAEditorOverlay.dtd broke the UTF-8 ellipses.

Other than that, I'm happy to see unused strings go away, but I don't think I own these. I've always treated editor/ui as r+sr=neil, since he's pretty much the only active editor peer.

Comment 3

[Vlado Valastiak [:wladow]]

Comment on attachment 347121 [details] [diff] [review]
Obsolete strings in editor/ui

As KaiRo knows, I played with this script a bit lately, here are my nits:

 <!-- For a "Paste" submenu when more than 1 
      clipboard formats are available -->
-<!ENTITY pasteHTMLCmd.label "HTML">
-<!ENTITY pasteHTMLCmd.accesskey "H">
 <!ENTITY pasteTextCmd.label "Text">
 <!ENTITY pasteTextCmd.accesskey "T">
 <!ENTITY pasteImageCmd.label "Image">
 <!ENTITY pasteImageCmd.accesskey "I">
 <!ENTITY pasteRowsCmd.label "Rows">
 <!ENTITY pasteRowsCmd.accesskey "R">
 <!ENTITY pasteColumnsCmd.label "Columns">
 <!ENTITY pasteColumnsCmd.accesskey "C">

All these can go out as they are not implemented, see http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editorOverlay.xul#430
We should remove the code too. And with this we can remove also InitPasteAsMenu function from http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#2001



+++ b/editor/ui/locales/en-US/chrome/composer/pref-composer.dtd
@@ -1,39 +1,27 @@
 <!-- extracted from content/pref-composer.xul -->
 
 <!--LOCALIZATION NOTE : FILE 'Composer' prefs dialog. Similar to Communcator 4.x Document Properties/Colors and Background -->
 
 <!--LOCALIZATION NOTE (pref.composer.title): DONT_TRANSLATE -->
 <!ENTITY  pref.composer.title           "Composer">
-
 <!ENTITY  saving                        "Saving">
-<!ENTITY  AutoSaveCheck                 "Automatically save every">
-<!ENTITY  minText                       "minutes">
-
-<!ENTITY  exterLegend.label             "External Editors">
 <!ENTITY  htmlSource                    "HTML Source:">
-<!ENTITY  imageeditor                   "Images:">
-<!ENTITY  chooseButton.label            "Choose">

saving and htmlSource are obsolete too.



+++ b/editor/ui/locales/en-US/chrome/dialogs/EditorPublish.dtd
@@ -42,22 +42,19 @@
 
 <!ENTITY publishTab.label             "Publish">
 <!ENTITY settingsTab.label            "Settings">
 
 <!ENTITY publishButton.label          "Publish">
 
 <!-- Publish Tab Panel -->
 <!ENTITY siteList.label               "Site Name:">
-<!ENTITY siteList.accesskey           "e">
-<!ENTITY siteList.tooltip             "Choose the site that you want to publish to">
 <!ENTITY newSiteButton.label          "New Site">
 <!ENTITY newSiteButton.accesskey      "N">
 <!ENTITY docDirList.label             "Site subdirectory for this page:">
-<!ENTITY docDirList.accesskey         "S">
 <!ENTITY docDirList.tooltip           "Choose or enter the name of the remote subdirectory for this page">

We should rather use them, like this:

> diff --git a/editor/ui/dialogs/content/EditorPublish.xul b/editor/ui/dialogs/content/EditorPublish.xul
> --- a/editor/ui/dialogs/content/EditorPublish.xul
> +++ b/editor/ui/dialogs/content/EditorPublish.xul
> @@ -76,10 +76,13 @@
>            <columns><column/><column/><column/></columns>
>            <rows>
>              <row align="center">
> -              <label value="&siteList.label;"/>
> +              <label value="&siteList.label;"
> +                     accesskey="&siteList.accesskey;"
> +                     control="SiteList"/>
>                <!-- Contents filled in at runtime -->
>                <menulist id="SiteList" 
>                  style="min-width:18em; max-width:18em;" crop="right"
> +                tooltiptext="&siteList.tooltip;"
>                  oncommand="SelectSiteList();"/>
>                <hbox>
>                  <button label="&newSiteButton.label;"
> @@ -106,7 +109,9 @@
>            </rows>
>          </grid>
>          <spacer class="spacer"/>
> -        <label value="&docDirList.label;"/>
> +        <label value="&docDirList.label;"
> +               accesskey="&docDirList.accesskey;"
> +               control="DocDirList"/>
>          <hbox align="center">
>            <!-- Contents filled in at runtime -->
>            <menulist id="DocDirList" class="minWidth20 uri-element" editable="true"  flex="1"



 <!ENTITY password.label               "Password:">
 <!ENTITY password.accesskey           "w">
-<!ENTITY password.tooltip             "The password associated with your user name">
 <!ENTITY savePassword.label           "Save Password">

The same here with password.tooltip, use this f.e.:

> +++ b/editor/ui/dialogs/content/EditorPublishOverlay.xul
> @@ -82,7 +82,8 @@
>                   control="PasswordInput"/>
>            <hbox>
>              <textbox id="PasswordInput" type="password" class="MinWidth5em" 
> -                     oninput="onInputSettings();"/>
> +                     oninput="onInputSettings();"
> +                     tooltiptext="&password.tooltip;"/>
>              <checkbox id="SavePassword" label="&savePassword.label;"
>                        accesskey="&savePassword.accesskey;" 
>                        tooltiptext="&savePassword.tooltip;"


And take care of patch encoding, this one is in CP1250.

Comment 4

[Simon Paquet [:sipaq]]

Attachment: Obsolete strings in editor/ui - dtd part - v2

New patch that incorporates the comments from Phil and Wladow. I'm asking Neil for review, since editor/ seems to be his domain and the code affects both Thunderbird and SeaMonkey.

Comment 5

[[e-mail]]

Comment on attachment 347155 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v2

>diff --git a/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd b/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd
>--- a/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd
>+++ b/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd
>@@ -1,7 +1,2 @@
>-
>-<!ENTITY editPageCmd.label                "Edit Page in Composer"> 
>-<!ENTITY editPageCmd.accesskey            "e">
>-
> <!ENTITY editLinkCmd.label                "Edit Link in Composer">
> <!ENTITY editLinkCmd.accesskey            "E">
>-
We don't use this version either, we use the one in editorOverlay.dtd

Comment 6

[[e-mail]]

Comment on attachment 347155 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v2

I saw an entity for an unused inline spellchecking preference. I'll need to see if it's likely that we might want such a preference.

>+<!ENTITY statusText.label "Done loading page">
Not used?

>+<!ENTITY insertFormMenu.label "Form">
I don't really want to move the form menu entities here, I want to be able to move the form menu from debug to Insert...

>+<!ENTITY NormalAbbr.label "Text">
>+<!ENTITY ParagraphAbbr.label "P">  
>+<!ENTITY Heading1Abbr.label "H1">
>+<!ENTITY Heading2Abbr.label "H2">
>+<!ENTITY Heading3Abbr.label "H3">
>+<!ENTITY Heading4Abbr.label "H4">
>+<!ENTITY Heading5Abbr.label "H5">
>+<!ENTITY Heading6Abbr.label "H6">
>+<!ENTITY BlockquoteAbbr.label "BQ">
>+<!ENTITY AddressAbbr.label "Addr.">
>+<!ENTITY PreformatAbbr.label "Pre.">
Not used?

Comment 7

[Simon Paquet [:sipaq]]

(In reply to comment #5)
> >--- a/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd
> >+++ b/editor/ui/locales/en-US/chrome/composer/editorNavigatorOverlay.dtd
> >@@ -1,7 +1,2 @@
> >-
> >-<!ENTITY editPageCmd.label                "Edit Page in Composer"> 
> >-<!ENTITY editPageCmd.accesskey            "e">
> >-
> > <!ENTITY editLinkCmd.label                "Edit Link in Composer">
> > <!ENTITY editLinkCmd.accesskey            "E">
> >-
> We don't use this version either, we use the one in editorOverlay.dtd

Right. I'll add this to a v3 patch.

(In reply to comment #6)
> >+<!ENTITY statusText.label "Done loading page">
> Not used?

See http://mxr.mozilla.org/comm-central/search?string=statusText.label&tree=comm-central

Only debugQATextEditorShell.xul uses this string right now, which is why I moved it from shared editor.dtd to Suite-only debugQAEditorOverlay.dtd. 

> >+<!ENTITY insertFormMenu.label "Form">
> I don't really want to move the form menu entities here, I want to be able to
> move the form menu from debug to Insert...

Can we please do such stuff in a followup bug? I'd really like to focus on cleanup work in this bug.

> >+<!ENTITY NormalAbbr.label "Text">
> >+<!ENTITY ParagraphAbbr.label "P">  
> >+<!ENTITY Heading1Abbr.label "H1">
> >+<!ENTITY Heading2Abbr.label "H2">
> >+<!ENTITY Heading3Abbr.label "H3">
> >+<!ENTITY Heading4Abbr.label "H4">
> >+<!ENTITY Heading5Abbr.label "H5">
> >+<!ENTITY Heading6Abbr.label "H6">
> >+<!ENTITY BlockquoteAbbr.label "BQ">
> >+<!ENTITY AddressAbbr.label "Addr.">
> >+<!ENTITY PreformatAbbr.label "Pre.">
> Not used?

You're right. According to http://mxr.mozilla.org/comm-central/search?string=Abbr.label&tree=comm-central those strings aren't used at all. My bad. I'll remove them completely in the v3 patch.

Comment 8

[Simon Paquet [:sipaq]]

Attachment: Obsolete strings in editor/ui - dtd part - v3

Comment 9

[[e-mail]]

Sorry, no update on the inline spellchecking preference entities yet.

(In reply to comment #7)
> (In reply to comment #6)
> > >+<!ENTITY statusText.label "Done loading page">
> > Not used?
> Only debugQATextEditorShell.xul uses this string right now, which is why I
> moved it from shared editor.dtd to Suite-only debugQAEditorOverlay.dtd.
Sorry, I hadn't noticed it there.

> > >+<!ENTITY insertFormMenu.label "Form">
> > I don't really want to move the form menu entities here, I want to be able to
> > move the form menu from debug to Insert...
> Can we please do such stuff in a followup bug? I'd really like to focus on
> cleanup work in this bug.
I just want to be *able* to move the form menu... which means not (re)moving those entities please.

Comment 10

[Simon Paquet [:sipaq]]

(In reply to comment #9)
>>+<!ENTITY insertFormMenu.label "Form">
>
> I just want to be *able* to move the form menu... which means not 
> (re)moving those entities please.

What is hindering you to move those strings wherever you want them when 
they are in debugQAEditorOverlay.dtd instead of editorOverlay.dtd?

But I can surely move them to a more appropriate place if you tell me 
where to put them. I just don't want them lying around unused anymore. 
Such things have a tendency to become permanent as evidenced by the 
huge amount of unused strings here, in bug 463886 or in bug 461112.

Comment 11

[[e-mail]]

Comment on attachment 347190 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v3

> <!ENTITY printPreviewCmd.label "Print Preview">
Looks like this isn't really used either (commented out in the XUL).

Comment 12

[[e-mail]]

(In reply to comment #10)
>(In reply to comment #9)
>>>+<!ENTITY insertFormMenu.label "Form">
>>
>> I just want to be *able* to move the form menu... which means not 
>> (re)moving those entities please.
>What is hindering you to move those strings wherever you want them when 
>they are in debugQAEditorOverlay.dtd instead of editorOverlay.dtd?
Because the place I would want to move them would be back to editorOverlay.dtd!

Comment 13

[[e-mail]]

Comment on attachment 347155 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v2

>-<!ENTITY  spellCheckInline.label        "Check spelling as you type">
>-<!ENTITY  spellCheckInline.accesskey    "C">
As with the forms, I'd like to describe this as a potential feature rather than an obsolete string.

Comment 14

[Simon Paquet [:sipaq]]

(In reply to comment #11)
>> <!ENTITY printPreviewCmd.label "Print Preview">
>
> Looks like this isn't really used either (commented out in the XUL).

I'll remove it.

Comment 15

[Simon Paquet [:sipaq]]

(In reply to comment #12 and comment #13)
>>>>+<!ENTITY insertFormMenu.label "Form">
>
>>What is hindering you to move those strings wherever you want them when 
>>they are in debugQAEditorOverlay.dtd instead of editorOverlay.dtd?
>
>Because the place I would want to move them would be back to 
>editorOverlay.dtd!
>
>>-<!ENTITY  spellCheckInline.label        "Check spelling as you type">
>>-<!ENTITY  spellCheckInline.accesskey    "C">
>
> As with the forms, I'd like to describe this as a potential feature rather 
> than an obsolete string.

Given our really bad history with regards to not removing obsolete strings 
consistently (see this bug, bug 463886 and bug 461112), I'm really opposed 
to letting those strings stay in the products as is.

It's really only a small matter to add them back, *if* a potential feature 
or bugfix becomes a reality. So I'd ask you to reconsider this. 

Please keep in mind that every additional string costs localization time 
for a new localizer. And these guys don't have much time to spare. As 
localizing Firefox is their obvious number one priority, Thunderbird only 
comes in second place with SeaMonkey and Sunbird fighting for a distant 
third place.

My goal for Thunderbird is to reach over 50 supported locales in 2009 
(from 46 at the moment) and I know that KaiRo hopes to expand the number 
of SeaMonkey localizations significantly as well (from the current 16). 
So this (additional unneeded work for new localizers) is a real concern here.

Comment 16

[Simon Paquet [:sipaq]]

Attachment: Obsolete strings in editor/ui - dtd part - v4

This patch covers all review comments until comment 11. 

In addition I also did the following in editorOverlay.xul:
- some minor whitespace cleanup
- removed two more commented out code sections
- removed the relevant strings (insertBreakCmd.accesskey and 
  insertBreakCmd.label) in case they weren't used in another location 
  (as with size-xx-smallCmd.label and size-xx-smallCmd.accesskey)


The patch does not yet cover the issues raised in comment 12 and 
comment 13 for the reasons laid out in comment 15.

Comment 17

[Simon Paquet [:sipaq]]

Comment on attachment 347214 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v4

Ugh, totally forgot to ask for review. Please see comment 15 and comment 16 for more info on this patch.

Comment 18

[Robert Kaiser]

I'm very much for removing strings if there's not a bug+patch out there for actually using them. As for the stuff to be moved to debugQA, I think if the overlay/menuitems are in debugQA, the strings for them should be as well, or else it's even harder for people working on the code to find where the fitting strings are.

Comment 19

[[e-mail]]

Comment on attachment 347214 [details] [diff] [review]
Obsolete strings in editor/ui - dtd part - v4

Oh well, it's not as if any of this stuff has any useful blame.

Comment 20

[[e-mail]]

Hmm, does this mean that debugQA doesn't depend on editorOverlay.dtd any more?

Comment 21

[Simon Paquet [:sipaq]]

(In reply to comment #20)
> Hmm, does this mean that debugQA doesn't depend on editorOverlay.dtd any more?

Given that debugQAEditorOverlay.xul references 45 strings and the patch adds 24 strings to the 21 existing ones in debugQAEditorOverlay.dtd, the .xul file should no longer be dependent on editorOverlay.dtd.

I'll test this before checkin. If nothing breaks, I'll remove the reference to editorOverlay.dtd in debugQAEditorOverlay.xul as well.

Comment 22

[Simon Paquet [:sipaq]]

Changeset 2fae120cb481 checked in with reference to editorOverlay.dtd removed in debugQAEditorOverlay.xul.

To make this work however, I had to duplicate the two strings 

  <!ENTITY pasteAsQuotationCmd.label "Paste As Quotation">
  <!ENTITY pasteAsQuotationCmd.accesskey "Q">

from editorOverlay.dtd in debugQAEditorOverlay.dtd

Neil, thanks for the review and the consideration of comments here.

Comment 23

[Simon Paquet [:sipaq]]

Attachment: Obsolete strings in editor/ui - properties part

Here's a much smaller patch to get rid of unused strings in one .properties file 
(editor.properties).

Comment 24

[[e-mail]]

Comment on attachment 347370 [details] [diff] [review]
Obsolete strings in editor/ui - properties part

>-Yes=Yes
> No=No
> Save=Save
> DontSave=Don't Save
> More=More
> Fewer=Fewer
> Less=Less
I'm sure we can get rid of more of these. I couldn't find "DontSave" or "Fewer" for instance.

> # LOCALIZATION NOTE (TableSelectKey): DONT_TRANSLATE
> TableSelectKey=Ctrl+
>-# LOCALIZATION NOTE (XulKeyDefault): DONT_TRANSLATE
>-XulKeyDefault=Ctrl+
> # LOCALIZATION NOTE (XulKeyMac): DONT_TRANSLATE
> XulKeyMac=Cmd+
>-# LOCALIZATION NOTE (XulKeyUnix): DONT_TRANSLATE
>-XulKeyUnix=Alt+
> # LOCALIZATION NOTE (Del): DONT_TRANSLATE
> Del=Del
Surely these should in fact be translated (e.g. Strg+ in German)?

Comment 25

[Simon Paquet [:sipaq]]

Changeset 8d4708360005 pushed.

-> FIXED