Closed Bug 291799 Opened 19 years ago Closed 19 years ago

Implement Spell as you Type for mailnews

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

Details

Attachments

(1 file, 7 obsolete files)

Port across TB's spell as you type implementation (bug 278310 and bug 280826)
Attached patch Patch v0.1 (obsolete) — Splinter Review
This patch:
* Inline Spell Check Front End for compose (TB bug 278310)
* Add mail recipients to the spell as you type ignore list (TB bug 280826)
Assignee: mail → bugzilla
Status: NEW → ASSIGNED
Attachment #181772 - Flags: review?(mnyromyr)
Just in case this helps with the review
Attachment #181772 - Flags: review?(mnyromyr)
Attachment #181774 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 292217
Attachment #181774 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Revised Patch v0.1a (obsolete) — Splinter Review
Changes since v0.1 (mostly suggested by Neil)
* Unbitrotted patch
* Removed non-related whitespace changes
* Included diff for mailnews.js
* menuseparator no longer removed between undo and cut
* Changed spellCheckInline.label
* Changed initEditor function to pass editor through
* Rewrote addRecipientsToIgnoreList function to use match and concat
Attachment #181772 - Attachment is obsolete: true
Attachment #181774 - Attachment is obsolete: true
Attachment #182329 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 182329 [details] [diff] [review]
Revised Patch v0.1a

>-      document.getElementById("spellCheckBeforeSend").hidden =
>-        !("@mozilla.org/spellchecker;1" in Components.classes);
>+      var hideSpellCheck = !("@mozilla.org/spellchecker;1" in Components.classes);
>+      document.getElementById("spellCheckBeforeSend").hidden = hideSpellCheck;
>+      document.getElementById("inlineSpellCheck").hidden = hideSpellCheck;
The first problem here is that the inline spellchecker code throws an exception
if the spell checker components couldn't be created. The second problem is that
you don't disable the toggle if the spell checker does not exist. So, you could
fix it, or you could require it, especially as it defaults to on, which would
mean not hiding either pref.

>-/**
>- * global variable inherited from MsgComposeCommands.js
>- *
>- 
>- var sPrefs;
>- var gPromptService;
>- var gMsgCompose;
>- 
>- */
This comment looks useful, might as well leave it in.

>+<script type="application/x-javascript" src="chrome://editor/content/editorInlineSpellCheck.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/accountUtils.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/widgetglue.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/mail-offline.js"/>
> <script type="application/x-javascript" src="chrome://editor/content/editor.js"/>
Pedantic neatness nit: keep editor scripts together

> <popup id="sidebarPopup"/>
>-<popup id="msgComposeContext" onpopupshowing="updateEditItems();">
Hmm... I forgot to leave a blank line here, would you mind?

>+          <menuitem label="&enableInlineSpellChecker.label;" id="menu_inlineSpellCheck"
>+                    accesskey="&enableInlineSpellChecker.accesskey;" checked="false"
>+                    type="checkbox" oncommand="ToggleInlineSpellChecker(event.target)"/>
Don't need checked="false" it's the default.

>+    for (name in names.value)
IIRC names.value is really an array, so maybe a for (var i = 0; i <
names.value.length; i++) loop would be more appropriate.

>+    if (InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell)
>+      InlineSpellChecker.checkDocument(window.content.document);
Hmm... is this is to work around bug 292520 or is it useful? I suppose it's
useful.

