Last Comment Bug 291799 - Implement Spell as you Type for mailnews
: Implement Spell as you Type for mailnews
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Ian Neal
:
Mentors:
Depends on:
Blocks: inlineSpell 292217
  Show dependency treegraph
 
Reported: 2005-04-25 07:23 PDT by Ian Neal
Modified: 2006-01-06 12:15 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v0.1 (30.76 KB, patch)
2005-04-25 07:33 PDT, Ian Neal
no flags Details | Diff | Review
Patch v0.1 (no whitespace changes) (27.71 KB, patch)
2005-04-25 07:44 PDT, Ian Neal
no flags Details | Diff | Review
Revised Patch v0.1a (28.49 KB, patch)
2005-05-01 08:22 PDT, Ian Neal
no flags Details | Diff | Review
Updated patch v0.1b (28.70 KB, patch)
2005-05-02 02:51 PDT, Ian Neal
no flags Details | Diff | Review
Help patch v0.1bm (2.83 KB, patch)
2005-05-02 04:48 PDT, Ian Neal
neil: review+
Details | Diff | Review
Combined patch v0.1c (31.51 KB, patch)
2005-05-02 16:14 PDT, Ian Neal
neil: review+
Details | Diff | Review
Tweaked patch v0.1d (31.35 KB, patch)
2005-05-04 15:16 PDT, Ian Neal
neil: review+
Details | Diff | Review
Fully updated patch v0.1e (Checked in) (31.37 KB, patch)
2005-05-11 13:16 PDT, Ian Neal
iann_bugzilla: review+
mnyromyr: review+
mozilla: superreview+
asa: approval1.8b2+
Details | Diff | Review

Description Ian Neal 2005-04-25 07:23:35 PDT
Port across TB's spell as you type implementation (bug 278310 and bug 280826)
Comment 1 Ian Neal 2005-04-25 07:33:54 PDT
Created attachment 181772 [details] [diff] [review]
Patch v0.1

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)
Comment 2 Ian Neal 2005-04-25 07:44:04 PDT
Created attachment 181774 [details] [diff] [review]
Patch v0.1 (no whitespace changes)

Just in case this helps with the review
Comment 3 Ian Neal 2005-05-01 08:22:55 PDT
Created attachment 182329 [details] [diff] [review]
Revised Patch v0.1a

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
Comment 4 neil@parkwaycc.co.uk 2005-05-01 14:22:31 PDT
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.
Comment 5 Ian Neal 2005-05-02 02:51:52 PDT
Created attachment 182384 [details] [diff] [review]
Updated patch v0.1b

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
Comment 6 Ian Neal 2005-05-02 04:48:22 PDT
Created attachment 182386 [details] [diff] [review]
Help patch v0.1bm

This patch:
* Adds entries to mail_help.xhtml for the new spellcheck option/pref
Comment 7 neil@parkwaycc.co.uk 2005-05-02 12:25:40 PDT
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 8 neil@parkwaycc.co.uk 2005-05-02 12:27:07 PDT
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.
Comment 9 Ian Neal 2005-05-02 15:29:28 PDT
Neil, what is the best way of testing if inline spell checking is available?
Is disabling best done using command=?
Comment 10 Ian Neal 2005-05-02 16:14:47 PDT
Created attachment 182438 [details] [diff] [review]
Combined patch v0.1c

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
Comment 11 neil@parkwaycc.co.uk 2005-05-04 06:13:43 PDT
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)
Comment 12 Ian Neal 2005-05-04 15:16:13 PDT
Created attachment 182620 [details] [diff] [review]
Tweaked patch v0.1d

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=
Comment 13 Karsten Düsterloh 2005-05-11 02:09:38 PDT
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.)
Comment 14 Ian Neal 2005-05-11 13:16:43 PDT
Created attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

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
Comment 15 Ian Neal 2005-05-11 13:34:25 PDT
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Carried forward Neil's r=
Requesting r= from Mnyromyr
Comment 16 Karsten Düsterloh 2005-05-11 16:35:23 PDT
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.
Comment 17 Ian Neal 2005-05-11 16:55:33 PDT
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

Requesting sr=
Backend issue spun off into bug 293835
Comment 18 Ian Neal 2005-05-12 13:29:06 PDT
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
Comment 19 Asa Dotzler [:asa] 2005-05-13 11:15:13 PDT
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

a=asa
Comment 20 Ian Neal 2005-05-13 15:06:12 PDT
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
Comment 21 Sven Grull 2005-05-15 02:34:21 PDT
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.
Comment 22 Ian Neal 2005-05-15 06:57:29 PDT
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.
Comment 23 Sven Grull 2005-05-15 07:15:15 PDT
Added bug 294251
Comment 24 Stephen Donner [:stephend] - PTO; back on 5/28 2006-01-06 12:15:17 PST
Verified FIXED using build 2006-01-06-09 of trunk SeaMonkey under Windows XP.

Note You need to log in before you can comment on or make changes to this bug.