change wording of Recheck Page button in spell checker

RESOLVED FIXED in Thunderbird 43.0

Status

Thunderbird
Message Compose Window
--
trivial
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: Thomas D. (currently busy elsewhere; needinfo?me))

Tracking

Trunk
Thunderbird 43.0
Bug Flags:
blocking1.8b4 -
blocking1.8b5 -
blocking1.9a1 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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...
(Reporter)

Comment 1

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

Comment 2

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

Updated

12 years ago
Flags: blocking1.9a1?
Flags: blocking1.8b5?
Flags: blocking1.8b4?

Comment 3

12 years ago
Created attachment 194967 [details] [diff] [review]
patch

Changes:

-<!ENTITY recheckButton.label "Recheck Page">
-<!ENTITY recheckButton.accessKey "P">
+<!ENTITY recheckButton.label "Recheck Message">
+<!ENTITY recheckButton.accessKey "M">
Attachment #194967 - Flags: review?(mscott)

Comment 4

12 years ago
Comment on attachment 194967 [details] [diff] [review]
patch

this then makes the spell check dialog in editor say recheck message instead of
page
Attachment #194967 - Flags: review?(mscott) → review-

Comment 5

12 years ago
please don't nominate things like this for stop ship bug status. Ask if you need
help understanding what these flags mean. Thanks.
Flags: blocking1.9a1?
Flags: blocking1.9a1-
Flags: blocking1.8b5?
Flags: blocking1.8b5-
Flags: blocking1.8b4?
Flags: blocking1.8b4-

Comment 6

12 years ago
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. 
Status: NEW → ASSIGNED

Updated

12 years ago
Assignee: mscott → supernova_00
Status: ASSIGNED → NEW

Comment 7

12 years ago
*** Bug 322194 has been marked as a duplicate of this bug. ***
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

11 years ago
(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?
QA Contact: message-compose

Comment 10

10 years ago
Well I can't figure this out without help so removing myself as the assignee in hopes someone else can take this bug.
Assignee: supernova_00 → nobody
Whiteboard: [patchlove
trivial patch. needs new owner
Whiteboard: [patchlove → [patchlove]

Comment 12

9 years ago
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.
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.
In the spirit of shortness, how about just "recheck" or restart?
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
Attachment #194967 - Attachment is obsolete: true
Kurt, any luck for a new patch?

Comment 17

8 years ago
No one has come up with a definitive answer on what to do here.
(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... :)
(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?
Flags: needinfo?(vseerror)
Yes
Flags: needinfo?(vseerror)
Component: Message Compose Window → Editor
Product: Thunderbird → Core
Created attachment 8626900 [details] [diff] [review]
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
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8626900 - Flags: ui-review?(richard.marti)
Attachment #8626900 - Flags: review?(mkmelin+mozilla)
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
Attachment #8626900 - Flags: ui-review?(richard.marti)
Attachment #8626900 - Flags: ui-review+
Attachment #8626900 - Flags: review?(mkmelin+mozilla)
Created attachment 8627000 [details] [diff] [review]
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.
Attachment #8626900 - Attachment is obsolete: true
Attachment #8627000 - Flags: ui-review+
Attachment #8627000 - Flags: review?(mkmelin+mozilla)

Updated

2 years ago
Component: Editor → Message Compose Window
Product: Core → Thunderbird
Version: unspecified → Trunk

Comment 24

2 years ago
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?
(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

2 years ago
(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.
(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.
Magnus, ping for reviewing this 4-line string change patch?
Flags: needinfo?(mkmelin+mozilla)

Comment 29

2 years ago
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" ?
Attachment #8627000 - Flags: review?(mkmelin+mozilla) → review-

Updated

2 years ago
Flags: needinfo?(mkmelin+mozilla)
Created attachment 8654518 [details] [diff] [review]
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?
Attachment #8627000 - Attachment is obsolete: true
Attachment #8654518 - Flags: ui-review?(richard.marti)
Attachment #8654518 - Flags: review?(neil)
Attachment #8654518 - Flags: ui-review?(richard.marti)
Attachment #8654518 - Flags: review?(neil)
Created attachment 8654521 [details] [diff] [review]
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?
Attachment #8654518 - Attachment is obsolete: true
Attachment #8654521 - Flags: ui-review?(richard.marti)
Attachment #8654521 - Flags: review?(neil)
Comment on attachment 8654521 [details] [diff] [review]
Patch v. 3.1 - [Recheck Page] → [Recheck Text]

Yeah, this is okay.
Attachment #8654521 - Flags: ui-review?(richard.marti) → ui-review+

Updated

2 years ago
Attachment #8654521 - Flags: review?(neil) → review+
Keywords: checkin-needed

Comment 33

2 years ago
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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
You need to log in before you can comment on or make changes to this bug.