>+  InlineSpellChecker.checkDocument(window.content.document);
This one is definitely to work around bug 292520, here I'm less convinced that
it has any other use.
Attachment #182329 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Updated patch v0.1b (obsolete) — Splinter Review
Changes since v0.1a (as per review)
* Added try/catch so spellcheck prefs are hidden when they need to be
* Left useful comment in addressingWidgetOverlay.js
* Moved loading of editor scripts next to each other in messengercompose.xul
* Added blank line between popups in messengercompose.xul
* Removed unneeded checked="false" from inline spellcheck option in compose
window
* Added hiding of spellcheck options in compose window
* Changed from (name in names.value) to (var i = 0; i < names.value.length;
i++) in addRecipientsToIgnoreList function
* Added comment about bug 292520 to InitEditor function
Attachment #182329 - Attachment is obsolete: true
Attachment #182384 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Help patch v0.1bm (obsolete) — Splinter Review
This patch:
* Adds entries to mail_help.xhtml for the new spellcheck option/pref
Attachment #182386 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 182384 [details] [diff] [review]
Updated patch v0.1b

Sorry for being unclear. Since you're defaulting the preference to on you're
assuming that spellchecking will be available, so don't bother hiding anything.
Comment on attachment 182384 [details] [diff] [review]
Updated patch v0.1b

InlineSpellChecker.Init() will throw an exception if inline spell checking
isn't available. So you could still disable the menuitem in that case.
Attachment #182386 - Flags: review?(neil.parkwaycc.co.uk) → review+
Neil, what is the best way of testing if inline spell checking is available?
Is disabling best done using command=?
Attachment #182384 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Combined patch v0.1c (obsolete) — Splinter Review
Changes since v0.1b:
* Added help patch in
* Removed hiding of spellcheck menuitems in options
* Added disabling of inline spellcheck menuitem if InlineSpellChecker.Init
throws
Attachment #182384 - Attachment is obsolete: true
Attachment #182386 - Attachment is obsolete: true
Attachment #182438 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 182438 [details] [diff] [review]
Combined patch v0.1c

I still don't like having both the spelling items and the regular context menu
items showing at once. Not very contextual :-P And I'd like to ignore email
address from spellcheck as well as names, but that would need a smarter word
break algorithm :-/

>+      var hideSpellCheck = true;
>+      try {
>+        hideSpellCheck = !("@mozilla.org/spellchecker;1" in Components.classes);
>+      }
>+      catch(e) { }
Don't need this try/catch (probably caused by a misunderstanding).

>+    target.setAttribute('checked', InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell);
You shouldn't need this, menuitems know how to toggle themselves.

>+<!ENTITY spellCheckNoSuggestions.label "No Suggestions Found">
Should have ()s around the label, e.g. navigator.properties contains
nothingAvailabel=(Nothing Available)
Attachment #182438 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch Tweaked patch v0.1d (obsolete) — Splinter Review
Changes since v0.1c (as per review)
* Removed unneeded try/catch
* Added brackets to label
* Removed manual setting of check as it is done automatically

I still think the regular items have to appear in the context menu when there
are spelling items in the context menu otherwise that would cause problems if
you want to paste to replace the misspelt word. (workaround would to be either
ignore or delete misspelt word before pasting)

Requesting confirming r=
Attachment #182438 - Attachment is obsolete: true
Attachment #182620 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #182620 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #182620 - Flags: review?(mnyromyr)
Comment on attachment 182620 [details] [diff] [review]
Tweaked patch v0.1d

>@@ -486,16 +495,20 @@ function awReturnHit(inputElement)
>       subjectField.focus();
>     }
>   }
>   else
>   {
>     nextInput.select();
>     awSetFocus(row+1, nextInput);
>   }
>+
>+  // be sure to add the user add recipient to our ignore list

???

>@@ -720,26 +733,30 @@ function _awSetFocus()
...
>+  // be sure to add the user add recipient to our ignore list

And here again.


If I enter an email address into the addressing widget and it gets
autocompleted (eg. |gnumpf| gets completed to |gnumpf@gnorg.grr|), the inline
spellchecker only excludes the incompleted gnumpf, but not the added gnorg.grr.
And as soon as I touch a used part of the addressing widget again, even gnumpf
is marked as incorrect. But of a pasted gnumpf@gnorg.grr, only the completion
gets marked as incorrect...
Is this supposed behaviour?


