Closed Bug 1400646 Opened 2 years ago Closed 2 years ago

Inline spellcheck in subject and message body doesn't update immediately when the spellcheck language is changed via the "Check Spelling" panel

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(1 file, 2 obsolete files)

Inline spellcheck in subject and message body doesn't update immediately when the spellcheck language is changed via the "Check Spelling" panel.

It updates when the "Check Spelling" panel is closed and body or subject gain focus again.

This is difficult to fix since the panel is out in editor/ui/dialogs/content/EdSpellCheck.js and when changing the language we get here:
https://dxr.mozilla.org/comm-central/rev/65567743280f48caf53b55ebd423896b84bcb34a/editor/ui/dialogs/content/EdSpellCheck.js#367

This already notifies the compose window by setting the "lang" attribute on the document
https://dxr.mozilla.org/comm-central/rev/65567743280f48caf53b55ebd423896b84bcb34a/editor/ui/dialogs/content/EdSpellCheck.js#367
which triggers a re-check when the compose window gains focus again.

To avoid one window messing with the internals of another this notification mechanism was invented with the disadvantage that the update isn't instant.
(In reply to Jorg K (GMT+2) from comment #0)
> This is difficult to fix since ...
Unmögliches erledigen wir sofort, Wunder dauern etwas länger, ab morgen wird gezaubert.
https://de.wiktionary.org/wiki/Unm%C3%B6gliches_wird_sofort_erledigt,_Wunder_dauern_etwas_l%C3%A4nger

That said, this is quite ugly.
Attachment #8909081 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #1)
> That said, this is quite ugly.

Der Zweck heiligt die Mittel... cool! :)
Less hacky version.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #2)
> Der Zweck heiligt die Mittel... cool! :)
That attitude is the root of all evil :-(
Assignee: nobody → jorgk
Attachment #8909081 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8909081 - Flags: review?(acelists)
Attachment #8909153 - Flags: review?(acelists)
OK, I have played with the Spell check dialog.
If you switch language inside it, it is immediately shown also in the message status bar. You have to click Recheck text to get misspelled words in the new language in the dialog.

With the patch, also the underlining of misspelled words in the message body/subject is changed immediately when new language is selected. But it is behind the dialog.

Is this change intended to polish these problems:
1. while in the dialog, in the new language the new misspelled word is selected in the message body, but it is not underlined yet (because another language was used for the underlining)
2. the subject is spellchecked and underlined for the new language only after clicking into it after leaving the dialog. Until then, it is underlined according to the old language

Also notice the Spell check dialog ignores the subject, but that is probably unfixable because the editor code does not see that field.
Comment on attachment 8909153 [details] [diff] [review]
1400646-check-spelling.patch (v2)

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

Now this is much better and does not provoke the HACK^3 reaction :) Thanks.

But I would also like to hear from Ian if this can be used in Seamonkey as is, or they need a different approach.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3728,5 @@
>   *
> + * ComposeChangeLanguage (called from the "Check Spelling" panel in SelectLanguage()):
> + * @param newLang New language to set.
> + * ChangeLanguage:
> + * @param event   Event of selecting an item in the spelling button menulist popup.

Why is ChangeLanguage documented here? Can you move it to the function? These comment block may be processed by automated tools (that is why it has this format) and none will understand this.

@@ +3733,2 @@
>   */
> +function ComposeChangeLanguage(newLang)

You didn't rename the argument to not touch the other lines. But this one isn't used much in the code so we can probably live with the churn of renaming it to aNewLang and change the 3-4 occurrences.
Attachment #8909153 - Flags: review?(acelists)
Attachment #8909153 - Flags: review+
Attachment #8909153 - Flags: feedback?(iann_bugzilla)
Fixed the comments and s/newLang/aLang/.

SM can port this, they just need to refactor their ChangeLanguage().
Attachment #8909153 - Attachment is obsolete: true
Attachment #8909153 - Flags: feedback?(iann_bugzilla)
Attachment #8909219 - Flags: review+
(In reply to :aceman from comment #4)
> Also notice the Spell check dialog ignores the subject, but that is probably
> unfixable because the editor code does not see that field.
No, it would be fixable, we just need to run the spell check on the two fields separately. Much pain for little gain. That was discussed up to cmt #57 in bug 368915 when I finally hijacked the bug for my own purposes after its summary had undergone various versions:

Summary: Allow spell checker's language select button ('Spell') in the subject line → Allow spell checking via the Spell button in the subject line
Summary: Allow spell checking via the Spell button in the subject line → Allow spell checking via the Spell button and ctrl+K in the subject line
Summary: Allow spell checking via the Spell button and ctrl+K in the subject line → Allow spell checking via the Spell button and Ctrl+Shift+P in the subject line
Allow spell checking via the Spell button and Ctrl+Shift+P in the subject line → Spell check command disabled in subject line (button, main menu "Check Spelling", keyboard shortcut Ctrl+Shift+P)
Spell check command disabled in subject line (button, main menu "Check Spelling", keyboard shortcut Ctrl+Shift+P) → Selection of spell check dictionaries disabled in subject line

See bug 368915 comment #172 for the hijack part. Anyway, something very useful came from that bug, the language indicator in the status bar, which can now also be used to change the language thanks to Thomas.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0cd3eec9f27f
update spellcheck in body and subject when language is changed from the 'check spelling' panel. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.