Inline Spell Checker Back End

RESOLVED FIXED in Thunderbird1.1

Status

RESOLVED FIXED
14 years ago
4 years ago

People

(Reporter: mscott, Assigned: mscott)

Tracking

Trunk
Thunderbird1.1
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 13 obsolete attachments)

10.87 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
41.39 KB, patch
mscott
: review+
mscott
: superreview+
Details | Diff | Splinter Review
51.20 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
The changes to layout to support inline spell checking. Broken out from Bug #58612
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird1.1
(Assignee)

Comment 1

14 years ago
Created attachment 171214 [details] [diff] [review]
current draft of editor changes (still a work in progress)

Some of this came from Neil's original patch in 58612.
(Assignee)

Comment 2

14 years ago
Created attachment 171215 [details] [diff] [review]
extensions\spellcheck patch
(Assignee)

Comment 3

14 years ago
Created attachment 171216 [details] [diff] [review]
layout changes

This part is pretty much unchanged from Neil's original patch with the
exception of changes I made to nsTextFrame.cpp

I also re-merged his layout changes back to the trunk where files have been
moved and changed since mozilla 1.6.
(Assignee)

Comment 4

14 years ago
Created attachment 171310 [details] [diff] [review]
more layout changes

adjust the underline color for misspelled words to be a red dotted line
Attachment #171216 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
Created attachment 171311 [details] [diff] [review]
more spell checker changes

1) re-write the inline spell checker to work on end of word delimiters instead
of on every character you type. 
2) When you backspace back to a mispelled word, clear out the underline. Add
the underline back in after a word delimter is added.
3) line breaks should cause us to spell check the last word before the br
element is added even if there isn't a word delimter.
4) add argument bullet proofing
5) use error macros and avoid some early returns
Attachment #171215 - Attachment is obsolete: true
(Assignee)

Comment 6

14 years ago
Created attachment 171312 [details] [diff] [review]
editor update

revamp the APIs and JS interface for accessing the inline spell checker.
Simplify some of the editor changes.
Attachment #171214 - Attachment is obsolete: true
(Assignee)

Comment 7

14 years ago
Note to self:

1) Use the I18N word breaker routine to figure out when we've reached the end of
the word instead of calling IsEndOfWordChar.

2) apostrophes don't work correctly (doonnnn't) because
nsTextServicesDocument::FindWordBounds treats ' as a end of word terminator
(Assignee)

Comment 8

14 years ago
Comment on attachment 171310 [details] [diff] [review]
more layout changes

David, are you the right person to review thes changes to nsFrame and
nsSelection or should I find someone else? Most of these changes were actually
made my Neil Deakin for some work he did for NVU, I'm just picking up that part
of his patch and using it for inline spell checking.
Attachment #171310 - Flags: superreview?(dbaron)
(Assignee)

Comment 9

14 years ago
Comment on attachment 171312 [details] [diff] [review]
editor update

Hi Daniel, these are the core editor changes required to support inline spell
checking. Most of this should look familiar as it is based on Neil's patch for
for NVU.
Attachment #171312 - Flags: review?(daniel)
Comment on attachment 171312 [details] [diff] [review]
editor update

Nothing major but one clarification (quotation marks) and a double-check
(leaks) needed.


