256628 - change wording of Recheck Page button in spell checker

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[sairuh (rarely reading bugmail)]

tested with 200408230x-0.8 on linux and mac; couldn't find an existing bug
(either tbird or moz mail) for this.

in the Spell Check dialog for thunderbird, shouldn't the Recheck Page button be
something like Recheck Message instead? I imagine this dialog was borrowed from
mozilla, so Recheck Page was from the composer (web page editor) app...

Comment 1

[sairuh (rarely reading bugmail)]

hrm, just realized that this might have an impact on l10n, and might not make it
for 1.0...

Comment 2

[Magnus Melin [:mkmelin]]

*** Bug 289404 has been marked as a duplicate of this bug. ***

Comment 3

[u88484]

Attachment: patch

Changes:

-<!ENTITY recheckButton.label "Recheck Page">
-<!ENTITY recheckButton.accessKey "P">
+<!ENTITY recheckButton.label "Recheck Message">
+<!ENTITY recheckButton.accessKey "M">

Comment 4

[Scott MacGregor]

Comment on attachment 194967 [details] [diff] [review]
patch

this then makes the spell check dialog in editor say recheck message instead of
page

Comment 5

[Scott MacGregor]

please don't nominate things like this for stop ship bug status. Ask if you need
help understanding what these flags mean. Thanks.

Comment 6

[u88484]

Scott can you point me to the right file then because I searched and searched
with  lxr and that is the only place 'Recheck Page' is found. 

Comment 7

[u88484]

*** Bug 322194 has been marked as a duplicate of this bug. ***

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

http://lxr.mozilla.org/seamonkey/source/editor/ui/locales/en-US/chrome/dialogs/EditorSpellCheck.dtd#64
which is used in
http://lxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdSpellCheck.xul#73

Comment 9

[u88484]

(In reply to comment #8)
> http://lxr.mozilla.org/seamonkey/source/editor/ui/locales/en-US/chrome/dialogs/EditorSpellCheck.dtd#64
> which is used in
> http://lxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdSpellCheck.xul#73
> 

That would still change it in editor.

Mscott, is there anyway to change this without also changing in editor? Like an ifdef or something?

Comment 10

[u88484]

Well I can't figure this out without help so removing myself as the assignee in hopes someone else can take this bug.

Comment 11

[Wayne Mery (:wsmwk)]

trivial patch. needs new owner

Comment 12

[Steve Simms]

Looking through the comments and the patch, it looks like the same button (or dialog?) is being used in two places, at least in SeaMonkey.

Is this also the case for Thunderbird?  If not, this patch will just need to get refreshed, and otherwise appears to be fine.

Comment 13

[Phil Ringnalda (:philor)]

What comment 4 through comment 9 are saying is that this dialog uses exactly the same XUL and DTD in SeaMonkey's Composer, SeaMonkey's mail, and Thunderbird. Because the answer to comment 9 is "no, you can't #ifdef in a localized file" fixing it either requires forking the DTD file to change one word, which I hope we wouldn't accept a patch to do, or rephrasing it to something like "recheck document" which will work whether the document is an email or an HTML page.

Comment 14

[Wayne Mery (:wsmwk)]

In the spirit of shortness, how about just "recheck" or restart?

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 194967 [details] [diff] [review]
patch

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/256628.diff 
(Stripping trailing CRs from patch.)
patching file EditorSpellCheck.dtd
Hunk #1 FAILED at 56.
1 out of 1 hunk FAILED -- saving rejects to file EditorSpellCheck.dtd.rej

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Kurt, any luck for a new patch?

Comment 17

[u88484]

No one has come up with a definitive answer on what to do here.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #17)
> No one has come up with a definitive answer on what to do here.

Getting our UI guru on board... :)

Comment 19

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #14)))
> In the spirit of shortness, how about just "recheck" or restart?

Comment 13 suggests "Recheck document". I'd agree with Wayne's implied statement that this might be a little too long, more so in other languages where "recheck" is already longer than EN, e.g. DE:
("Recheck Page")
 "Seite erneut prüfen"
 "Dokument erneut prüfen"
("Recheck Document")

Just "Recheck" might not be clear enough about the scope of rechecking - will it just recheck the word in the input box which is very close? Given that rechecking will actually dump all your previous judgements about ignoring words and what not, I think this should be a case of ux-error-prevention.

As a compromise (hoping it will turn out a bit shorter in most languages), what about this?
 "Recheck all"       <- my suggestion (EN)
("Recheck document") <- for comparison

 "Alles erneut prüfen"     <- my suggestion (DE)
("Dokument erneut prüfen") <- for comparison

Wayne?

Comment 20

[Wayne Mery (:wsmwk)]

Yes

Comment 21

[Thomas D. (:thomas8)]

Attachment: 18064.patch v.2: change Recheck Page label to Recheck All

Suggested reviewer Ehsan doesn't do reviews, so I'll pick mkmelin. Pls adjust as needed.

Richard, pls see comment 19 for ui-review, to which Wayne agreed in comment 20.
We're changing the label not just for TB, but for any consumers of Editor, that's why we chose a short and neutral one-fits-all label.

Access keys (for EN-US)

+<!ENTITY recheckButton2.label "Recheck All">
- all potential access keys are currently taken, and make sense where they are
- duplicate access keys are unwanted
- so we need to steal one from a less frequently used element:

-<!ENTITY checkwordButton.accessKey "k">
+<!ENTITY checkwordButton.accessKey "W">
(no need to change identifier because this secondary change is only applicable for EN-US where we are changing it just now; other locales will need to juggle around with their own access keys so there's no need to notify anyone about this change)

[Check Word] had "k", now "W" - this is for the imo rare scenario when you replace a word with a totally different word during spell-check, AND want to spell-check the new word right here in the box

> +<!ENTITY recheckButton2.accessKey "k">
[Recheck All] now has "k" and identifiers updated to notify localizers

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8626900 [details] [diff] [review]
18064.patch v.2: change Recheck Page label to Recheck All

ui-+ but this doesn't work. You have only changed the entities in the language file but you need also to change the XUL file which uses the entities. See https://dxr.mozilla.org/comm-central/search?q=recheckButton.label

Comment 23

[Thomas D. (:thomas8)]

Attachment: patch v.2.1

(In reply to Richard Marti (:Paenglab) from comment #22)
> Comment on attachment 8626900 [details] [diff] [review]
> 18064.patch v.2: change Recheck Page label to Recheck All
> 
> ui-+ but this doesn't work. You have only changed the entities in the
> language file but you need also to change the XUL file which uses the
> entities. See
> https://dxr.mozilla.org/comm-central/search?q=recheckButton.label

Of course, thanks.

Comment 24

[Jorg K (CEST = GMT+2)]

Please note the following:
This is *not* an M-C core editor bug.
The source files clearly live in C-C (https://dxr.mozilla.org/comm-central/source/editor/ui).
Ehsan is an M-C developer, he doesn't do C-C reviews.

Why did you change recheckButton.label to recheckButton2.label? Why not just change the value?

Comment 25

[Thomas D. (:thomas8)]

(In reply to Jorg K from comment #24)
> Please note the following:
> This is *not* an M-C core editor bug.
> The source files clearly live in C-C

Yeah, thanks! I'm glad to be corrected and learn more, as I had my doubts after I set it...

> (https://dxr.mozilla.org/comm-central/source/editor/ui).
> Ehsan is an M-C developer, he doesn't do C-C reviews.

Well, I didn't ask him anyway and obviously reviewers differ according to products/components. Fwiw, Ehsan according to his current BMO self-description does not do *any* reviews atm, perhaps because his review queue is already long.
>  Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!)

> Why did you change recheckButton.label to recheckButton2.label? Why not just
> change the value?

Tradition has it that developers claim it's necessary to change the identifier/name when we change the value of such entities, supposedly because it's said to be the only reliable way of notifying localizers about the need for a new translation. I totally agree with Jorg and I'm on record that this practice is unexpected; in fact, it's very bad as it should never be necessary to change the name of the entity when we're just tweaking the value. If this nasty tradition is still needed, it's really bad design in the localisation system. Jorg, you'll be hero if you offer evidence that this is no longer needed, or rather, if you make the responsible folks adapt the localisation system to work without this unbelievable hack...

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #25)
> ... evidence that this is no longer needed, ...

I know nothing about localisation, so I leave it to Magnus and his review.

Sadly I read here:
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices
"Changing the string ID ... is the only reliable method to ensure that localizers update existing localizations."

Since the change is rather minor, it wouldn't be a drama if one translation were missed.

Comment 27

[Thomas D. (:thomas8)]

(In reply to Jorg K from comment #26)
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices

Thanks for the link to documentation!

> "Changing the string ID ... is the only reliable method to ensure that
> localizers update existing localizations."

Sad indeed.

> Since the change is rather minor, it wouldn't be a drama if one translation
> were missed.

Minor or not, we're changing this for a reason, and we want the new strings in every translation, so until there's a better universal system for localization, we'll stick to the procedure as a matter of principle and product quality. Also to avoid further duplicates of this bug filed against other localizations who missed the string update because we didn't alert them via ID change.

Comment 28

[Thomas D. (:thomas8)]

Magnus, ping for reviewing this 4-line string change patch?

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8627000 [details] [diff] [review]
patch v.2.1

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

I'm not a reviewer for editor/. Try neil ...

And yes, when we change meanings we have to update the localization key in the entity file. Nobody likes it much but it's the only bulletproof way anyone's come up with so far.

::: editor/ui/locales/en-US/chrome/dialogs/EditorSpellCheck.dtd
@@ +9,4 @@
>  <!ENTITY wordEditField.label "Replace with:">
>  <!ENTITY wordEditField.accessKey "w">
>  <!ENTITY checkwordButton.label "Check Word">
> +<!ENTITY checkwordButton.accessKey "W">

w is already taken (replace _w_ith) two lines above

@@ +28,4 @@
>  <!ENTITY addToUserDictionaryButton.accessKey "d">
>  <!ENTITY editUserDictionaryButton.label "Edit…">
>  <!ENTITY editUserDictionaryButton.accessKey "E">
> +<!ENTITY recheckButton2.label "Recheck All">

I'm not sure this is any cleared
How about "Recheck Text" ?

Comment 30

[Thomas D. (:thomas8)]

Attachment: Patch v. 3 - Recheck Text

Hi Neil (suggested reviewer per Magnus comment 29), a one-line string fix patch for your review, tia :)

Richard, "Recheck Text" per Magnus comment 29 sounds good, clear and sufficiently neutral to me, and you?

Comment 31

[Thomas D. (:thomas8)]

Attachment: Patch v. 3.1 - [Recheck Page] → [Recheck Text]

(sorry, last patch was incomplete)

Hi Neil (suggested reviewer per Magnus comment 29), a one-line string fix patch for your review, tia :)

Richard, "Recheck Text" per Magnus comment 29 sounds good, clear and sufficiently neutral to me, and you?

Comment 32

[Richard Marti (:Paenglab)]

Comment on attachment 8654521 [details] [diff] [review]
Patch v. 3.1 - [Recheck Page] → [Recheck Text]

Yeah, this is okay.

Comment 33

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a5e9eff60b9cc5350c5f4d6b566eef885b1accdf
Bug 256628 - Change "Recheck Page" button label to "Recheck Text" in spell check dialog. r=neil,ui-r=paenglab