Closed
Bug 173046
Opened 22 years ago
Closed 22 years ago
SpellCheck in Mail should skip blockquote
Categories
(SeaMonkey :: Composer, defect, P1)
SeaMonkey
Composer
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.3beta
People
(Reporter: rods, Assigned: bugzilla)
References
Details
Attachments
(2 files, 8 obsolete files)
(deleted),
patch
|
mozeditor
:
review+
kinmoz
:
superreview+
asa
:
approval1.3a+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rods
:
review+
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
When blockquotes are of type "cite" spellcheck needs to skip them.
Reporter | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
Reporter | ||
Updated•22 years ago
|
Summary: SpellCheck in Mail shoild skip blockquote → SpellCheck in Mail should skip blockquote
Reporter | ||
Comment 1•22 years ago
|
||
I was going to move a bugscape bug over, but I have decided against it
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 2•22 years ago
|
||
Ok, I do want this bug open
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Comment 4•22 years ago
|
||
Basically this patch wraps the ContentIterator with a class that can have a
filter.
The filter can then be used to "skip" certain content nodes like a blockquote.
When composing in mail and doing spell check we ask the mail iface if there is
a filter to be installed for filtering the mail content.
Comment on attachment 101999 [details] [diff] [review]
patch v1
==== I think we may want to have a filter that is shared by both Composer and
MailNews, the only difference would be that mail would conditionally tell the
filter to also skip over the blockquote[cite=cite]. Having said that, this
would probably have to be moved out from nsIEditorMailSupport.idl:
+ void getContentIteratorFilter(out nsIContentIteratorFilter aFilter);
==== This filter doesn't belong at the editor core level, since it is an app
specific thing. Editor core should not know anything about spellchecking:
+ nsMailSpellCheckFilter.cpp \
==== I think this needs to be restructured, otherwise we'll never check for
anything but blockquotes:
+ if (tag) {
+ if (tag == mBlockQuoteAtom) {
+ nsAutoString cite;
+ if (NS_SUCCEEDED(aNode->GetAttr(kNameSpaceID_None, mTypeAtom, cite)))
{
+ return cite.Equals(NS_LITERAL_STRING("cite").get());
+ }
+ }
+ } else if (tag == mScriptAtom) {
+ return PR_TRUE;
+ } else if (tag == mTextAreaAtom) {
+ return PR_TRUE;
+ } else if (tag == mSelectAreaAtom) {
+ return PR_TRUE;
+ } else if (tag == mMapAtom) {
+ return PR_TRUE;
+ }
==== The filter interface should be implemented in IDL so that people can have
a filter written in JS if necessary. I'd also rename it to something like
nsTextServicesFilter.idl instead of nsIContentIteratorFilter.h.
==== InitWithDocument() needs to be modified too. Are you sure you want to tie
the filter to the Init methods? Doing so means we won't be able to dynamically
add filters (imagine checkboxes on a dialog).
- NS_IMETHOD InitWithEditor(nsIEditor *aEditor) = 0;
+ NS_IMETHOD InitWithEditor(nsIEditor *aEditor, nsIContentIteratorFilter*
aFilter) = 0;
==== I really think the skipping needs to be done in
FilteredContentIterator::Next() and Prev() instead of CurrentNode(), so that
the direction of iteration can be taken into account. Also, you have to keep in
mind that the next or previous sibling can also be a node to skip, so there
needs to be some sort of loop. Also if there is no next or previous sibling it
may be necessary to go to the parent's next or previous sibling.
==== Also the |IsBlock()| should be |DidCrossBlockBoundary()| or something of
that nature, and that should get set to true whenever we skip something.
Attachment #101999 -
Flags: needs-work+
Reporter | ||
Comment 6•22 years ago
|
||
*** Bug 145706 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 7•22 years ago
|
||
A bit different from the first patch:
1) Created a new iface nsITextServicesFilter
2) The impl of this filter is craeted by the EditorShell and set into the
TextService obj
3) A new obj FilteredContentIterator "wraps" a content iterator obj and has
nsITextServicesFilter to figure out what nodes need to be skipped.
4) As the TexSvc processes node it uses the filtered content iterator and can
then skip nodes going forward or backward.
Attachment #101999 -
Attachment is obsolete: true
Cc jfrancis ... we talked long ago about filtered content iterators in the
context of this bug, and I remember him saying that he was thinking of or had
some other uses for such a thing. I'm wondering if we should have a generic
iterator filter mechanism outside of the textservices module that could be used
by all.
Comment on attachment 102808 [details] [diff] [review]
patch v2
In the context of the TextServices, this all looks good. It may need more work
if we wanted a general content filtering mechanism to share between
mozilla/content and mozilla/editor/txtsvc.
==== I'd probably rename this file and it's class to nsComposerTSFilter or
nsComposerTxtsvcFilter:
+ nsComposeFilter.cpp \
==== FYI, all spellchecker related code that lived in |nsEditorShell| has now
moved to a different class |nsEditorSpellCheck|. So some of the nsEditorShell
code in this patch will need updating/changing.
==== Right now the skipping of quotes is tied to the |mMailCompose| editor
isn't there supposed to be some sort of pref that we need to check in addition?
That is I thought we were supposed to be able to dynamically turn on/off the
skipping of quotes in mail?
+ if (NS_SUCCEEDED(CreateComposeFilter(getter_AddRefs(txtSvcFilter),
mMailCompose))) {
==== I see a couple of references to |nsIContentIteratorFilter| which is a
class/interface that doesn't exist.
+interface nsIContentIteratorFilter;
+class nsIContentIteratorFilter;
==== The comment shouldn't reference spellchecking, instead it should say
something generic like "Sets the filter to be used while iterating over
content. @param aFilter filter to be used while iterating over content." Also,
the current implementation isn't very dynamic since it requires that any filter
used, must be set prior to calling the |InitWithDocument()| or
|InitWithEditor()| methods which create the iterator. That should be mentioned
in the comment too.
+
+ /**
+ * Sets a filter
+ * @param aFilter filter to be used to filter content for spellcheck
+ */
+ NS_IMETHOD SetFilter(nsITextServicesFilter *aFilter) = 0;
==== |nsIContent| isn't scriptable so this might have to be changed to
|nsIDOMNode|. What we want to do is eventually allow the mail guys or embedders
to add filters, in js or C++.
+ boolean skip(in nsIContent aNode);
==== FilteredContentIterator's |First()| and |Last()| may also need to do some
skipping/advancing. A real world example of this would be the "Start after
quoted reply" pref in mail, since that would mean the content we want to be
skipped would be the first thing the iterator would iterate over.
==== The textservices does a fair amount of calls to PositionAt(), you may need
to clear/reset some of FilteredContentIterator's state flags in that method?
==== You may want to track |mDirection| in FilteredContentIterator's
|MakePre()| and |MakePost()| just in case the caller manually switches things
up! Right now the TextServicesCode doesn't switch the modes on the iterator,
but if this code were ever going to be made generic it brings some interesting
issues of how to advance to the non-filtered node without interfering with the
user's specified traversal.
==== I don't like the fact that the |IsBlock()| now takes an iterator. It's
meant to be a generic method to get info about a node. You are tying the node
passed in to the current state of an iterator. It's probably best to move that
code into it's own utility method and call it from the places that use the
iterator. Are you sure that all |IsBlock()| calls need to know boundary
crossings? It's been a while, so I don't remember off the top of my head.
-nsTextServicesDocument::IsBlockNode(nsIContent *aContent)
+nsTextServicesDocument::IsBlockNode(nsIContent *aContent, nsIContentIterator*
aFilteredIter)
{
+ // We can assume here that the Iterator is a FilteredContentIterator because
+ // all the iterator are created in CreateContentIterator which create a
+ // FilteredContentIterator
+ // So if the iterator bailed on one of the "filtered" content nodes then we
+ // consider that to be a block and bail with PR_TRUE
+ if (aFilteredIter) {
+ FilteredContentIterator* filter = NS_STATIC_CAST(FilteredContentIterator
*, aFilteredIter);
+ if (filter) {
+ if (filter->DidCrossBlockBoundary()) {
+ return PR_TRUE;
+ }
+ }
+ }
+
Comment 10•22 years ago
|
||
Comment on attachment 102808 [details] [diff] [review]
patch v2
==== These enums are kind of the same, can't we share their use?
+ typedef enum {eDirNotSet, eForward, eBackward} eDirectionType;
+ typedef enum {eAdvNext, eAdvPrev} eAdvNodeDirType;
Reporter | ||
Comment 11•22 years ago
|
||
This patch:
1)Does not handle First and Last, in otherwaords, if the very first node is a
blockquote it won't skip over it. Fixing that case will require reworking the
ContentIterator
2) It now creates the filter from a factory. The filter can be created for mail
or for composer.
3) The FilteredContentIterator and the nsITextServicesFilter use nsIDOMNode so
filters can be scriptable.
Attachment #102808 -
Attachment is obsolete: true
Reporter | ||
Comment 12•22 years ago
|
||
For your reviewing/super-reviewing convenience
Reporter | ||
Updated•22 years ago
|
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Comment 13•22 years ago
|
||
could this bug be super-reviewed?
Reporter | ||
Comment 14•22 years ago
|
||
We are waiting one some changes to the content iterators before this can be
checked in. This bug should have a dependency...
Comment 15•22 years ago
|
||
Hi All,
A fresh build that works with 1.2b (1.2.0.2002111508) was posted
at http://bugzilla.mozilla.org/attachment.cgi?id=106306&action=view
Unfortunately, it still checks block quoted text (which is sad,
as it makes pathetic an otherwise excellent piece of writing). We may
all have to wait for Netscape's spell checker to catch up with 1.2.
--Tony
Comment 16•22 years ago
|
||
rod, can we get an updated diff to review? The last set of changes in this bug
don't have the modifications I gave you that call ClearDidCrossBoundary() which
fixed some of your test cases.
Reporter | ||
Comment 17•22 years ago
|
||
Same as Patch v3 except the FilteredIterator now used Kin's new pre/post
iterators
Attachment #103311 -
Attachment is obsolete: true
Attachment #103312 -
Attachment is obsolete: true
Reporter | ||
Updated•22 years ago
|
Attachment #106919 -
Flags: superreview?(kin)
Attachment #106919 -
Flags: review?(jfrancis)
Comment 18•22 years ago
|
||
Just wanted to mention a few related issues:
http://mozdev.org/bugs/show_bug.cgi?id=751 : "<name> wrote:" quotation line
should not be checked
http://mozdev.org/bugs/show_bug.cgi?id=752 : Quoted text should not be checked.
http://mozdev.org/bugs/show_bug.cgi?id=788 : Don't spellcheck sigs
http://mozdev.org/bugs/show_bug.cgi?id=971 : Spellchecker should ignore URLs
OS: Windows 2000 → All
Hardware: PC → All
Reporter | ||
Comment 19•22 years ago
|
||
Aleksey, These other things need to be "wrapped" in special kind of tags that we
can then skipped. The mail team is aware.
Comment 20•22 years ago
|
||
Comment on attachment 106919 [details] [diff] [review]
patch v4
==== Change "node" to "nodes":
+ // For node that are blockquotes, we must make sure
==== We'll want to remove this debugging ifdef code before checkin:
+ if (tag == mBlockQuoteAtom) {
+#if 1
+ *_retval = PR_TRUE;
+ return NS_OK;
+#else
==== Do we want to do a case-insensitive compare?
+ *_retval = cite.Equals(NS_LITERAL_STRING("cite").get());
==== Were you going to fill in these comments? :-)
+/**
+ *
+ */
+class nsComposeTxtSrvFilter : public nsITextServicesFilter
==== Perhaps we should define the Contract/ProgID here too so that it's easier
for folks find. The define could also
be used in place of the hardcoded string in nsComposerRegistration.cpp.
+#define NS_TXTSRVFILTER_CID \
+{/* {171E72DB-0F8A-412a-8461-E4C927A3A2AC}*/ \
+0x171e72db, 0xf8a, 0x412a, \
+{ 0x84, 0x61, 0xe4, 0xc9, 0x27, 0xa3, 0xa2, 0xac} }
+
==== Make the indentation for the code in |nsComposeTxtSrvFilterConstructor()|
match the rest of the file.
==== What's this comment for?
+/* void setFilter (in nsITextServicesFilter filter); */
+NS_IMETHODIMP
+nsEditorSpellCheck::SetFilter(nsITextServicesFilter *filter)
==== We shouldn't be depending on txtsvc in core editor code, specifically,
editor/libeditor/base/Makefile.in or
editor/libeditor/text/Makefile.in
layout \
content \
txmgr \
+ txtsvc \
htmlparser \
necko \
pref \
==== So why does the constructor for |nsFilteredContentIterator()| take a range
arg? |mRange| should be set in the
range version of |Init()| so that we are totally in sync with the range that
get's used when |mIter->Init(aRange)|
is called.
==== In |nsFilteredContentIterator::Init(nsIContent* aRoot)| we should probably
create an |mRange| that selects the
root node. This can be done easily with the |SelectNode()| method on nsIRange.
==== Like I said above, |nsFilteredContentIterator::Init(nsIDOMRange* aRange)|
should be setting mRange. Do we need
to create a copy of |aRange| and set |mRange| to the copy? Maybe not if the
only code that creates this iterator
doesn't modify the range after it gives it to us. Note that the underlying
content iterator doesn't store a range so
this isn't an issue for it.
==== You can no longer dynamically switch directions so this comment |Next()|
and |Prev()| isn't necessary anymore.
+ // ASSUMPTION: We are assuming here that no one will
+ // call MakePre or MakePost while iterating
==== Shouldn't this be doing some initialization?
+ nsFilteredContentIterator() { }
==== This debugging code needs to be commented out or removed ... as well as
the KDEBUG code you enabled.
+int cnt = 0;
NS_IMETHODIMP
nsTextServicesDocument::DeleteSelection()
{
==== You set |result| based on |filter|, but if |filter| were ever null you
would've crashed because of the AddRef.
+ nsFilteredContentIterator* filter = new
nsFilteredContentIterator(mTxtSvcFi
lter, aRange);
+ *aIterator = NS_STATIC_CAST(nsIContentIterator *, filter);
+ NS_ADDREF(*aIterator);
+ result = filter?NS_OK:NS_ERROR_FAILURE;
Comment 21•22 years ago
|
||
I purposely didnt read kin's review comments, so there is probably overlap.
Makefile.in:
+ nsComposeTxtSrvFilter.cpp
You also need to add this to the mac build, via editor.xml. Similarly
include your other new source files in the appropriate project.xml
---------------------
in nsComposeTxtSrvFilter.cpp:
+nsComposeTxtSrvFilter::nsComposeTxtSrvFilter() :
+ mIsForMail(PR_FALSE)
+{
+ NS_INIT_ISUPPORTS();
+
+ mBlockQuoteAtom = getter_AddRefs(NS_NewAtom("blockquote"));
+ mTypeAtom = getter_AddRefs(NS_NewAtom("type"));
+ mScriptAtom = getter_AddRefs(NS_NewAtom("script"));
+ mTextAreaAtom = getter_AddRefs(NS_NewAtom("textarea"));
+ mSelectAreaAtom = getter_AddRefs(NS_NewAtom("select"));
+ mMapAtom = getter_AddRefs(NS_NewAtom("mMapAtom"));
+}
Is this an exhaustive list of the tags you want to skip? Do we need
<head>, <object>, <applet>, & <embed>?
+#if 1
+ *_retval = PR_TRUE;
+ return NS_OK;
+#else
should be removed
---------------------
in nsComposeTxtSrvFilter.cpp:
+#define NS_TXTSRVFILTER_CID \
CID should have a name closer to the implementation (nsComposeTxtSrvFilter)
to avoid confusion with other (future) Text Services Filters.
---------------------
in nsComposerRegistration.cpp:
throughout, and in the header, mDidCross is described as indicating
whether you crossed a block boundary, when in fact it means did
you skip a node. Fix discriptions. In fact, I suggest mDidSkip.
Similarly, rename:
+ PRBool DidCrossBlockBoundary() { return mDidCross; }
+ void ClearDidCrossBlockBoundary() { mDidCross = PR_FALSE; }
fix typo:
+// There are no macros that enable use to have 2 constructors
+static NS_IMETHODIMP
There are two static function that are NS_IMETHODIMP. Should be
static nsresult.
---------------------
in nsFilteredContentIterator.cpp:
+nsFilteredContentIterator::~nsFilteredContentIterator()
+{
+ mIterator = nsnull;
+}
mIterator is an nsCOMPtr. nulling it is unneccessary.
+nsFilteredContentIterator::Init(nsIContent* aRoot)
+{
+ NS_ENSURE_TRUE(mIterator, NS_ERROR_FAILURE);
+ mIsOutOfRange = PR_FALSE;
+ mDirection = eForward;
+ mCurrentIterator = mPreIterator;
+ return mCurrentIterator->Init(aRoot);
+}
+
+NS_IMETHODIMP
+nsFilteredContentIterator::Init(nsIDOMRange* aRange)
+{
+ NS_ENSURE_TRUE(mIterator, NS_ERROR_FAILURE);
+ mIsOutOfRange = PR_FALSE;
+ mDirection = eForward;
+ mCurrentIterator = mPreIterator;
+ return mCurrentIterator->Init(aRange);
+}
Add NS_ENSURE_TRUE(mPreIterator, NS_ERROR_FAILURE); to these.
+nsresult
+nsFilteredContentIterator::AdvanceNode(nsIDOMNode* aNode, nsIDOMNode*&
aNewNode, eDirectionType aDir)
...
+ aNode->GetParentNode(getter_AddRefs(parent));
+ NS_ASSERTION(parent, "parent can't be NULL");
+
+ // Make sure the parent is in the DOMRange before going further
+ PRBool intersects;
+ nsresult rv = mRange->IntersectsNode(parent, &intersects);
+ if (NS_SUCCEEDED(rv) && intersects) {
How can parent not intersect range? Presumably aNode intersected range, right?
+void
+nsFilteredContentIterator::CheckAdvNode(nsIDOMNode* aNode, PRBool& aDidCross,
eDirectionType aDir)
+{
...
+ } else {
+ return; // fell out of range
CheckAdvNode() only skips over nodes if it finds a later node not to skip.
This has some ramifications for the rest of the nsFilteredContentIterator
class. For instance, if a range has only a mailcite in it, First() will
be set to the mailcite (and mIsOutOfRange will be set). But since
CurrentNode() does not check mIsOutOfRange, you will spell check the
mailcite.
In Next() & Prev(), delete the lines:
+ // ASSUMPTION: We are assuming here that no one will
+ // call MakePre or MakePost while iterating
In Next(), Prev(), and CurrentNode():
check mIsOutOfRange and do the right thing.
---------------------
nsTextServicesDocument.cpp:
+int cnt = 0;
...
+ printf("\n---- Before Delete %d\n", cnt++);
+ printf("Sel: (%2d, %4d) (%2d, %4d)\n", mSelStartIndex, mSelStartOffset,
mSelEndIndex, mSelEndOffset);
+ PrintOffsetTable();
remove/ifdef/comment-out debugging cruft
*aIterator = 0;
+ // Create a nsFilteredContentIterator
+ // This class wraps the ContentIterator in order to give itself a chance
+ // to filter out certain content nodes
+ if (!*aIterator) {
Why the if statement? We know *aIterator = 0 from line above.
+ result = filter?NS_OK:NS_ERROR_FAILURE;
some whitespace in there would be nice. I would prefer
if (!filter)
return NS_ERROR_FAILURE;
which should do the job given what happens later.
+nsTextServicesDocument::ClearDidCrossBoundary(nsIContentIterator* aFilteredIter)
+{
+ // We can assume here that the Iterator is a nsFilteredContentIterator because
+ // all the iterator are created in CreateContentIterator which create a
+ // nsFilteredContentIterator
+ // So if the iterator bailed on one of the "filtered" content nodes then we
+ // consider that to be a block and bail with PR_TRUE
fix comment. Looks copied from nsTextServicesDocument::DidCrossBoundary()
in FirstTextNodeInCurrentBlock():
+
+ if (DidCrossBoundary(iter))
+ break;
This terminates the loop, possibly before we find the first text node in
block. I guess you are relying on skipped nodes always being block
boundaries in their own right. Do you mean to make that assumtion?
Filters that looked for spans that had a style attribute with
visibilty : hidden, for instance, could not work with this.
Similar comments apply to FirstTextNodeInNextBlock(). And though
not touched by the patch, FirstTextNodeInPrevBlock() is also vulnerable
since it uses FirstTextNodeInCurrentBlock() under the hood.
Comment 22•22 years ago
|
||
Rod, ignore my last comment in the review. I am unfamiliar with Text Services
and didnt realize that it's "blocks" were actually just portions of what I
ordinarily think of when I hear "block".
Comment 23•22 years ago
|
||
Comment on attachment 106919 [details] [diff] [review]
patch v4
Is there a tab on this line:
+ nsComposeTxtSrvFilter.cpp \
In nsComposeTxtSrvFilter.cpp, is the copyright year correct (same question for
nsComposeTxtSrvFilter.h, nsITextServicesFilter.idl,
nsFilteredContentIterator.cpp, nsFilteredContentIterator.h):
+ * Portions created by the Initial Developer are Copyright (C) 1998
This block doesn't deal with null content; perhaps you intended to do the if
check after the QI?
+ if (aNode) {
+ nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
+ nsCOMPtr<nsIAtom> tag;
+ content->GetTag(*getter_AddRefs(tag));
spell out "nxt" in nsFilteredContentIterator.cpp:
+ nsCOMPtr<nsIDOMNode> nxtNode;
I'm not sure about others, but I prefer when local variables are named things
like "ignore" or "ignoreDidCross" such as here:
+ PRBool didCross;
+ CheckAdvNode(node, didCross, eBackward);
In nsFilteredContentIterator.h, use PRPackedBool instead of PRBool? (Is there
any chance any other booleans might be added some day?)
I don't see any mac project file changes.
Attachment #106919 -
Flags: superreview?(kin) → superreview-
Comment 24•22 years ago
|
||
doh! I forgot to mention for this block:
+ mBlockQuoteAtom = getter_AddRefs(NS_NewAtom("blockquote"));
+ mTypeAtom = getter_AddRefs(NS_NewAtom("type"));
+ mScriptAtom = getter_AddRefs(NS_NewAtom("script"));
+ mTextAreaAtom = getter_AddRefs(NS_NewAtom("textarea"));
+ mSelectAreaAtom = getter_AddRefs(NS_NewAtom("select"));
+ mMapAtom = getter_AddRefs(NS_NewAtom("mMapAtom"));
I think we want to use do_getAtom() instead of NS_NewAtom
Reporter | ||
Comment 25•22 years ago
|
||
Attachment #106919 -
Attachment is obsolete: true
Reporter | ||
Comment 26•22 years ago
|
||
Comment on attachment 108025 [details] [diff] [review]
patch with all reviewer's comments
bringing joe's r= forward
Attachment #108025 -
Flags: superreview?(kin)
Attachment #108025 -
Flags: review?(jfrancis)
Comment 27•22 years ago
|
||
Rod, the latest patch does not have nsFilteredContentIterator.cpp, so I was
unable to confirm that the fixes were in. Can you attach a complete patch?
Comment 28•22 years ago
|
||
Comment on attachment 108025 [details] [diff] [review]
patch with all reviewer's comments
r=brade with a few tweaks we discussed on AIM
Attachment #108025 -
Flags: review?(jfrancis) → review+
Reporter | ||
Comment 29•22 years ago
|
||
Attachment #108025 -
Attachment is obsolete: true
Reporter | ||
Updated•22 years ago
|
Attachment #108027 -
Flags: superreview?(kin)
Attachment #108027 -
Flags: review?(jfrancis)
Reporter | ||
Comment 30•22 years ago
|
||
forgot the "-N" option
Attachment #108027 -
Attachment is obsolete: true
Reporter | ||
Updated•22 years ago
|
Attachment #108028 -
Flags: superreview?(kin)
Attachment #108028 -
Flags: review?(jfrancis)
Comment 31•22 years ago
|
||
Rod, Next(), Prev(), and CurrentNode() still don't check mIsOutOfRange. Is this
intentional?
Reporter | ||
Comment 32•22 years ago
|
||
Attachment #108028 -
Attachment is obsolete: true
Reporter | ||
Updated•22 years ago
|
Attachment #108071 -
Flags: superreview?(kin)
Attachment #108071 -
Flags: review?(jfrancis)
Attachment #108025 -
Flags: superreview?(kin) → superreview-
Attachment #108028 -
Flags: superreview?(kin) → superreview-
Attachment #108027 -
Flags: superreview?(kin) → superreview-
Comment 33•22 years ago
|
||
Comment on attachment 108071 [details] [diff] [review]
final patch?
You may want to make the mIsOutOfRange check in CurrentNode() casue an error
return value, in order to sta consistent with the underlying iterator (which
returns an error if it cannot find a current node).
Attachment #108071 -
Flags: review?(jfrancis) → review+
Comment 34•22 years ago
|
||
Comment on attachment 108071 [details] [diff] [review]
final patch?
sr=kin@netscape.com
==== Fix the "usnig" and "ski;p" typos:
+ * This class implements a filter interface, that enables
+ * those usnig it to ski;p over certain nodes when traversing content
==== I would change "editor" to "composer" since this lives in the composer
dll, but it's up to you:
+// Generic for the editor
+#define COMPOSER_TXTSRVFILTER_CONTRACTID
"@mozilla.org/editor/txtsrvfilter;1"
+
+// This is the same but includes "cite" typed blocked quotes
+#define COMPOSER_TXTSRVFILTERMAIL_CONTRACTID
"@mozilla.org/editor/txtsrvfiltermail;1"
==== DO we need to check the return status of the |SelectNode()| call?
+ if (domRange && domNode) {
+ domRange->SelectNode(domNode);
+ }
+
+ rv = mPreIterator->Init(domRange);
==== Get rid of the extra spaces in the arg list:
+ rv = nsTextServicesDocument::ComparePoints(aEndNode, aEndOffset,
parentNode, indx, &endRes);
==== Shouldn't these be using the COMPOSER_TXTSRVFILTER_CONTRACTID and
COMPOSER_TXTSRVFILTERMAIL_CONTRACTID defines?
+ filterContractId = "@mozilla.org/editor/txtsrvfiltermail;1";
+ else
+ filterContractId = "@mozilla.org/editor/txtsrvfilter;1";
Attachment #108071 -
Flags: superreview?(kin) → superreview+
Updated•22 years ago
|
Attachment #106919 -
Flags: review?(jfrancis)
Updated•22 years ago
|
Attachment #108028 -
Flags: review?(jfrancis)
Updated•22 years ago
|
Attachment #108027 -
Flags: review?(jfrancis)
Updated•22 years ago
|
Flags: blocking1.3a?
Comment 36•22 years ago
|
||
Comment on attachment 108071 [details] [diff] [review]
final patch?
I assume the "blocking1.3a" was intended as a request to land this patch (which
should be done with the "approval1.3a" flag in the patch manager). Removed
blocking request and a=asa for checkin to 1.3a.
Attachment #108071 -
Flags: approval1.3a+
Reporter | ||
Comment 37•22 years ago
|
||
fixed
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 38•22 years ago
|
||
Note that Mac is still broken; see bug 184935.
Comment 39•22 years ago
|
||
Tried to read most comments, and I can't read code, so sorry if this was already
addressed:
Assuming this bug is to make spelllchecker skip quoted text ("> bla bla"),
perhaps there should be some UI checkbox to let the user decide if he *does*
want the quoted text to be checked (the default should be not to check quoted text).
| |
| Language: [x] Spellcheck quoted text |
| [ English/US ] [Close ] |
+--------------------------------------------+
Comment 40•22 years ago
|
||
Hi All,
If this is fixed, is there somewhere we can download it from to give
it a try out?
Many thanks,
--Tony
aewell@bbis.com
Comment 41•22 years ago
|
||
> If this is fixed, is there somewhere we can download it from to give
> it a try out?
Download (or compile) the latest nightly builds and download (or compile) the
spellchecker as described on http://spellchecker.mozdev.org/
Comment 42•22 years ago
|
||
Hi Aleksey,
I downloaded http://mozillacafe.org/spellchecker_1.3.xpi from
Spellchecker for Mozilla 1.3a and Nightly builds
Windows install download (2002-12-15)Bug 2678 has been fixed.
Linux x86 install download (2002-12-15)
It still is not working. What did I miss?
--Tony
aewell@gbis.com
Comment 43•22 years ago
|
||
This fix landed *after* 1.3a. It also "broke" binary compatibility. So you'll
need to find (or compile yourself) a binary that specifically targets post-1.3a
nightlies.
Assignee | ||
Comment 44•22 years ago
|
||
this is working only when the user press the spell check button on the toolbar.
It does not work when the spellchecker is automatically invoked while sending a
message.
I have a fix coming...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 45•22 years ago
|
||
reassign to myself...
Assignee: rods → ducarroz
Status: REOPENED → NEW
Assignee | ||
Comment 46•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #109554 -
Flags: review?(rods)
Assignee | ||
Comment 47•22 years ago
|
||
Also, if I correctly understand the implementation, this feature works only when
composing message using an HTML compose window. More works is needed for
plaintext compose, I'll open another bug...
Status: NEW → ASSIGNED
Flags: blocking1.3b?
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Reporter | ||
Comment 48•22 years ago
|
||
Comment on attachment 109554 [details] [diff] [review]
Propsed fix for remaining problem, v1
r=rods
Attachment #109554 -
Flags: review?(rods) → review+
Assignee | ||
Updated•22 years ago
|
Attachment #109554 -
Flags: superreview?(sspitzer)
Comment 49•22 years ago
|
||
Should i forget about comment #39?
Assignee | ||
Comment 50•22 years ago
|
||
Peter, your best change would be to file a new bug...
Comment 51•22 years ago
|
||
Comment on attachment 109554 [details] [diff] [review]
Propsed fix for remaining problem, v1
sr=sspitzer
Attachment #109554 -
Flags: superreview?(sspitzer) → superreview+
Assignee | ||
Comment 52•22 years ago
|
||
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 53•22 years ago
|
||
Comment on attachment 109554 [details] [diff] [review]
Propsed fix for remaining problem, v1
>+ var spellCheckMail = (window.document.firstChild.getAttribute("windowtype") == "msgcompose");
Actually editor.js has a function IsWebComposer() which might do what you want
(except in reverse, i.e. var spellCheckMail = !IsWebComposer();).
Updated•22 years ago
|
Flags: blocking1.3b? → blocking1.3b-
Comment 54•22 years ago
|
||
OK, i just filed bug 186825 to address the suggestion I made in comment #39.
Comment 55•22 years ago
|
||
JF: in comment #47 you said you would spin another bug for skipping
spellchecking quoted portions in plain text compose messages. Could you post
that bug number here please?
Assignee | ||
Comment 56•22 years ago
|
||
I haven't yet...
Comment 57•22 years ago
|
||
The changes in the latest patch makes the single 'Close' button into two buttons
a 'Send' one and a 'Stop' one. Is there code attached to make the 'Send' button
send the email yet? Logged bug 190734 about 'Send' button not working.
Comment 58•22 years ago
|
||
Last checkin caused another regression; spell checker does not appear if there
are no misspelled words.
Comment 59•22 years ago
|
||
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210
Quoted text is still being spell checked if the original message contains either
an email address or URL.
Curiously, editing the quoted text in the new email to include either an email
address or URL does not result in the quoted text getting spell checked, which
is good.
An email address or URL has to be present in the original email, prior to
quoting it in the email being composed, in order for this spellchecker bug to
show itself again.
If neither email address nor URL are present, all works as advertised and the
quoted text is not spell checked.
Time to reopen the bug.
Comment 60•22 years ago
|
||
*** Bug 181391 has been marked as a duplicate of this bug. ***
Comment 61•22 years ago
|
||
*** Bug 198032 has been marked as a duplicate of this bug. ***
Comment 62•22 years ago
|
||
Re comment 55, I have logged bug 201866 for the plain text compose case.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 63•20 years ago
|
||
I just tested 1.8b1 and this version doesn't skip the quotes.
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 skip the quotes. Therefore the bug
mus came back between the release of 1.8a6 and 15th February.
Can somebody verify this and reopen the bug?
You need to log in
before you can comment on or make changes to this bug.
Description
•