>Index: composer/src/nsComposeTxtSrvFilter.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/composer/src/nsComposeTxtSrvFilter.cpp,v
>retrieving revision 1.9
>diff -u -w -r1.9 nsComposeTxtSrvFilter.cpp
>--- composer/src/nsComposeTxtSrvFilter.cpp	17 Jun 2004 00:12:35 -0000	1.9
>+++ composer/src/nsComposeTxtSrvFilter.cpp	15 Jan 2005 01:38:58 -0000
>@@ -49,6 +49,7 @@
>   mPreAtom         = do_GetAtom("pre");
>   mSpanAtom        = do_GetAtom("span");
>   mMozQuoteAtom    = do_GetAtom("_moz_quote");
>+  mClassAtom       = do_GetAtom("class");
>   mTypeAtom        = do_GetAtom("type");
>   mScriptAtom      = do_GetAtom("script");
>   mTextAreaAtom    = do_GetAtom("textarea");
>@@ -82,6 +83,11 @@
>         if (NS_SUCCEEDED(content->GetAttr(kNameSpaceID_None, mMozQuoteAtom, mozQuote))) {
>           *_retval = mozQuote.LowerCaseEqualsLiteral("true");            
>         }
>+
>+        nsAutoString className;
>+        if (NS_SUCCEEDED(content->GetAttr(kNameSpaceID_None, mClassAtom, className))) {
>+          *_retval = className.EqualsIgnoreCase("moz-signature");

You're sure about that ? A class name _is_ case sensitive. I understand you
consider that
"moz-signature" value can only come from inside Mozilla but is it really worth
the time
spent to ignore case ?

>Index: composer/src/nsEditorSpellCheck.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/composer/src/nsEditorSpellCheck.cpp,v
>retrieving revision 1.17
>diff -u -w -r1.17 nsEditorSpellCheck.cpp
>--- composer/src/nsEditorSpellCheck.cpp	24 Nov 2004 22:48:35 -0000	1.17
>+++ composer/src/nsEditorSpellCheck.cpp	15 Jan 2005 01:38:58 -0000
>@@ -23,6 +23,7 @@
>  *      Kin Blas <kin@netscape.com>
>  *      Akkana Peck <akkana@netscape.com>
>  *      Charley Manske <cmanske@netscape.com>
>+ *      Neil Deakin <neil@mozdevgroup.com> (realtime spellchecking)

We usually don't mention the reason why a contributor is added.

>Index: libeditor/base/nsEditor.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/base/nsEditor.cpp,v
>retrieving revision 1.434
>diff -u -w -r1.434 nsEditor.cpp
>--- libeditor/base/nsEditor.cpp	24 Nov 2004 22:48:35 -0000	1.434
>+++ libeditor/base/nsEditor.cpp	15 Jan 2005 01:38:59 -0000
>@@ -101,6 +101,7 @@
> #include "nsEditor.h"
> #include "nsEditorUtils.h"
> #include "nsISelectionDisplay.h"
>+#include "nsIInlineSpellChecker.h"
> #include "nsINameSpaceManager.h"
> #include "nsIHTMLDocument.h"
> 
>@@ -215,6 +216,9 @@
>   delete mEditorObservers;   // no need to release observers; we didn't addref them
>   mEditorObservers = 0;
>   
>+  if (mInlineSpellChecker)
>+    mInlineSpellChecker->Destroy();
>+
>   if (mActionListeners)
>   {
>     PRInt32 i;
>@@ -1141,6 +1145,23 @@
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP nsEditor::GetInlineSpellChecker(nsIInlineSpellChecker ** aInlineSpellChecker)
>+{
>+  NS_ENSURE_ARG_POINTER(aInlineSpellChecker);
>+  nsresult rv;
>+
>+  if (!mInlineSpellChecker) {
>+    mInlineSpellChecker = do_CreateInstance(MOZ_INLINESPELLCHECKER_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = mInlineSpellChecker->Init(this);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  NS_IF_ADDREF(*aInlineSpellChecker = mInlineSpellChecker);
>+  

Remove spaces on blank line.

>+  return NS_OK;
>+}
> 
> #ifdef XP_MAC
> #pragma mark -
>@@ -5203,6 +5224,27 @@
> {
>   return RemoveAttribute(aElement, aAttribute);
> }
>+
>+nsresult
>+nsEditor::HandleRealTimeSpellCheck(PRInt32 action,
>+                                   nsISelection *aSelection,
>+                                   nsIDOMNode *previousSelectedNode,
>+                                   PRInt32 previousSelectedOffset,
>+                                   nsIDOMNode *aStartNode,
>+                                   PRInt32 aStartOffset,
>+                                   nsIDOMNode *aEndNode,
>+                                   PRInt32 aEndOffset)
>+{
>+  return mInlineSpellChecker ? mInlineSpellChecker->SpellCheckAfterEditorChange(action,
>+                                                       aSelection,
>+                                                       previousSelectedNode,
>+                                                       previousSelectedOffset,
>+                                                       aStartNode,
>+                                                       aStartOffset,
>+                                                       aEndNode,
>+                                                       aEndOffset) : NS_OK;

Hard to read because of the line's length...

>Index: libeditor/base/nsEditor.h
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/base/nsEditor.h,v
>retrieving revision 1.158
>diff -u -w -r1.158 nsEditor.h
>--- libeditor/base/nsEditor.h	12 Sep 2004 05:00:57 -0000	1.158
>+++ libeditor/base/nsEditor.h	15 Jan 2005 01:38:59 -0000
>@@ -59,6 +59,8 @@
> #include "nsIDTD.h"
> #include "nsIDOMElement.h"
> #include "nsSelectionState.h"
>+#include "nsIEditorSpellCheck.h"
>+#include "nsIInlineSpellChecker.h"
> 
> class nsIEditActionListener;
> class nsIDocumentStateListener;
>@@ -547,6 +549,15 @@
> 
>   PRBool GetShouldTxnSetSelection();
> 
>+  nsresult HandleRealTimeSpellCheck(PRInt32 action,
>+                                    nsISelection *aSelection,
>+                                    nsIDOMNode *previousSelectedNode,
>+                                    PRInt32 previousSelectedOffset,
>+                                    nsIDOMNode *aStartNode,
>+                                    PRInt32 aStartOffset,
>+                                    nsIDOMNode *aEndNode,
>+                                    PRInt32 aEndOffset);
>+
> public:
>   // Argh!  These transaction names are used by PlaceholderTxn and
>   // nsPlaintextEditor.  They should be localized to those classes.
>@@ -563,6 +574,7 @@
>   nsWeakPtr       mSelConWeak;   // weak reference to the nsISelectionController
>   nsIViewManager *mViewManager;
>   PRInt32         mUpdateCount;
>+  nsCOMPtr<nsIInlineSpellChecker> mInlineSpellChecker;  // used for real-time spellchecking
>   nsCOMPtr<nsITransactionManager> mTxnMgr;
>   nsWeakPtr         mPlaceHolderTxn;     // weak reference to placeholder for begin/end batch purposes
>   nsIAtom          *mPlaceHolderName;    // name of placeholder transaction
>Index: libeditor/html/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/html/Makefile.in,v
>retrieving revision 1.17
>diff -u -w -r1.17 Makefile.in
>--- libeditor/html/Makefile.in	15 Dec 2004 04:06:57 -0000	1.17
>+++ libeditor/html/Makefile.in	15 Jan 2005 01:38:59 -0000
>@@ -54,6 +54,7 @@
> 		  unicharutil \
> 		  content \
> 		  txmgr \
>+		  txtsvc \
> 		  htmlparser \
> 		  necko \
> 		  pref \
>Index: libeditor/html/nsHTMLEditRules.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v
>retrieving revision 1.317
>diff -u -w -r1.317 nsHTMLEditRules.cpp
>--- libeditor/html/nsHTMLEditRules.cpp	12 Jan 2005 19:11:47 -0000	1.317
>+++ libeditor/html/nsHTMLEditRules.cpp	15 Jan 2005 01:39:01 -0000
>@@ -22,6 +22,7 @@
>  * Contributor(s):
>  *   Pierre Phaneuf <pp@ludusdesign.com>
>  *   Daniel Glazman <glazman@netscape.com>
>+ *   Neil Deakin <neil@mozdevgroup.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),
>@@ -425,13 +426,17 @@
>   nsresult res = mHTMLEditor->GetSelection(getter_AddRefs(selection));
>   if (NS_FAILED(res)) return res;
>   
>+  nsCOMPtr<nsIDOMNode> rangeStartParent, rangeEndParent;
>+  PRInt32 rangeStartOffset = 0, rangeEndOffset = 0;
>+  

Spaces on blank line.

>   // do we have a real range to act on?
>   PRBool bDamagedRange = PR_FALSE;  
>   if (mDocChangeRange)
>   {  
>-    nsCOMPtr<nsIDOMNode> rangeStartParent, rangeEndParent;
>     mDocChangeRange->GetStartContainer(getter_AddRefs(rangeStartParent));
>     mDocChangeRange->GetEndContainer(getter_AddRefs(rangeEndParent));
>+    mDocChangeRange->GetStartOffset(&rangeStartOffset);
>+    mDocChangeRange->GetEndOffset(&rangeEndOffset);
>     if (rangeStartParent && rangeEndParent) 
>       bDamagedRange = PR_TRUE; 
>   }
>@@ -540,6 +545,12 @@
>     }    
>   }
> 
>+  res = mHTMLEditor->HandleRealTimeSpellCheck(action,selection,
>+                                              mRangeItem.startNode,mRangeItem.startOffset,
>+                                              rangeStartParent,rangeStartOffset,
>+                                              rangeEndParent,rangeEndOffset);

Please add a whitespace after commas for readability.

>+  if (NS_FAILED(res)) return res;

Two lines preferred.

>+
>   // detect empty doc
>   res = CreateBogusNodeIfNeeded(selection);
>   
>Index: libeditor/html/nsHTMLEditor.h
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.h,v
>retrieving revision 1.218
>diff -u -w -r1.218 nsHTMLEditor.h
>--- libeditor/html/nsHTMLEditor.h	12 Jan 2005 19:11:47 -0000	1.218
>+++ libeditor/html/nsHTMLEditor.h	15 Jan 2005 01:39:01 -0000
>@@ -48,6 +48,7 @@
> #include "nsITableEditor.h"
> #include "nsIEditorMailSupport.h"
> #include "nsIEditorStyleSheets.h"
>+#include "nsITextServicesDocument.h"
> 
> #include "nsEditor.h"
> #include "nsIDOMElement.h"
>@@ -788,6 +789,9 @@
>   // an array for holding default style settings
>   nsVoidArray mDefaultStyles;
> 
>+   // for real-time spelling
>+   nsCOMPtr<nsITextServicesDocument> mTextServices;
>+
>   // Maintain a static parser service ...
>   static nsIParserService* sParserService;
> 
>Index: libeditor/text/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/text/Makefile.in,v
>retrieving revision 1.14
>diff -u -w -r1.14 Makefile.in
>--- libeditor/text/Makefile.in	15 Dec 2004 04:06:57 -0000	1.14
>+++ libeditor/text/Makefile.in	15 Jan 2005 01:39:01 -0000
>@@ -53,6 +53,7 @@
> 		  layout \
> 		  content \
> 		  txmgr \
>+		  txtsvc \
> 		  htmlparser \
> 		  necko \
> 		  pref \
>Index: libeditor/text/nsTextEditRules.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.cpp,v
>retrieving revision 1.190
>diff -u -w -r1.190 nsTextEditRules.cpp
>--- libeditor/text/nsTextEditRules.cpp	31 May 2004 12:15:21 -0000	1.190
>+++ libeditor/text/nsTextEditRules.cpp	15 Jan 2005 01:39:02 -0000
>@@ -49,7 +49,9 @@
> #include "nsIDOMNodeList.h"
> #include "nsISelection.h"
> #include "nsISelectionPrivate.h"
>+#include "nsISelectionController.h"
> #include "nsIDOMRange.h"
>+#include "nsIDOMNSRange.h"
> #include "nsIDOMCharacterData.h"
> #include "nsIContent.h"
> #include "nsIContentIterator.h"
>@@ -58,6 +60,8 @@
> #include "nsIPrefBranch.h"
> #include "nsIPrefService.h"
> #include "nsUnicharUtils.h"
>+#include "nsIWordBreakerFactory.h"
>+#include "nsLWBrkCIID.h"
> 
> // for IBMBIDI
> #include "nsIPresShell.h"
>@@ -196,6 +200,14 @@
>   nsAutoLockRulesSniffing lockIt(this);
>   mDidExplicitlySetInterline = PR_FALSE;
>   
>+  // get the selection and cache the position before editing
>+  nsCOMPtr<nsISelection> selection;
>+  nsresult res = mEditor->GetSelection(getter_AddRefs(selection));
>+  if (NS_FAILED(res)) return res;

2 lines.

>+
>+  selection->GetAnchorNode(getter_AddRefs(mCachedSelectionNode));
>+  selection->GetAnchorOffset(&mCachedSelectionOffset);
>+  

Spaces on blank line;

>   if (!mActionNesting)
>   {
>     // let rules remember the top level action
>@@ -221,6 +233,11 @@
>     res = mEditor->GetSelection(getter_AddRefs(selection));
>     if (NS_FAILED(res)) return res;
>   
>+    res = mEditor->HandleRealTimeSpellCheck(action,selection,
>+                                            mCachedSelectionNode,mCachedSelectionOffset,
>+                                            nsnull,0,nsnull,0);

Whitespace after comma.

>+    if (NS_FAILED(res)) return res;

2 lines.

>+
>     // detect empty doc
>     res = CreateBogusNodeIfNeeded(selection);
>     if (NS_FAILED(res)) return res;
>Index: libeditor/text/nsTextEditRules.h
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.h,v
>retrieving revision 1.75
>diff -u -w -r1.75 nsTextEditRules.h
>--- libeditor/text/nsTextEditRules.h	18 Apr 2004 14:19:36 -0000	1.75
>+++ libeditor/text/nsTextEditRules.h	15 Jan 2005 01:39:02 -0000
>@@ -203,6 +203,8 @@
>   PRInt32              mPasswordIMEIndex;
>   nsCOMPtr<nsIDOMNode> mBogusNode;     // magic node acts as placeholder in empty doc
>   nsCOMPtr<nsIDOMNode> mBody;          // cached root node
>+  nsCOMPtr<nsIDOMNode> mCachedSelectionNode;    // cached selected node
>+  PRInt32              mCachedSelectionOffset;  // cached selected offset
>   PRUint32             mFlags;
>   PRUint32             mActionNesting;
>   PRPackedBool         mLockRulesSniffing;
>Index: txtsvc/public/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/editor/txtsvc/public/Makefile.in,v
>retrieving revision 1.12
>diff -u -w -r1.12 Makefile.in
>--- txtsvc/public/Makefile.in	18 Apr 2004 14:19:38 -0000	1.12
>+++ txtsvc/public/Makefile.in	15 Jan 2005 01:39:02 -0000
>@@ -54,6 +54,7 @@
> 
> XPIDLSRCS	= \
> 		nsITextServicesFilter.idl \
>+		nsIInlineSpellChecker.idl \
> 		$(NULL)
> 
> include $(topsrcdir)/config/rules.mk
>Index: txtsvc/public/nsIInlineSpellChecker.idl
>===================================================================
>RCS file: txtsvc/public/nsIInlineSpellChecker.idl
>diff -N txtsvc/public/nsIInlineSpellChecker.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ txtsvc/public/nsIInlineSpellChecker.idl	15 Jan 2005 01:39:02 -0000
>@@ -0,0 +1,77 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Real-time Spellchecking
>+ *
>+ * The Initial Developer of the Original Code is Neil Deakin.
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s): Neil Deakin (enndeakin@sympatico.ca)
>+ *                 Scott MacGregor (mscott@mozilla.org)

"Contributor(s):" should be alone on its line, contributors' list beginning
at next line with 2 chars indentation.

>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nsISupports.idl"
>+#include "domstubs.idl"
>+
>+interface nsISelection;
>+interface nsIEditor;
>+interface nsIEditorSpellCheck;
>+
>+[scriptable, uuid(f5d1ec9e-4d30-11d8-8053-da0cc7df1f20)]
>+
>+interface nsIInlineSpellChecker : nsISupports
>+{
>+  readonly attribute nsIEditorSpellCheck spellChecker;
>+
>+  [noscript] void init(in nsIEditor editor);

Worried me a bit because the inlinespellchecker keeps a ref to the editor
that itself keeps a ref to the inlinespellchecker, but the former being a weak
reference, there should be no leak. Please double-check.

>+  [noscript] void destroy(); 
>+
>+  attribute boolean enableRealTimeSpell;
>+
>+  void spellCheckAfterEditorChange(in PRInt32 action,
>+                                   in nsISelection aSelection,
>+                                   in nsIDOMNode previousSelectedNode,

Some parameters follow the aSomething rule, some don't. Comment valid
for all other IDLs in the patch.

>+                                   in PRInt32 previousSelectedOffset,
>+                                   in nsIDOMNode aStartNode,
>+                                   in PRInt32 aStartOffset,
>+                                   in nsIDOMNode aEndNode,
>+                                   in PRInt32 aEndOffset);
>+
>+  void spellCheckRange(in nsIDOMRange aSelection);
>+
>+  nsIDOMRange getMispelledWord(in nsIDOMNode aNode, in PRInt32 aOffset);
>+  void replaceWord(in nsIDOMNode aNode, in PRInt32 aOffset, in AString newword);
>+  void addWordToDictionary(in AString word);
>+  void ignoreWord(in AString word);
>+};
>+
>+%{C++
>+
>+#define MOZ_INLINESPELLCHECKER_CONTRACTID "@mozilla.org/spellchecker-inline;1"
>+
>+%}
>Index: txtsvc/public/nsITextServicesDocument.h
>===================================================================
>RCS file: /cvsroot/mozilla/editor/txtsvc/public/nsITextServicesDocument.h,v
>retrieving revision 1.13
>diff -u -w -r1.13 nsITextServicesDocument.h
>--- txtsvc/public/nsITextServicesDocument.h	18 Apr 2004 14:19:38 -0000	1.13
>+++ txtsvc/public/nsITextServicesDocument.h	15 Jan 2005 01:39:02 -0000
>@@ -266,6 +266,14 @@
>    */
> 
>   NS_IMETHOD SetDisplayStyle(TSDDisplayStyle aStyle) = 0;
>+
>+  /**
>+   * Returns the DOM range for a given offset and length
>+   * @param aOffset offset into string returned by GetCurrentTextBlock().
>+   * @param aLength number characters selected.
>+   * @param aDOMRange the DOM range that represents the offset and length
>+   */

If you start adding javadoc comments, please don't forget the @return line.

>+  NS_IMETHOD GetDOMRangeFor(PRInt32 aOffset, PRInt32 aLength, nsIDOMRange** aRange) = 0;
> };
> 
> #endif // nsITextServicesDocument_h__
>Index: txtsvc/src/nsTextServicesDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp,v
>retrieving revision 1.57
>diff -u -w -r1.57 nsTextServicesDocument.cpp
>--- txtsvc/src/nsTextServicesDocument.cpp	1 Nov 2004 18:50:10 -0000	1.57
>+++ txtsvc/src/nsTextServicesDocument.cpp	15 Jan 2005 01:39:03 -0000
>@@ -21,6 +21,7 @@
>  *
>  * Contributor(s):
>  *   Pierre Phaneuf <pp@ludusdesign.com>
>+ *   Neil Deakin <neil@mozdevgroup.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),
>@@ -321,8 +322,6 @@
> {
>   NS_ENSURE_ARG_POINTER(aDOMRange);
>   NS_ENSURE_TRUE(mDOMDocument, NS_ERROR_FAILURE);
>-  NS_ENSURE_TRUE(mEditor, NS_ERROR_FAILURE);
>-  NS_ENSURE_TRUE(mNotifier, NS_ERROR_FAILURE);
> 
>   LOCK_DOC(this);
> 
>@@ -548,10 +547,10 @@
>   // Now adjust the range so that it uses our new
>   // end points.
> 
>-  result = aRange->SetStart(rngStartNode, rngStartOffset);
>+  result = aRange->SetEnd(rngEndNode, rngEndOffset);
>   NS_ENSURE_SUCCESS(result, result);
> 
>-  return aRange->SetEnd(rngEndNode, rngEndOffset);
>+  return aRange->SetStart(rngStartNode, rngStartOffset);
> }
> 
> NS_IMETHODIMP
>@@ -2425,18 +2424,74 @@
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>+NS_IMETHODIMP
>+nsTextServicesDocument::GetDOMRangeFor(PRInt32 aOffset, PRInt32 aLength, nsIDOMRange** aRange)
>+{
>+  if (!mSelCon || aOffset < 0 || aLength < 0)
>+    return NS_ERROR_FAILURE;
>+
>+  nsIDOMNode *sNode = 0, *eNode = 0;
>+  PRInt32 i, sOffset = 0, eOffset = 0;
>+  OffsetEntry *entry;
>+
>+  // Find the start
>+  for (i = 0; !sNode && i < mOffsetTable.Count(); i++)
>+  {
>+    entry = (OffsetEntry *)mOffsetTable[i];
>+    if (entry->mIsValid)
>+    {
>+      if (entry->mIsInsertedText)
>+      {
>+        if (entry->mStrOffset == aOffset)
>+        {
>+          sNode   = entry->mNode;
>+          sOffset = entry->mNodeOffset + entry->mLength;
>+        }
>+      }
>+      else if (aOffset >= entry->mStrOffset && aOffset <= entry->mStrOffset + entry->mLength)
>+      {
>+        sNode   = entry->mNode;
>+        sOffset = entry->mNodeOffset + aOffset - entry->mStrOffset;
>+      }
>+    }
>+  }
>+
>+  if (!sNode)
>+    return NS_ERROR_FAILURE;
>+
>+  // Now find the end
>+  PRInt32 endOffset = aOffset + aLength;
>+
>+  for (i = mOffsetTable.Count() - 1; !eNode && i >= 0; i--)
>+  {
>+    entry = (OffsetEntry *)mOffsetTable[i];
>+    
>+    if (entry->mIsValid)
>+    {
>+      if (entry->mIsInsertedText)
>+      {
>+        if (entry->mStrOffset == eOffset)
>+        {
>+          eNode   = entry->mNode;
>+          eOffset = entry->mNodeOffset + entry->mLength;
>+        }
>+      }
>+      else if (endOffset >= entry->mStrOffset && endOffset <= entry->mStrOffset + entry->mLength)
>+      {
>+        eNode   = entry->mNode;
>+        eOffset = entry->mNodeOffset + endOffset - entry->mStrOffset;
>+      }
>+    }
>+  }
>+
>+  return CreateRange(sNode, sOffset, eNode, eOffset, aRange);
>+}
>+
> nsresult
> nsTextServicesDocument::InsertNode(nsIDOMNode *aNode,
>                                    nsIDOMNode *aParent,
>                                    PRInt32 aPosition)
> {
>-  //**** KDEBUG ****
>-  // printf("** InsertNode: 0x%.8x  0x%.8x  %d\n", aNode, aParent, aPosition);
>-  // fflush(stdout);
>-  //**** KDEBUG ****
>-
>-  NS_ASSERTION(0, "InsertNode called, offset tables might be out of sync."); 
>-
>   return NS_OK;
> }
> 
>@@ -4579,6 +4634,13 @@
>   return result;
> }
> 
>+// Spellchecker code has this. See bug 211343
>+#ifdef XP_MAC
>+#define IS_NBSP_CHAR(c) (((unsigned char)0xca)==(c))
>+#else
>+#define IS_NBSP_CHAR(c) (((unsigned char)0xa0)==(c))
>+#endif
>+
> nsresult
> nsTextServicesDocument::FindWordBounds(nsVoidArray *aOffsetTable,
>                                        nsString *aBlockStr,
>@@ -4628,6 +4690,21 @@
>                                  &beginWord, &endWord);
>   NS_ENSURE_SUCCESS(result, result);
> 
>+  PRBool hasBeginQuote = PR_FALSE;
>+  if (str[beginWord] == '\''){

Please add whitespace between ) and { or make a new line with the curly brace.

Why do you test only single quote ' and not " or even « and other
language-specific
quotation marks ?

>+    hasBeginQuote = PR_TRUE;
>+    beginWord++;
>+  }
>+  if (hasBeginQuote && str[endWord - 1] == '\'') endWord--;