>Index: mailnews/compose/resources/content/messengercompose.xul
>===================================================================

Context menu with both spell checking items and usual stuff is fine by me. I'd
be very puzzled if the latter would be missing.

>+function openEditorContextMenu()
>+{
>+  // if we have a mispelled word, do one thing, otherwise show the usual context menu

Not a very useful comment. Which "one thing" is done in this case?

>Index: extensions/help/resources/locale/en-US/mail_help.xhtml
>===================================================================
...
>+  <li><strong>Spell As You Type:</strong> Choose this option to have the
>+    spelling of the message text checked as you type.</li>

BTW: Shouldn't this (and in the other DTDs as well and in this bug's title) be
"Spellcheck as you type"? We don't actually /spell/ but spell*check*? 
(But that may be me not being a native English speaker.)
Changes since v0.1d:
* All comment changes made
* Changed "Spell As You Type" to "Spellcheck As You Type"

Note:
Only names not email address is added to the spell checker ignore list
Attachment #182620 - Attachment is obsolete: true
Attachment #183311 - Flags: review+
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Carried forward Neil's r=
Requesting r= from Mnyromyr
Attachment #183311 - Flags: review?(mnyromyr)
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

The strange behaviour of the inline spell checking code for pasted email
addresses seems to be a backend issue, so we should file a new bug on that.
Attachment #183311 - Flags: review?(mnyromyr) → review+
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Requesting sr=
Backend issue spun off into bug 293835
Attachment #183311 - Flags: superreview?(bienvenu)
Attachment #182620 - Flags: review?(mnyromyr)
Attachment #183311 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Requesting a= for a suite only patch for something already tested and
implemented in TB
Attachment #183311 - Flags: approval1.8b2?
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

a=asa
Attachment #183311 - Flags: approval1.8b2? → approval1.8b2+
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Checking in mailnews/mailnews.js;
new revision: 3.243; previous revision: 3.242
mailnews/compose/prefs/resources/content/pref-composing_messages.xul;
pref-composing_messages.xul
new revision: 1.46; previous revision: 1.45
mailnews/compose/prefs/resources/locale/en-US/pref-composing_messages.dtd;
pref-composing_messages.dtd
new revision: 1.23; previous revision: 1.22
mailnews/compose/resources/content/addressingWidgetOverlay.js;
addressingWidgetOverlay.js
new revision: 1.90; previous revision: 1.89
mailnews/compose/resources/content/messengercompose.xul;
new revision: 1.281; previous revision: 1.280
mailnews/compose/resources/content/MsgComposeCommands.js;
MsgComposeCommands.js
new revision: 1.364; previous revision: 1.363
mailnews/compose/resources/locale/en-US/messengercompose.dtd;
new revision: 1.89; previous revision: 1.88
extensions/help/resources/locale/en-US/mail_help.xhtml;
new revision: 1.60; previous revision: 1.59
done
Attachment #183311 - Attachment description: Fully updated patch v0.1e → Fully updated patch v0.1e (Checked in)
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: inlineSpell
Great work. But it seems that persdict.dat is not updated when using the right
click popup and "Add To Dictionary". 
Adding a new word to the personal dictionary removes the dotted line and the
addition seems to be stored in memory as I do not have to readd it as long I do
not close Mozilla. So it looks like that persdict.dat is not written to disc
when closing Mozilla.
Adding a new word via the normal spell check frontend still works and this bug
does not appear with Thunderbird.
If I should file a new bug for this problem please let me know.
Yes, log a new bug on it and cc me and neil.parkwaycc.co.uk into it - workaround
is to open and then close a "Check Spelling" window.

I've looked at the front end code and, at the moment, cannot see anything it is
doing that is different to TB. As far as I can see mPersonalDictionary's
EndSession() is not being called and it should be when mozSpellChecker is deleted.
Added bug 294251
Verified FIXED using build 2006-01-06-09 of trunk SeaMonkey under Windows XP.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: