The default bug view has changed. See this FAQ.

Implement Spell as you Type for mailnews

VERIFIED FIXED

Status

SeaMonkey
MailNews: Message Display
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

12 years ago
Port across TB's spell as you type implementation (bug 278310 and bug 280826)
(Assignee)

Comment 1

12 years ago
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)
Assignee: mail → bugzilla
Status: NEW → ASSIGNED
Attachment #181772 - Flags: review?(mnyromyr)
(Assignee)

Comment 2

12 years ago
Created attachment 181774 [details] [diff] [review]
Patch v0.1 (no whitespace changes)

Just in case this helps with the review
(Assignee)

Updated

12 years ago
Attachment #181772 - Flags: review?(mnyromyr)
(Assignee)

Updated

12 years ago
Attachment #181774 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

12 years ago
Blocks: 292217
(Assignee)

Updated

12 years ago
Attachment #181774 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 3

12 years ago
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
(Assignee)

Updated

12 years ago
Attachment #181772 - Attachment is obsolete: true
Attachment #181774 - Attachment is obsolete: true
Attachment #182329 - Flags: review?(neil.parkwaycc.co.uk)

Comment 4

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #182329 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 5

12 years ago
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
Attachment #182329 - Attachment is obsolete: true
Attachment #182384 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 6

12 years ago
Created attachment 182386 [details] [diff] [review]
Help patch v0.1bm

This patch:
* Adds entries to mail_help.xhtml for the new spellcheck option/pref
Attachment #182386 - Flags: review?(neil.parkwaycc.co.uk)

Comment 7

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

12 years ago
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.

Updated

12 years ago
Attachment #182386 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 9

12 years ago
Neil, what is the best way of testing if inline spell checking is available?
Is disabling best done using command=?
(Assignee)

Updated

12 years ago
Attachment #182384 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 10

12 years ago
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
Attachment #182384 - Attachment is obsolete: true
Attachment #182386 - Attachment is obsolete: true
Attachment #182438 - Flags: review?(neil.parkwaycc.co.uk)

Comment 11

12 years ago
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+
(Assignee)

Comment 12

12 years ago
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=
Attachment #182438 - Attachment is obsolete: true
Attachment #182620 - Flags: review?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #182620 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Updated

12 years ago
Attachment #182620 - Flags: review?(mnyromyr)

Comment 13

12 years ago
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.)
(Assignee)

Comment 14

12 years ago
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
(Assignee)

Updated

12 years ago
Attachment #182620 - Attachment is obsolete: true
Attachment #183311 - Flags: review+
(Assignee)

Comment 15

12 years ago
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 16

12 years ago
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+
(Assignee)

Comment 17

12 years ago
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)
(Assignee)

Updated

12 years ago
Attachment #182620 - Flags: review?(mnyromyr)

Updated

12 years ago
Attachment #183311 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 18

12 years ago
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 19

12 years ago
Comment on attachment 183311 [details] [diff] [review]
Fully updated patch v0.1e (Checked in)

a=asa
Attachment #183311 - Flags: approval1.8b2? → approval1.8b2+
(Assignee)

Comment 20

12 years ago
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)
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 58612

Comment 21

12 years ago
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.
(Assignee)

Comment 22

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

12 years ago
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.