2 lines please.

>+
>+  // Strip out the NBSPs at the ends
>+  while ((beginWord <= endWord) && (IS_NBSP_CHAR(str[beginWord]))) beginWord++;

2 lines please.

>+  if (str[endWord] == (unsigned char)0x20){

) {

>+    PRUint32 realEndWord = endWord - 1;
>+    while ((realEndWord > beginWord) && (IS_NBSP_CHAR(str[realEndWord]))) realEndWord--;

2 lines please.

>+    if (realEndWord < endWord - 1) endWord = realEndWord + 1;

2 lines please.

>+  }
>+
>   // Now that we have the string offsets for the beginning
>   // and end of the word, run through the offset table and
>   // convert them back into dom points.
>Index: txtsvc/src/nsTextServicesDocument.h
>===================================================================
>RCS file: /cvsroot/mozilla/editor/txtsvc/src/nsTextServicesDocument.h,v
>retrieving revision 1.19
>diff -u -w -r1.19 nsTextServicesDocument.h
>--- txtsvc/src/nsTextServicesDocument.h	10 Oct 2004 03:30:42 -0000	1.19
>+++ txtsvc/src/nsTextServicesDocument.h	15 Jan 2005 01:39:03 -0000
>@@ -159,6 +159,7 @@
>   NS_IMETHOD DeleteSelection();
>   NS_IMETHOD InsertText(const nsString *aText);
>   NS_IMETHOD SetDisplayStyle(TSDDisplayStyle aStyle);
>+  NS_IMETHOD GetDOMRangeFor(PRInt32 aOffset, PRInt32 aLength, nsIDOMRange** aRange);
> 
>   /* nsIEditActionListener method implementations. */
>   nsresult InsertNode(nsIDOMNode * aNode,
>Index: ui/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/editor/ui/jar.mn,v
>retrieving revision 1.14
>diff -u -w -r1.14 jar.mn
>--- ui/jar.mn	7 Jan 2005 01:31:43 -0000	1.14
>+++ ui/jar.mn	15 Jan 2005 01:39:03 -0000
>@@ -25,6 +25,7 @@
>       content/editor/pref-composer.xul                         (composer/content/pref-composer.xul) 
>       content/editor/pref-publish.xul                          (composer/content/pref-publish.xul) 
>       content/editor/editorSmileyOverlay.xul                   (composer/content/editorSmileyOverlay.xul) 
>+      content/editor/editorInlineSpellCheck.js                 (composer/content/editorInlineSpellCheck.js) 
>       content/editor/editorPrefsOverlay.xul                    (composer/content/editorPrefsOverlay.xul) 
>       content/editor/editorNavigatorOverlay.xul                (composer/content/editorNavigatorOverlay.xul) 
>       content/editor/editorMailOverlay.xul                     (composer/content/editorMailOverlay.xul) 
>Index: ui/composer/content/editorInlineSpellCheck.js
>===================================================================
>RCS file: ui/composer/content/editorInlineSpellCheck.js
>diff -N ui/composer/content/editorInlineSpellCheck.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ ui/composer/content/editorInlineSpellCheck.js	15 Jan 2005 01:39:03 -0000
>@@ -0,0 +1,174 @@
>+/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Netscape Public License
>+ * Version 1.1 (the "License"); you may not use this file except in
>+ * compliance with the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/NPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Inline Spellchecker
>+ *
>+ * The Initial Developer of the Original Code is Mozdev Group, Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s): Neil Deakin (neil@mozdevgroup.com)
>+ *                 Scott MacGregor (mscott@mozilla.org)

See above.

>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or 
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the NPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the NPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+const kSpellMaxNumSuggestions = 7; // Maximum number of suggested words to fill into the context menu. Most
>+                                   // applications (like Open Office) set this value to 15.

Could/Should this be a user pref ?

>+
>+const kSpellNoMispelling = -1;
>+const kSpellNoSuggestionsFound = 0;
>+
>+var InlineSpellChecker =
>+{
>+  editor : null,
>+  inlineSpellChecker: null,
>+
>+  Init : function (editor, enable)
>+  {
>+    this.editor = editor;
>+    this.inlineSpellChecker = editor.inlineSpellChecker;
>+    this.inlineSpellChecker.enableRealTimeSpell = enable;
>+  },
>+
>+  checkDocument : function(doc)
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return null;

2 lines.

>+
>+    var range = doc.createRange();
>+    range.selectNodeContents(doc.body);
>+    this.inlineSpellChecker.spellCheckRange(range);
>+  },
>+
>+  getMispelledWord : function()
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return null;

2 lines.

>+
>+    var selection = this.editor.selection;
>+    return this.inlineSpellChecker.getMispelledWord(selection.anchorNode,selection.anchorOffset);

Whitespace after comma.

>+  },
>+
>+  updateMenu : function(menuid, separatorid)
>+  {
>+    var word = this.getMispelledWord();
>+
>+    var suggestionsMenu = document.getElementById(menuid);
>+    var suggestionsMenuSeparator = document.getElementById(separatorid);
>+    if (word){

) {

>+      suggestionsMenu.removeAttribute("hidden");
>+      suggestionsMenuSeparator.removeAttribute("hidden");
>+    } else {

Else on new line.

>+      suggestionsMenu.setAttribute("hidden","true");
>+      suggestionsMenuSeparator.setAttribute("hidden","true");

Whitespace after comma.

>+    }
>+  },
>+
>+  // returns kSpellNoMispelling if there is no mispelling of the word
>+  // For mispelled words, returns kSpellNoSuggestionsFound when there are no suggestions otherwise the
>+  // number of suggestions is returned.
>+  updateSuggestionsMenu : function (menupopup, firstNonWordMenuItem, word)
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return kSpellNoMispelling;

2 lines please.

>+
>+    var child = menupopup.firstChild;
>+    while (child != firstNonWordMenuItem) {
>+      var next = child.nextSibling;
>+      menupopup.removeChild(child);
>+      child = next;
>+    }
>+
>+    if (!word){

) {

>+      word = this.getMispelledWord();
>+      if (!word) return kSpellNoMispelling;

2 lines please.

>+    }
>+
>+    var spellChecker = this.inlineSpellChecker.spellChecker;
>+    if (!spellChecker) return kSpellNoMispelling;

2 lines please.

>+
>+    var found = false;
>+    var numSuggestedWords = 0; 
>+
>+    var isIncorrect = spellChecker.CheckCurrentWord(word.toString());
>+    if (isIncorrect){

) {

>+      do {
>+        var suggestion = spellChecker.GetSuggestedWord();
>+        if (!suggestion) break;

2 lines please.

>+
>+        found = true;

This 'found' is apparently unused since you now rely only on the return value
of |GetSuggestedWord()|.

>+
>+        var item = document.createElement("menuitem");
>+        item.setAttribute("label",suggestion);
>+        item.setAttribute("value",suggestion);

Whitespace after comma.

>+        menupopup.insertBefore(item, firstNonWordMenuItem);
>+        numSuggestedWords++;
>+      } while(numSuggestedWords < kSpellMaxNumSuggestions);

Whitespace before ( please.

>+    }
>+    else 
>+      numSuggestedWords = kSpellNoMispelling;
>+
>+    return numSuggestedWords;
>+  },
>+
>+  selectSuggestion : function (newword, node, offset)
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return null;

2 lines please.

>+
>+    if (!node){

) {

>+      var selection = this.editor.selection;
>+      node = selection.anchorNode;
>+      offset = selection.anchorOffset;
>+    }
>+
>+    this.inlineSpellChecker.replaceWord(node,offset,newword);
>+  },
>+
>+  addToDictionary : function (node, offset)
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return null;

2 lines please.

>+
>+    if (!node){

) {

>+      var selection = this.editor.selection;
>+      node = selection.anchorNode;
>+      offset = selection.anchorOffset;
>+    }
>+
>+    var word = this.inlineSpellChecker.getMispelledWord(node,offset);

Whitespace after comma.

>+    if (word) this.inlineSpellChecker.addWordToDictionary(word);

2 lines please.

>+  },
>+
>+  ignoreWord : function (node, offset)
>+  {
>+    if (!this.inlineSpellChecker || !this.inlineSpellChecker.enableRealTimeSpell) return null;

2 lines please.

>+
>+    if (!node){

) {

>+      var selection = this.editor.selection;
>+      node = selection.anchorNode;
>+      offset = selection.anchorOffset;
>+    }
>+
>+    var word = this.inlineSpellChecker.getMispelledWord(node,offset);

Whitespace after comma.

>+    if (word) this.inlineSpellChecker.ignoreWord(word);

2 lines please.

>+  }
>+}
Attachment #171312 - Flags: review?(daniel) → review-
(Assignee)

Comment 11

14 years ago
Created attachment 171670 [details] [diff] [review]
updated patch based on review comments

new patch to address daniel's review comments. (Thanks for that quick turn
around!)

Daniel to answer two of your questions:

1) I confirmed there wasn't a cylce leak
2) I may end up making the # of words in the menu a configureable pref down the
line.
Attachment #171312 - Attachment is obsolete: true
(Assignee)

Comment 12

14 years ago
Comment on attachment 171670 [details] [diff] [review]
updated patch based on review comments

new patch to address daniel's review comments. (Thanks for that quick turn
around!)

Daniel to answer two of your questions:

1) I confirmed there wasn't a cylce leak
2) I may end up making the # of words in the menu a configureable pref down the
line.
Attachment #171670 - Flags: review?(daniel)
(In reply to comment #12)

> Daniel to answer two of your questions:
> 
> 1) I confirmed there wasn't a cylce leak
> 2) I may end up making the # of words in the menu a configureable pref down the
> line.

Scott, I still don't understand the quotes test. Why only single-quotes ?
(Assignee)

Comment 14

14 years ago
Created attachment 171804 [details] [diff] [review]
updated spellcheck module changes

fixes lots of edge cases and issues with deleting text in mispelled words.
Attachment #171311 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
Created attachment 171811 [details] [diff] [review]
updated editor patch, removes unnecessary code in nsTextServicesDocument.
Attachment #171670 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #171670 - Flags: review?(daniel)
(Assignee)

Comment 16

14 years ago
Comment on attachment 171811 [details] [diff] [review]
updated editor patch, removes unnecessary code in nsTextServicesDocument.

Thanks for making me revist the change in
nsTextServicesDocument::FindWordBounds
with the check for single quotes. 

That part of the patch was from Neil's original code. I think he was trying to
work around a problem with the I18n word breaker later on in the spell check
code when it trys to handle apostrophes. Where it does handle " correctly.

I was able to safely remove this code altogether as I'm working on changes
further down the pipeline to make the word breaker handle apostrophes.
Attachment #171811 - Flags: review?(daniel)
Comment on attachment 171811 [details] [diff] [review]
updated editor patch, removes unnecessary code in nsTextServicesDocument.

cool:-)
r=daniel@glazman.org
Attachment #171811 - Flags: review?(daniel) → review+
Comment on attachment 171310 [details] [diff] [review]
more layout changes

>+        //ITS OK TO CAST HERE THE RESULT WE USE WILLNOT DO BAD CONVERSION

Why are you yelling?  And what does this mean, anyway?
(Assignee)

Comment 19

14 years ago
Created attachment 172154 [details] [diff] [review]
updated spellcheck module changes (supports navigation)

This latest update to the spell check directory adds support for using the
arrow keys to navigate to another word (we now spell check the old word). For
using the mouse to move to another word (we now spell check the old word). And
initial support for pasting a group of words into the compose window. We now
spell check the new pasted text. 

This patch makes the inline spell checker instance register as a mouse and key
dom even listener so we can track all of these events.
(Assignee)

Updated

14 years ago
Attachment #171804 - Attachment is obsolete: true
(Assignee)

Comment 20

14 years ago
Created attachment 172703 [details] [diff] [review]
latest round of changes

Fixes include:
1) proper support for copy and paste
2) several code optimizations 
3) mispell two words in a row in a bullet item then hit return, before we would
undo t spelling underline on the first word
4) we now properly recognize URLS of the form http://foo or mailto:foo. When
pasting we never underling these words at all. When typing, we'll underline the
scheme name due to a hard to fix bug. 
5) mispellings for words with apostrophes like didnn't now get underlined
instead of treating the apostrophe as a word separator. In general we are much
smarter about guessing what a word is.
6) Add word to the dictionary issues should be fixed (with words that had
multiple spaces between them.
Attachment #172154 - Attachment is obsolete: true
Comment on attachment 171310 [details] [diff] [review]
more layout changes

nsTextFrame.cpp:

>@@ -3331,6 +3363,7 @@
>         {
>           char *currenttext  = iter.CurrentTextCStrPtr();
>           PRUint32   currentlength= iter.CurrentLength();
>+
>           //TextStyle &currentStyle = iter.CurrentStyle();

Seemed better the way it was...

>@@ -3367,6 +3400,12 @@
>         aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mColor,canDarkenColor));
>         aRenderingContext.DrawString(text, PRUint32(textLength), dx, dy + mAscent);
>       }
>+      }
>+      else if (!isPaginated) 
>+      {
>+        aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mColor,canDarkenColor));
>+        aRenderingContext.DrawString(text, PRUint32(textLength), dx, dy + mAscent);
>+      }
>       PaintTextDecorations(aRenderingContext, aStyleContext, aPresContext,
>                            aTextStyle, dx, dy, width,
>                            unicodePaintBuffer.mBuffer,

This looks like a merging error and should probably be undone.
Comment on attachment 171310 [details] [diff] [review]
more layout changes

sr=dbaron given comment 21.  And actually ignore comment 18 if you want -- I
see that that comment was there before.

But please make sure that you check in the whitespace changes that the "diff
-w" ignores.
Attachment #171310 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 23

14 years ago
Created attachment 172977 [details] [diff] [review]
updated layout changes with dbaron's review comments

updated patch with David Baron's review comments. Moving forward his review.
Attachment #171310 - Attachment is obsolete: true
Attachment #172977 - Flags: superreview+
(Assignee)

Comment 24

14 years ago
Created attachment 172983 [details] [diff] [review]
updated editor patch

Just re-freshing the editor changes against the trunk again. There was a merge
conflict in the diff with an IDL IID change. Carrying forward glazman's review
and module owner approval. Asking Bienvenu for an sr.
Attachment #171811 - Attachment is obsolete: true
Attachment #172983 - Flags: superreview?(bienvenu)
Attachment #172983 - Flags: review+

Updated

14 years ago
Attachment #172983 - Flags: superreview?(bienvenu) → superreview+

Comment 25

14 years ago
The layout changes were actually part of bug 181105. Also, bug 220157 should be
fixed by them as well.
(Assignee)

Comment 26

14 years ago
Created attachment 173104 [details] [diff] [review]
final editor patch

I made a minor change to nsTextDocumentServices to use a weak reference to the
editor object instead of a strong reference to prevent a circle. 

Carrying forward the reviews.
Attachment #172983 - Attachment is obsolete: true
Attachment #173104 - Flags: superreview+
Attachment #173104 - Flags: review+
(Assignee)

Comment 27

14 years ago
Created attachment 173113 [details] [diff] [review]
updated spell checker patch 

removed more dead code, add more comments about the purpose of the remaining
methods, added more argument null pointer checks. 

This patch is now ready to start the review process. Future issues with the
behavior (I'm sure there will be many) can be handled in separate bugs.
Attachment #173113 - Flags: superreview?
(Assignee)

Comment 28

14 years ago
Comment on attachment 173113 [details] [diff] [review]
updated spell checker patch 

It helps if I actually specify a reviewer name in the super review box....
Attachment #173113 - Flags: superreview? → superreview?(bienvenu)

Comment 29

14 years ago
Comment on attachment 173113 [details] [diff] [review]
updated spell checker patch 

a bunch of nits which you can ignore if you want...

+  NS_ENSURE_ARG_POINTER(aEnabled);
+  *aEnabled = mSpellCheck ? PR_TRUE : PR_FALSE; 
+  return NS_OK;

this could be *aEnabled = mSpellCheck != nsnull;

+      // spell check the text at the previous caret position, but no need to
+      // check the quoted text itself.

why don't we need to spell check the quoted text? Is that what other in line
spell checkers do?

+    case kOpUndo:
+    case kOpRedo:
+      // XXX handling these cases is quite hard -- this isn't quite right

you might say what's not quite right. I guess we need to spell check in case
undoing/redoing (re)introduces misspelling(s).

naming convention style nit:

+      nsAutoString parenttagname;
+      parentelement->GetTagName(parenttagname);
+
+      if (parenttagname.Equals(NS_LITERAL_STRING("blockquote"),
nsCaseInsensitiveStringComparator()))

parentTagName

similarly for

+      const PRUnichar *textchrs = textblock.get();
+      PRInt32 textlength = textblock.Length();
and there are a few others, like currentrange, checkrange, etc...

+  nsCOMPtr<nsISelection> spellCheckSelection;
+  rv = selcon->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
aSpellCheckSelection);
+  return rv;

can just return selcon->GetSelection...

+  selection->GetFocusOffset(&mCurrentSelectionOffset);
+
+  return NS_OK;

maybe can just return selection->GetFocusOffset(), though maybe you just want
to always return NS_OK...

+  PRBool done, isMispelled;

should be isMisspelled :-)
Attachment #173113 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 30

14 years ago
Created attachment 173197 [details] [diff] [review]
updated patch with review comments for spell check component
Attachment #172703 - Attachment is obsolete: true
Attachment #173113 - Attachment is obsolete: true
Attachment #173197 - Flags: superreview+
(Assignee)

Comment 31

14 years ago
all three patches have been checked in. Marking fixed. I'll treat inline spell
check behavior issues in individual bugs going forward. 
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 32

14 years ago
I just tested 1.8b1 and this version doesn't skip quoted text during spellcheck.
So I tested the oldest available nightly of 15th February and the newest nightly
of 1st March. In both versions the spellchecker doesn't skip the quotes.

Then I tested 1.8a6. This version does still skips the quotes. Therefore I think
the bug came back between the release of 1.8a6 and 15th February.

The patch for this bug was checked in during this period. There were not much
other checkins in editor. Is it possible, that the patch effected the skipping
of quoted text?

Comment 33

14 years ago
Someone tested some older Seamonkey-Tinderbox-Creature-Zip-Builds for me.
Result:
2005020111 -- quoted text is not checked
2005020118 -- quoted text is checked

So it appears to prove true, that this patch causes this behavior.

Should I open a new bug, or will this patch be fixed here?

Updated

14 years ago
Blocks: 58612
Blocks: 1122051
You need to log in before you can comment on or make changes to this bug.