Closed
Bug 151040
Opened 22 years ago
Closed 18 years ago
Add access to Mac OS X global spelling checkers
Categories
(Camino Graveyard :: OS Integration, enhancement, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
Camino1.5
People
(Reporter: Saarinen, Assigned: mikepinkerton)
References
(Blocks 1 open bug)
Details
(Keywords: fixed1.8.1)
Attachments
(7 files, 13 obsolete files)
21.82 KB,
patch
|
Details | Diff | Splinter Review | |
771 bytes,
text/html
|
Details | |
20.92 KB,
image/jpeg
|
Details | |
8.30 KB,
patch
|
bryner
:
superreview+
bzbarsky
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
18.80 KB,
patch
|
bryner
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
809 bytes,
patch
|
mikepinkerton
:
review+
|
Details | Diff | Splinter Review |
9.97 KB,
patch
|
Details | Diff | Splinter Review |
Please consider adding access to Mac OS X global spelling checker for all text
blocks. I spend so much time filling out forms and such on the web (like this
one for instance!) that it would really be helpfull to have access to a spelling
checker. See OmniWeb for a decent implementation of this feature.
Comment 1•22 years ago
|
||
good idea
Status: NEW → ASSIGNED
Priority: -- → P5
Summary: Add access to Mac OS X global spelling checker → [RFE] Add access to Mac OS X global spelling checker
Comment 4•22 years ago
|
||
Related to/Dependant on bug <a href="http://bugzilla.mozilla.org/show_bug.cgi?id=148098">
148098</a>?
Updated•22 years ago
|
Summary: [RFE] Add access to Mac OS X global spelling checker → Add access to Mac OS X global spelling checker
Comment 5•22 years ago
|
||
*** Bug 175645 has been marked as a duplicate of this bug. ***
Comment 6•22 years ago
|
||
This would be a nice feature, but I wouldn't prioritise it over fixing bug
148098... we don't need form input to get any slower on old machines. :-)
Comment 7•22 years ago
|
||
If we implement a native text widget (bug 148098, we get spell-checking (as
well as services and things listed in a dozen other bugs) for free. It is
probably also possible to hack in the spell checker while still using gecko
text, but this is still unlikely to create any major performance hit. It _is_
going to be important to decide at some point whether the difficulties
implementing features to fix all the problems (both with features and with
performance) in gecko text inputoutweigh the work it would take to use a native
widget; this bug is definitely closely realated to 148098, even though it
doesn't necessarily have to depend on it.
Comment 8•22 years ago
|
||
I think that the use native widgets would slow down scrolling and page
rendering. Omniweb, for example, is switching from native widgets because of
speed issues (or so I heard a couple months ago).
Comment 9•22 years ago
|
||
I've looked into this a little and the biggest problem seems to be a way to draw
the red dots under the misspelt words. I don't know if there is any way to do
this within a text field in Gecko.
How about just drawing the text in red, italics, perhaps? Most text entry
widgets don't support styled text anyhow.
Comment 11•22 years ago
|
||
drawing the red dots isn't a problem (iirc); I ran into problems with the OSX
spellchecker not showing its dialog.
Target Milestone: --- → Chimera1.1
Comment 12•22 years ago
|
||
This patch is where I left off (includes debugging etc); may be missing some
nib/resource changes.
Comment 13•22 years ago
|
||
brade what does it do?
Comment 14•22 years ago
|
||
kin: see patch attached (it's VERY rough)
Simon Woodside: the patch is a first attempt to invoke the OSX spell checker for
textareas / input fields. More needs to be done before it can land.
Comment 15•21 years ago
|
||
Anny news, would be nice for 1.0 release?
Comment 16•21 years ago
|
||
This is probably about the same as the previous patch with some other things
thrown in too. I am about to lose this computer so posting this patch in case
someone else wants to pick it up and make it work.
Updated•20 years ago
|
Status: NEW → ASSIGNED
Updated•20 years ago
|
Assignee: brade → qa-mozilla
Status: ASSIGNED → NEW
Comment 17•20 years ago
|
||
If Useful: Apple recently posted some sample code to the reference library.
Sample Code: SpellingChecker-CocoaCarbon
http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html
Comment 18•20 years ago
|
||
If Useful: Apple recently posted some sample code to the reference library.
Sample Code: SpellingChecker-CocoaCarbon
http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html
Comment 19•20 years ago
|
||
Ludo, would you mind if I looked at this?
Comment 20•20 years ago
|
||
(In reply to comment #19)
> Ludo, would you mind if I looked at this?
Not at all
Comment 22•20 years ago
|
||
Assuming we use the gecko inline editor work, depends on bug 58612.
I think we'll also have to implement nsIEditorSpellCheck to wrap the cocoa spell
checker.
Depends on: inlineSpell
Comment 23•20 years ago
|
||
Hey all,
This is a beta patch. I just wanted to invite anyone watching to give their
thoughts on it.
Things NOT implemented are:
1) spellcheck as you type. It just beeps - I know.
2) underlining or coloring misspelled words. This kinda goes with #1 - doesn't
matter too much by itself.
Note on the patch: the way I currently implement the Ignore feature is to have
all text fields on a page share an ignore list. Any fields in another window or
tab have a separate ignore list. The ignore list is thrown away whenever the
location in a tab changes.
Attachment #112158 -
Attachment is obsolete: true
Comment 24•20 years ago
|
||
Comment 25•20 years ago
|
||
Julian I was quickly looking at the nib you attached and I was wondering. In OS
X text area contextmenu the menu looks as follows:
Guesses Section
------
Ignore Spelling
Learn Spelling
------
Everything beneath these options are in your nib file I was just wondering if
you also had these opptions as they are pretty necesarry.
Comment 26•20 years ago
|
||
...
Comment 27•20 years ago
|
||
I found a way that Mozilla uses to mark things as misspelled. This shows what
it looks like. It's a solid underline but I think it looks fine.
Comment 28•20 years ago
|
||
I'd love to pick this up again. But, 58612 hasn'gt been touched for awhile.
Maybe I should apologize on bug 58612 for my rude comment, and then gently ask
them to pick it up again :)
Can we get someone with some clout :-) to "lean on" bug 58612 so that it gets
fixed in the Gecko 1.9 cycle so that we can actually get spell-checking by the
time Camino 1.1 rolls around off the trunk again...?
Component: General → OS Integration
Comment 30•19 years ago
|
||
For all that is sacred in this world, can someone...anyone, please work on this?
The masses are awaiting spelcheking in Camino.
Comment 31•19 years ago
|
||
(In reply to comment #30)
> For all that is sacred in this world, can someone...anyone, please work on this?
> The masses are awaiting spelcheking in Camino.
This bug is dependent on a core bug. Until that core bug is fixed, there won't
be spell checking in Camino. If you want this to be fixed, go vote for bug 58612.
Comment 32•19 years ago
|
||
(In reply to comment #31)
> (In reply to comment #30)
> > For all that is sacred in this world, can someone...anyone, please work on this?
> > The masses are awaiting spelcheking in Camino.
>
> This bug is dependent on a core bug. Until that core bug is fixed, there won't
> be spell checking in Camino. If you want this to be fixed, go vote for bug 58612.
I would expect Firefox spellchecker to depend on bug 58612, as it is crossplatform. But why should
Camino's ?
Bug 58612 sounds like a complete implementation. (With discussion over how to render the erroneous
words and what event should trigger spell checking.)
For Camino, we should aim at getting as close as possible to how Safari works, that is rely on Mac OS X
spell checker. (Same dictionary, same dialog box for spell check settings, same rendering of erroneous
words, etc.) (As was initially requested for that bug.)
This is certainly possible to reuse in a web browser if Safari team got it right. Isn't it the whole idea
behind Camino: leverage Mac OS X functionalities to better fit into the mac user experience ?
Comment 33•19 years ago
|
||
I wuold be working on this wholeheartedly, as I think it is possible to do it
right without requiring the Core bug, but I am extremely distracted since at
this moment I do not have a place to live starting Sep 23.
Comment 34•19 years ago
|
||
Because this needs changes to core which won't get fixed until Gecko 1.9 (see bug 58612), targeting for Camino 2.0.
Priority: P5 → P2
Target Milestone: Camino1.1 → Camino2.0
Assignee | ||
Comment 35•19 years ago
|
||
here's what we need to do now that the spellcheck code has landed on the trunk (from brettw):
1. Enable the spellcheck extension and make sure it gets distributed.
Also, be sure you have the dictionaries you want. They go in
components/myspell.
2. Set layout.textarea.spellcheckDefault = true. This will make
textareas get spellchecking by default.
3. Check that this works. There will be no UI, but you should see the
red underlining if everything is set up properly.
4. Add the context menu stuff. See my patch for browser.js on the bug.
You can call my shared JS file for most of the tasks. You may want to
wait on this part for a few weeks: my shared JS UI file might get
converted into a component or something else.
Updated•19 years ago
|
Target Milestone: Camino2.0 → Camino1.1
Comment 36•19 years ago
|
||
(In reply to comment #35)
> here's what we need to do now that the spellcheck code has landed on the trunk
> (from brettw):
>
> 1. Enable the spellcheck extension and make sure it gets distributed.
> Also, be sure you have the dictionaries you want. They go in
> components/myspell.
Shouldn't we use the OS X dictionaries? Any idea how we could do that?
Comment 37•19 years ago
|
||
Yes, we should. Is there a dictionary API that we can implement?
Depends on: 302050
(In reply to comment #37)
> Yes, we should. Is there a dictionary API that we can implement?
Assuming this needs to be done in Core(?), this looks like it might be the right API:
http://developer.apple.com/documentation/Carbon/Reference/Dictionary_Manager/index.html
Comment 39•19 years ago
|
||
Comment on attachment 172534 [details] [diff] [review]
beta patch
rumor has it someone's working on this for real, again ;)
Attachment #172534 -
Attachment is obsolete: true
Updated•19 years ago
|
Attachment #172535 -
Attachment is obsolete: true
Comment 40•19 years ago
|
||
Comment 41•19 years ago
|
||
Comment 42•19 years ago
|
||
I tried to add some new files into the patch using /dev/null, I hope it works with 'patch' =)
---------
Changes in order to build:
Add to mozconfig: --enable-extensions=default,spellcheck
To the pbxproj:
Add src/embedding/CHEditorSpellCheck*
In the Targets tab, Add spellcheck.xpt to Copy Files (64) (I think it's the first Copy Files)
In the Targets tab, Add libspellchecker.dylib to the next Copy Files
Add to HEADER_SEARCH_PATHS: ../dist/include/editor ../dist/include/txtsvc
My changes introduced some build error which I had to work around, this is what it looks like:
/mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm: In function `unsigned int -[CHBrowserView draggingEntered:](CHBrowserView*, objc_selector*, objc_object*)':
/mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm:1528: warning: invalid conversion from `int' to `OpaqueDragRef*'
Comment 43•19 years ago
|
||
Comment on attachment 208498 [details] [diff] [review]
changes to core
Hi Boris, I noticed you reviewed the core patch for bug 302050. Would you mind taking a look at this one?
Attachment #208498 -
Flags: review?(bzbarsky)
Comment 44•19 years ago
|
||
Comment on attachment 208498 [details] [diff] [review]
changes to core
For future reference, this would be a _lot_ easier to review if you use better diff options. Something like diff -up8 or so. -p puts in the function names, -8 gives more context.
>Index: extensions/spellcheck/src/mozEnglishWordUtils.cpp
>Index: extensions/spellcheck/src/mozInlineSpellChecker.h
I can't review this part; please get someone familiar with spell-checker to do so.
>Index: layout/forms/nsTextControlFrame.cpp
> nsTextControlFrame::Destroy(nsPresContext* aPresContext)
> {
>+ nsContentUtils::UnregisterPrefCallback(PREF_DEFAULT_SPELLCHECK,
>+ nsTextControlFrame::RealTimeSpellCallback, this);
This is mis-indented.
>@@ -1705,7 +1707,12 @@
>+ // we working on Camino decided to allow inline spellcheck for single line fields
>+#ifdef MOZ_MACBROWSER
That's fine but NOT with ifdefs. Add a pref to control it, and set it in your config or something.
>nsTextControlFrame::RealTimeSpellCallback(const char* pref, void* context)
>+{
This should assert that |pref| is what you expect. Also, aPref, aContext for the args.
>+ nsTextControlFrame* frame = NS_REINTERPRET_CAST(nsTextControlFrame*, context);
NS_STATIC_CAST, please.
>+ if (frame)
Under what conditions would |frame| be null? If it can't be, assert instead of checking.
>+ return NS_OK;
This function doesn't return an nsresult. It returns an int. I suggest returning 0.
>+
>+
Why so much space? One blank line between functions should be about right.
>Index: layout/forms/nsTextControlFrame.h
>+ static NS_HIDDEN_(int) PR_CALLBACK RealTimeSpellCallback(const char* pref, void* closure);
I'm not sure NS_HIDDEN is a good idea here. I'd take it out.
Attachment #208498 -
Flags: review?(bzbarsky) → review-
Updated•19 years ago
|
Whiteboard: ok heresa biug bad test :P
Comment 46•19 years ago
|
||
I guess I'll contact bryner about the spellcheck part
Attachment #208498 -
Attachment is obsolete: true
Attachment #209190 -
Flags: review?(bzbarsky)
Comment 47•19 years ago
|
||
I suggest contacting a spellcheck owner/peer (like mscott) for the spellcheck part, actually.
Comment 48•19 years ago
|
||
Comment on attachment 209190 [details] [diff] [review]
changes to core v2
>Index: layout/forms/nsTextControlFrame.cpp
> #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
>+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine"
Wouldn't it make more sense to call the new pref "layout.textInput.spellcheckDefault"? Compare with the "layout.textarea.spellcheckDefault" pref? And perhaps change things such that it's possible to spell-check single-line inputs even if you're not spell-checking textareas?
That is, depending on whether this is a single or multi-line control register for different prefs and so forth?
Comment 49•19 years ago
|
||
(In reply to comment #48)
> (From update of attachment 209190 [details] [diff] [review] [edit])
> >Index: layout/forms/nsTextControlFrame.cpp
> > #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
> >+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine"
>
> Wouldn't it make more sense to call the new pref
> "layout.textInput.spellcheckDefault"? Compare with the
> "layout.textarea.spellcheckDefault" pref? And perhaps change things such that
> it's possible to spell-check single-line inputs even if you're not
> spell-checking textareas?
That's exactly the point: the pref doesn't single handedly turn on or off spellchecking for text inputs; it turns it on or off *given that* spellchecking is on. It treats the already existing pref as a master pref. I saw no use in making it possible to enable realtime spell on JUST single-line inputs.
>
> That is, depending on whether this is a single or multi-line control register
> for different prefs and so forth?
>
I'm sorry; what?
Comment 50•19 years ago
|
||
> That's exactly the point: the pref doesn't single handedly turn on or off
> spellchecking for text inputs; it turns it on or off *given that* spellchecking
> is on.
Right. This is the part I don't understand. Why? That is, why apply a pref that controls spellchecking for textareas to text inputs and then have _another_ pref to decide whether it applies? Why not just apply one pref to textareas and another to text inputs?
As things stand, to make your patch correct you would need to register an observer for both prefs. And then you still have the conceptual weirdness of a textarea pref affecting the behavior of text inputs...
Comment 51•19 years ago
|
||
For what its worth, I originally was planning on having one pref with values 0 (off) 1 (on for textareas) 2 (on for everything). But then we moved the pref to be called *.textarea.* which makes this make less sense.
There's no reason I can think of why you would want to have spellchecking on by default for single-line, but off by default for multi-line. But having two prefs control three states as Julian did seems a little strange to me. Would it make people happier to rename it to "layout.spellcheckDefault" and have three possible values?
Comment 52•19 years ago
|
||
Comment on attachment 209190 [details] [diff] [review]
changes to core v2
Hi,
Would you want to lend a hand and review the changes to extensions/spellcheck that are in this patch?
Attachment #209190 -
Flags: review?(mscott)
Comment 54•19 years ago
|
||
Yeah, making it a tri-state would also work for me. We didn't ship that pref in Firefox 1.5, did we?
Comment 55•19 years ago
|
||
Comment on attachment 209190 [details] [diff] [review]
changes to core v2
Is this include really necessary?
+#include "mozISpellI18NUtil.h"
I don't see where you've added any new code to the spellchecker that would depend on that header file.
I'm not comfortable reviewing the layout changes. I'll leave that to bz and others.
Attachment #209190 -
Flags: review?(mscott) → review+
Comment 56•19 years ago
|
||
(In reply to comment #55)
> (From update of attachment 209190 [details] [diff] [review] [edit])
> Is this include really necessary?
>
> +#include "mozISpellI18NUtil.h"
>
> I don't see where you've added any new code to the spellchecker that would
> depend on that header file.
The containing header file, mozInlineSpellChecker.h, has build errors when I build on my Mac, related to the use of mozISpellI18NUtil. Probably there is a difference in the include tree between platforms.
Comment 57•19 years ago
|
||
Hi Boris, I hope this is the final core patch
Attachment #209805 -
Flags: review?(bzbarsky)
Updated•19 years ago
|
Attachment #209190 -
Flags: review?(bzbarsky)
Comment 58•19 years ago
|
||
Some more info about building in Camino, I see I added some additional header search paths:
../editor/composer/src
../extensions/spellcheck/idl/_xpidlgen
../extensions/spellcheck/src
I guess these should be added to dist like the other items. I'll leave that for the sr to consider that...
Comment 59•19 years ago
|
||
Comment on attachment 209805 [details] [diff] [review]
changes to core v3
>Index: layout/forms/nsTextControlFrame.cpp
>@@ -130,13 +130,13 @@
>-#define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
>+#define PREF_DEFAULT_SPELLCHECK "layout.spellcheckDefault"
You need to change places that use the old pref value (eg firefox.js). I don't see changes to those in this patch. Note that you need to change not only the name but the value in that file.
>@@ -1702,20 +1704,38 @@ nsTextControlFrame::SyncRealTimeSpell()
>+ // check the pref to see what the default should be, default is 0: never spellcheck
>+ PRInt32 spellcheckLevel = nsContentUtils::GetIntPref(PREF_DEFAULT_SPELLCHECK, 0);
>+ if (!readOnly) {
Why not get the pref once we have !readonly? Seems like it would be more efficient in that case, and identical in the other cases.
>+ enable = true;
PR_TRUE.
r=bzbarsky with those three issues fixed. Thanks!
Attachment #209805 -
Flags: review?(bzbarsky) → review+
Comment 60•19 years ago
|
||
Hi Brian, wanna sr this? I think you're the man
Attachment #209190 -
Attachment is obsolete: true
Attachment #209805 -
Attachment is obsolete: true
Attachment #210068 -
Flags: superreview?
Updated•19 years ago
|
Attachment #210068 -
Flags: superreview? → superreview?(bryner)
Updated•19 years ago
|
Attachment #210068 -
Flags: superreview?(bryner) → superreview+
Comment 61•19 years ago
|
||
be sure to use both v3 patches
Attachment #209252 -
Attachment is obsolete: true
Assignee | ||
Comment 62•19 years ago
|
||
--- src/browser/GoMenu.mm 29 Aug 2005 17:55:04 -0000 1.18
+++ src/browser/GoMenu.mm 30 Jan 2006 02:24:13 -0000
@@ -58,12 +58,13 @@
#import "HistoryItem.h"
#import "HistoryDataSource.h"
#import "GoMenu.h"
+#include <Carbon/Carbon.h>
is this necessary?
+// Spellchecking
+- (IBAction)chCheckSpelling:(id)sender;
+- (IBAction)chShowGuessPanel:(id)sender;
+- (IBAction)chToggleContinuousSpellChecking:(id)sender;
why the |ch| prefix?
+#define CH_ENSURE_SUCCESS(res) \
+ if (!NS_SUCCEEDED(res)) return;
isn't this already a gecko macro? why create our own?
+ else {
+ [self closeSpellDocument];
+ mSpellDocumentTag = [NSSpellChecker uniqueSpellDocumentTag];
+ }
nit. fix indents. 2 space.
+ nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]);
+ nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow);
+ if (!privateWindow)
+ return NULL;
again fix indents. several times in this routine.
+ nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem);
+ nsITextControlElement *returnElem = textElem;
+ return returnElem;
this is generally a bad pattern, we should avoid returning un-addref'd interfaces from functions.
+ // NOTE |GetCurrentDoc| doesn't AddRef?!
+ nsIDocument* doc = content->GetCurrentDoc();
+ if (!doc)
+ return;
+ nsIPresShell* presShell = doc->GetShellAt(0);
+ if (!presShell)
+ return;
are you sure about these not addRef'ing?
+ if (spellChecker == nsnull) {
use |if (!spellChecker) {| instead
+ CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);
that seems really skanky. why can we make this assumption? Why not add another platform-specific interface?
+// Imitate the spellcheck actions for Cocoa text fields.
+// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch
+// so that Cocoa text fields such as the location bar don't respond to the spellcheck items
this comment should be in the header file.
+// The only difference between "Check Spelling" and "SpellingÉ" is that
use "..." not the ellipsis char, so it's readable across platforms.
+ nsCOMPtr<nsITextControlElement> activeElement = [self focusedTextControlElement];
why is this wrapped in a nsCOMPtr if you didn't addref before the return? be consistent.
+ if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) {
+ NSBeep();
+ NSLog(@"Selection changed");
+ return;
+ }
who's going to see this log message? Why bother printing it?
+ int newState = [rtSpell state] == NSOnState ? 2 : 0;
aren't there constants for these states?
+- (int)spellDocumentTag
+{
+ return mSpellDocumentTag;
+}
+
does this really need to be part of the public api? should it be a private category?
--- src/history/HistoryOutlineViewDelegate.mm 15 Jan 2006 21:03:32 -0000 1.10
+++ src/history/HistoryOutlineViewDelegate.mm 30 Jan 2006 02:24:15 -0000
@@ -47,12 +47,14 @@
#import "BrowserWindowController.h"
#import "BookmarkViewController.h"
#import "MainController.h"
#import "HistoryOutlineViewDelegate.h"
+#include <Carbon/Carbon.h>
+
again, not necessary
+class CHEditorSpellCheck: public nsIEditorSpellCheck {
open brace on a line by itself please
+ nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter; // weak
+ nsCOMPtr<nsITextControlFrame> mCurTextControl; // weak
+ nsCOMPtr<nsIDOMNode> mDocRoot; // weak
+ nsCOMPtr<nsIEditor> mEditor; // weak
how can these be weak, we own a strong ref to them with the nsCOMPtr!
+ static CHEditorSpellCheck* mSharedSpellCheck; // never dies after creation
prefix statics with |s|, eg |sSharedSpellCheck|
+CHEditorSpellCheck::CHEditorSpellCheck()
+: mCurGuesses(nil),
+mCurGuessIndex(-1),
+mSpellDocTag(-1)
+{
+}
isn't there an init call taht needs to be made here to init the refcount?
+NS_IMETHODIMP
+CHEditorSpellCheck::CanSpellCheck(PRBool* canSpellCheck)
+{
+ *canSpellCheck = PR_TRUE;
+ return NS_OK;
+}
you should validate that |canSpellCheck| isn't null first.
+ PRInt32 begin,end;
+ PRBool isMisspelled = PR_FALSE;
+ PRBool docEOF = PR_FALSE;
+ PRBool wrapped = PR_FALSE;
|begin| and |end| should be initialized here to something reasonable.
Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can use? We don't want to ship the dictionary for mac FF either.
Comment 63•19 years ago
|
||
Hi mike...
(In reply to comment #62)
> #import "GoMenu.h"
>
> +#include <Carbon/Carbon.h>
>
> is this necessary?
Yes. All the added Carbon includes are necessary to allow us to remove it from PreferenceManager.h, which I need to include for CHBrowserView. Carbon.h pulls in a symbol collision for DragRef. I only need to include PrefMgr for the new define I added. Maybe that could go in its own file.
> +#define CH_ENSURE_SUCCESS(res) \
> + if (!NS_SUCCEEDED(res)) return;
>
> isn't this already a gecko macro? why create our own?
Which is it? This question open to everyone =)
> + nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem);
> + nsITextControlElement *returnElem = textElem;
> + return returnElem;
>
> this is generally a bad pattern, we should avoid returning un-addref'd
> interfaces from functions.
I learned it from some other BrowserView code, and the core code in the next comment. Nonetheless, I'll change it by the next patch unless you revoke the comment
> + // NOTE |GetCurrentDoc| doesn't AddRef?!
> + nsIDocument* doc = content->GetCurrentDoc();
> + if (!doc)
> + return;
> + nsIPresShell* presShell = doc->GetShellAt(0);
> + if (!presShell)
> + return;
>
> are you sure about these not addRef'ing?
Yup, I followed all the core code that calls these.
> + CHEditorSpellCheck* edSpellChecker =
> NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);
>
> that seems really skanky. why can we make this assumption? Why not add another
> platform-specific interface?
It's safe because we know that CHEditorSpellCheck is the implementor of nsIEditorSpellCheck according to AppComponents. I'm not sure about platform-specific interfaces - feel free to tell me more offline (well, online, but not here).
> + nsCOMPtr<nsITextControlElement> activeElement = [self
> focusedTextControlElement];
>
> why is this wrapped in a nsCOMPtr if you didn't addref before the return? be
> consistent.
I need to hop the interfaces with QI. I suppose that's a good reason to make |focusedTextControlElement| return by ref? =)
>
> + int newState = [rtSpell state] == NSOnState ? 2 : 0;
>
> aren't there constants for these states?
Yeah, in nsTextControlFrame.h (I created them). If we include that, we can use the constants. That file includes a *slew* of other files, though.
> +- (int)spellDocumentTag
> +{
> + return mSpellDocumentTag;
> +}
> +
>
> does this really need to be part of the public api? should it be a private
> category?
Except that, when doing realtime spellcheck, the CHEditorSpellCheck gets invoked without the BrowserView. But, the BrowserView owns the spellcheck tag. I couldn't come up with any other way of syncing the BrowserView to the CHEdSpellCheck.
> + nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter; // weak
> + nsCOMPtr<nsITextControlFrame> mCurTextControl; // weak
> + nsCOMPtr<nsIDOMNode> mDocRoot; // weak
> + nsCOMPtr<nsIEditor> mEditor; // weak
>
> how can these be weak, we own a strong ref to them with the nsCOMPtr!
I'm kinda asking if they should be weak. Should I hold on to them?
> +CHEditorSpellCheck::CHEditorSpellCheck()
> +: mCurGuesses(nil),
> +mCurGuessIndex(-1),
> +mSpellDocTag(-1)
> +{
> +}
>
> isn't there an init call taht needs to be made here to init the refcount?
In our constructor? Wouldn't that be in one of the base classes? Do you mean the NS_IMPL_ISUPPORTS stuff? I changed the file to use that instead of the separate macros.
> + PRInt32 begin,end;
> + PRBool isMisspelled = PR_FALSE;
> + PRBool docEOF = PR_FALSE;
> + PRBool wrapped = PR_FALSE;
>
> |begin| and |end| should be initialized here to something reasonable.
Really? The core code doesn't. I'm all in favor of precedence...
> Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can
> use? We don't want to ship the dictionary for mac FF either.
Tell me more about standalone components.
Status: NEW → ASSIGNED
Comment 64•19 years ago
|
||
What do we do to get someone to land the core patch?
Comment 65•19 years ago
|
||
(In reply to comment #64)
> What do we do to get someone to land the core patch?
>
If it has r and sr then ask someone to land it for you.
Comment 66•19 years ago
|
||
Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)
I checked this in on trunk.
For future reference, it'd be good to put core patches in core bugs...
Attachment #210068 -
Attachment description: changes to core v3.1 → changes to core v3.1 (checked in)
Comment 67•19 years ago
|
||
Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)
We need this on the branch. bz: Are you the right person to ask for this stuff?
Also, the changes to the spellchecker seemed to be for Camino. Does this affect Firefox on the Mac, or are we still planning on using myspell there too?
Attachment #210068 -
Flags: branch-1.8.1?(bzbarsky)
Comment 68•19 years ago
|
||
Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)
Looks ok, assuming bug 302050 is going on branch
Attachment #210068 -
Flags: branch-1.8.1?(bzbarsky) → branch-1.8.1+
Comment 69•19 years ago
|
||
> Also, the changes to the spellchecker seemed to be for Camino. Does this affect
> Firefox on the Mac, or are we still planning on using myspell there too?
>
Didn't the patch for bug 302050 include mozEnglishWordUtils? I don't know if that code is live or dead, but it's possible that Firefox is using that code for spellcheck. My change to that file would fix an issue that I assume would occur for any build that has realtime spell enabled, although I only know for sure that it was an issue in Camino. Also, if Firefox gets a UI for the realtime pref, which I don't think it has currently, then the changes to nsTextControlFrame will ensure that the pref is updated live.
Comment 70•19 years ago
|
||
whom should I flag for review?
Attachment #210100 -
Attachment is obsolete: true
Comment 71•19 years ago
|
||
patch notes: I needed to leave the change for the Carbon/Carbon.h includes because I needed to include PrefMgr.h for CHBrowserView after all. Everything else I said I'd do in comment 63 I did.
Comment 72•19 years ago
|
||
Comment on attachment 211692 [details] [diff] [review]
changes to camino v3.1
> Index: src/application/MainController.mm
> ===================================================================
> + // sync realtime spellcheck setting
> + // default is zero - off. 2 is on.
> + int enableRealTimeSpellCheck = [prefsManager getIntPref:kRealTimeSpellPref withSuccess:&success];
> + if (!success || !enableRealTimeSpellCheck) {
> + [mRealTimeSpellMenuItem setState:NSOffState];
> + }
> + else {
> + [mRealTimeSpellMenuItem setState:NSOnState];
> + }
Does the menu this item is in have auto-updating turned off?
Otherwise this won't stick.
> Index: src/browser/GoMenu.mm
> ===================================================================
> RCS file: /cvsroot/mozilla/camino/src/browser/GoMenu.mm,v
> retrieving revision 1.18
> diff -u -6 -p -r1.18 GoMenu.mm
> --- src/browser/GoMenu.mm 29 Aug 2005 17:55:04 -0000 1.18
> +++ src/browser/GoMenu.mm 13 Feb 2006 06:44:52 -0000
> @@ -58,12 +58,13 @@
>
> #import "HistoryItem.h"
> #import "HistoryDataSource.h"
>
> #import "GoMenu.h"
>
> +#include <Carbon/Carbon.h>
Put framework includes at the top, please.
> Index: src/embedding/CHBrowserView.h
> ===================================================================
> +// Spellchecking
> +// Imitate the spellcheck actions for Cocoa text fields.
> +// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch
> +// so that Cocoa text fields such as the location bar don't respond to the spellcheck items
> +- (IBAction)chCheckSpelling:(id)sender;
> +- (IBAction)chShowGuessPanel:(id)sender;
> +- (IBAction)chToggleContinuousSpellChecking:(id)sender;
> +
> +- (void)checkSpelling:(id)sender;
> +- (void)changeSpelling:(id)sender;
> +- (void)ignoreSpelling:(id)sender;
> +- (void)closeSpellDocument;
> +- (int)spellDocumentTag;
Spell checking adds a lot of code to CHBrowserView. Perhaps you could
move it into a separate file which contains a category of CHBrowserView.
It also adds some bad dependencies. Perhaps we should hide it all behind
a protocol, and move the implementation up into non-embedding Camino code.
> Index: src/embedding/CHBrowserView.mm
> ===================================================================
> +// Spellcheck integration
> +#include "nsITextControlElement.h"
> +#include "nsIFocusController.h"
> +#include "nsIContent.h"
> +#include "nsIFrame.h"
> +#include "nsITextControlFrame.h"
> +#include "nsIInlineSpellChecker.h"
> +#include "nsIEditor.h"
> +#include "nsIEditorSpellCheck.h"
> +#include "nsISelection.h"
> +#include "nsIPlaintextEditor.h"
This strengthens my conviction. CHBrowserView is supposed to be a "model"
gecko embedder, and most of these headers are not frozen and should not
be used by embedders. It would be nice to move this all out.
> // security
> #include "nsISecureBrowserUI.h"
> #include "nsISSLStatusProvider.h"
> #include "nsISSLStatus.h"
> #include "nsIX509Cert.h"
>
> #import "CHBrowserView.h"
> +#import "PreferenceManager.h"
You're adding a dependency here of lower-level code (in embedding)
on higher-level code (in Camino). No can do.
> +- (void)spellcheckCore;
What does this mean? The method name is confusing
> +#pragma mark -
> +// Cocoa spellcheck integration
> +
> +// Get the current text control frame that has the focus, or NULL if none.
> +- (already_AddRefed<nsITextControlElement>)getFocusedTextControlElement
> +{
> + nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]);
> + nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow);
> + if (!privateWindow)
> + return nsnull;
Does this work in framesets?
> +// Search for the misspelled word starting at the selection, wrapping if necessary
> +- (void)spellcheckCore
> +{
> + nsresult rv;
Move this down to where you first use it.
> + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> + nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement);
> + if (!content)
> + return;
> + // NOTE |GetCurrentDoc| doesn't AddRef?!
Right. Raw pointer return values are not addreffed.
> + // manual spell check should use the inline spell checker's spell checker if possible.
> + // otherwise we use our own editor spellcheck, which is shared between all
> + // CHBrowserViews, and re-inited if the editor is different, i.e.
> + // a different text field is active.
So when do we fail to get the inline spell checker's spell checker? Your comment
should say why/when this happens.
> + CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);
> + if (edSpellChecker == NULL) {
> + NSLog(@"Unable to get CHEditorSpellCheck");
> + return;
> + }
This is pretty evil. Normally if you need to do something like this, you create a
private interface (nsPIFoo) and QI the object to it. How can you be sure that there
isn't some other component in core which is creating spell checker objects?
Note that going to about:plugins can get core objects instantiated that
we normally override in AppComponents. See bug 285211.
> + PRUnichar* prWord;
> + rv = spellChecker->GetNextMisspelledWord(&prWord);
> + NS_ENSURE_TRUE(rv,);
prWord is leaked. (Sucks that the they don't use nsAString).
> + // now that new range is highlighted, get the (slightly incorrect) flat range
> + // to cache for our own purpose
> + rv = textFrame->GetSelectionRange(&mSavedSelStart, &mSavedSelEnd);
> + // not much ado if that failed
Explain to the lay reader lwhen these offsets will be wrong.
> +// Correct the spelling of a misspelled word already spellchecked
> +- (void)changeSpelling:(id)sender
> +{
> + nsresult rv;
> +
> + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> + if (!activeElement) {
> + NSLog(@"Focus lost");
> + return;
> + }
Remove NSLog in final patch.
> + // Let's make sure that the user hasn't changed the selection since they checked the spelling
> + // for the misspelled word, otherwise the spellchecker just inserts the text willy-nilly
> + // this just compares the selected range at the time the spelling was checked, to
> + // the time the ignore was issued; doesn't check that the identity of the focused
> + // text frame is the same.
> + int selStart, selEnd;
> + textFrame->GetSelectionRange(&selStart, &selEnd);
> + if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) {
> + NSBeep();
Beep?
> + nsCOMPtr<nsIEditor> editor;
> + rv = textFrame->GetEditor(getter_AddRefs(editor));
> + NS_ENSURE_TRUE(rv,);
> +
> + nsAutoString newWord;
> + NSString* value = [[sender selectedCell] stringValue];
> + [value assignTo_nsAString:newWord];
> + nsCOMPtr<nsIDOMNode> node;
> + PRInt32 offset;
> + // get a node and offset for normal selection (which is the word to change)
> + nsCOMPtr<nsISelection> selection;
> + rv = editor->GetSelection(getter_AddRefs(selection));
> + NS_ENSURE_TRUE(rv,);
> + rv = selection->GetFocusNode(getter_AddRefs(node));
> + NS_ENSURE_TRUE(rv,);
> + rv = selection->GetFocusOffset(&offset);
> + NS_ENSURE_TRUE(rv,);
> +
> + nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker;
> + rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker));
> + NS_ENSURE_TRUE(rv,);
> +
> + // if realtime spellcheck is on, we pass it to the inline checker
> + // otherwise, it won't work, we have to do it ourselves
> + PRBool enabled;
> + rv = inlineSpChecker->GetEnableRealTimeSpell(&enabled);
> + if (enabled) {
> + nsAutoString word;
> + [[[sender selectedCell] stringValue] assignTo_nsAString:word];
I think [sender selectedCell] needs some explanation.
> + // pass a point in the highlighted word to the inline checker
> + rv = inlineSpChecker->ReplaceWord(node, offset, newWord);
> + }
> + else {
In this clause, 'offset' is not used, so maybe move the above code into
here.
> + // taken from nsTextServicesDocument::InsertText
> + rv = editor->BeginTransaction();
> + if (NS_FAILED(rv))
> + return;
> + nsCOMPtr<nsIPlaintextEditor> textEditor = do_QueryInterface(editor, &rv);
> + // word is already the selection
> + if (NS_SUCCEEDED(rv))
> + rv = textEditor->InsertText(newWord);
> + editor->EndTransaction();
> + }
> +}
> +
> +- (IBAction)chToggleContinuousSpellChecking:(id)sender
> +{
> + NSMenuItem* rtSpell = (NSMenuItem*)sender;
> + [rtSpell setState:([rtSpell state] == NSOnState ? NSOffState : NSOnState)];
> + int newState = [rtSpell state] == NSOnState ? 2 : 0;
> + [[PreferenceManager sharedInstance] setPref:kRealTimeSpellPref toInt:newState];
So we're saving the pref separately from Cocoa. Should we, and does the right
thing happen when native text fields (e.g. the url bar) have focus?
> +// NSIgnoreMisspelledWords
> +- (void)ignoreSpelling:(id)sender
> +{
> + nsresult rv;
> +
> + // inline spellchecker will call CHEditorSpellCheck::IgnoreWordAllOccurrences
> + // which will ignore the word with NSSpellChecker
> + // inline checker will update the highlighting for this text frame,but
> + // what about other text frames that have the word now ignored?
Did you file a bug?
> + nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> + if (!activeElement) {
> + NSLog(@"Focus changed");
> + return;
Remove NSLog.
> + }
> + nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement);
> + // NOTE |GetCurrentDoc| doesn't AddRef?!
> + nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc();
> + nsIPresShell* presShell = doc->GetShellAt(0);
> + if (!presShell)
> + return;
> +
> + nsIFrame* frame = presShell->GetPrimaryFrameFor(content);
> + nsCOMPtr<nsITextControlFrame> textFrame = do_QueryInterface(frame);
> + if (!textFrame)
> + return;
> +
> + nsCOMPtr<nsIEditor> editor;
> + rv = textFrame->GetEditor(getter_AddRefs(editor));
> + NS_ENSURE_TRUE(rv,);
> +
> + nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker;
> + rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker));
> + NS_ENSURE_TRUE(rv,);
This hunk of code appears quite a bit. Factor it?
> Index: src/history/HistoryOutlineViewDelegate.mm
> ===================================================================
> RCS file: /cvsroot/mozilla/camino/src/history/HistoryOutlineViewDelegate.mm,v
> retrieving revision 1.10
> diff -u -6 -p -r1.10 HistoryOutlineViewDelegate.mm
> --- src/history/HistoryOutlineViewDelegate.mm 15 Jan 2006 21:03:32 -0000 1.10
> +++ src/history/HistoryOutlineViewDelegate.mm 13 Feb 2006 06:44:54 -0000
> @@ -47,12 +47,14 @@
> #import "BrowserWindowController.h"
> #import "BookmarkViewController.h"
> #import "MainController.h"
>
> #import "HistoryOutlineViewDelegate.h"
>
> +#include <Carbon/Carbon.h>
> +
Put framework includes at the top, please.
> +++ src/embedding/CHEditorSpellCheck.h 2006-02-12 19:54:13.000000000 -0800
> +/*
> + * Class CHEditorSpellCheck: a replacement for the class
> + * nsEditorSpellCheck, loaded into the spellcheck module at runtime.
> + */
> +class CHEditorSpellCheck: public nsIEditorSpellCheck
> +{
> +public:
> + CHEditorSpellCheck();
> + virtual ~CHEditorSpellCheck();
> +
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIEDITORSPELLCHECK
> +
> + // get a shared spell checker that can be used (and recycled) during manual spellcheck
> + static nsresult GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck);
> + void SetSpellDocumentTag(int spellDocTag);
Why a setter and no getter?
> --- /dev/null 2006-02-12 22:53:27.000000000 -0800
> +++ src/embedding/CHEditorSpellCheck.mm 2006-02-04 01:57:23.000000000 -0800
> +// static
> +nsresult CHEditorSpellCheck::GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck)
> +{
> + if (!sSharedSpellCheck) {
> + sSharedSpellCheck = new CHEditorSpellCheck;
> + }
> + *aSpellCheck = sSharedSpellCheck;
> + return *aSpellCheck ? NS_OK : NS_ERROR_NULL_POINTER;
> +}
Someone needs to hold a reference to this object, otherwise it might
get deleted.
> +NS_IMETHODIMP
> +CHEditorSpellCheck::InitSpellChecker(nsIEditor* editor, PRBool enableSelectionChecking)
> +{
> + nsresult rv;
> +
> + // we try to reuse the same EditorSpellCheck if the user is only using manual check
> + if (editor == mEditor) {
> + NSLog(@"-[CHEditorSpellCheck InitSpellChecker]: editor the same, not reiniting");
> + return NS_OK;
> + }
Remove NSLog
> +// spell check with wrapping
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetNextMisspelledWord(PRUnichar** nextMisspelledWord)
> +{
> + // It would be nice to get the flat string of the text frame and pass the
> + // whole thing to NSSpellChecker. However,
> + // we wouldn't know the block index and offset of the misspelled word in
> + // the TSD.
> + nsresult rv = EnsureConverter();
> + NS_ENSURE_SUCCESS(rv, rv);
> + if(!mWordUtil)
> + return NS_ERROR_NULL_POINTER;
> +
> + // Here we build a range that has the entire editor contents and reinit
> + // the TSD with it
> + nsCOMPtr<nsIDOMRange> docRange =
> + do_CreateInstance("@mozilla.org/content/range;1", &rv);
> + NS_ENSURE_SUCCESS(rv, rv);
> + rv = docRange->SelectNodeContents(mDocRoot);
> + NS_ENSURE_SUCCESS(rv, rv);
> + rv = mTsDoc->SetExtent(docRange);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + PRUint32 selOffset;
> + rv = SetupDoc(&selOffset);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + PRInt32 begin,end;
> + PRBool isMisspelled = PR_FALSE;
> + PRBool docEOF = PR_FALSE;
> + PRBool wrapped = PR_FALSE;
> + nsCOMPtr<nsIDOMRange> wrapExtent;
> + // get a range of the current cursor position which we might use for wrapping
> + rv = mTsDoc->GetDOMRangeFor(selOffset, 0, getter_AddRefs(wrapExtent));
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + while (!isMisspelled && (!wrapped || !docEOF))
> + {
> + nsString str;
> + rv = mTsDoc->GetCurrentTextBlock(&str);
> +
> + if (NS_FAILED(rv))
> + return rv;
Remove blank line before, add one after.
> + do {
> + rv = mWordUtil->FindNextWord(str.get(),str.Length(),selOffset,&begin,&end);
Spaces after commas please.
> + if(NS_SUCCEEDED(rv)&&(begin != -1)){
Space after if.
> + const nsAString &currWord = Substring(str, begin, end - begin);
> + rv = CheckWordInternal(currWord, &isMisspelled, YES);
> + NS_ENSURE_SUCCESS(rv, rv);
> + if(isMisspelled){
> + *nextMisspelledWord = ToNewUnicode(currWord);
> + rv = mTsDoc->SetSelection(begin, end-begin);
> + rv = mTsDoc->ScrollSelectionIntoView();
> + break;
> + }
> + }
> + if (end != -1)
> + selOffset = end;
> + } while (end != -1);
> +
> + // wrapping stuff here: we've reached the end, gotta wrap
> + // lazily set the start of the wrap range and set the TSD extent
> + if (NS_SUCCEEDED(mTsDoc->IsDone(&docEOF)) && docEOF && !wrapped) {
> + rv = wrapExtent->SetStart(mDocRoot, 0);
> + NS_ENSURE_SUCCESS(rv, rv);
> + rv = mTsDoc->SetExtent(wrapExtent);
> + NS_ENSURE_SUCCESS(rv, rv);
> + rv = mTsDoc->FirstBlock();
> + NS_ENSURE_SUCCESS(rv, rv);
> + wrapped = PR_TRUE;
> + docEOF = PR_FALSE;
> + }
> + else
> + mTsDoc->NextBlock();
> +
> + selOffset=0;
> + }
> +
> + if (!isMisspelled)
> + *nextMisspelledWord = ToNewUnicode(NS_LITERAL_STRING(""));
Can't just return nsnull?
> + return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetSuggestedWord(PRUnichar** suggestedWord)
> +{
> + nsAutoString suggestion;
> + if (mCurGuessIndex < [mCurGuesses count]) {
> + [[mCurGuesses objectAtIndex:mCurGuessIndex++] assignTo_nsAString:suggestion];
> + }
What if mCurGuessIndex is still -1 ?
> + *suggestedWord = ToNewUnicode(suggestion);
> + return NS_OK;
> +}
> +NS_IMETHODIMP
> +CHEditorSpellCheck::CheckWordInternal(const nsAString& word, PRBool* isMisspelled,
> + BOOL computeSuggestions)
> +{
> + // This is the case at the start of inline spellcheck
> + if (mSpellDocTag == -1) {
> + nsCOMPtr<nsIDOMDocument> domDoc;
> + nsresult rv = mEditor->GetDocument(getter_AddRefs(domDoc));
> + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> + nsCOMPtr<nsISupports> container = doc->GetContainer();
> + nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
> +
> + CHBrowserView* browserView = [CHBrowserView browserViewFromDOMWindow:window];
> + if (!browserView) {
> + NSLog(@"Failed to get current browser view");
> + return NS_ERROR_FAILURE;
> + }
> + mSpellDocTag = [browserView spellDocumentTag];
> + }
This is pretty weird, that we reach over to get a CHBrowserView, just
to getthe spell check doc tag. Is there a better way?
> + NSSpellChecker* spChecker = [NSSpellChecker sharedSpellChecker];
> + NSString* cocoaWord = [NSString stringWith_nsAString:word];
> + NSRange spRange = [spChecker checkSpellingOfString:cocoaWord
> + startingAt:0
> + language:nil
> + wrap:NO
> + inSpellDocumentWithTag:mSpellDocTag
> + wordCount:NULL];
> +
> + *isMisspelled = (spRange.length != 0);
> +
> + if (computeSuggestions) {
> + [mCurGuesses release];
> + mCurGuesses = nil;
> + mCurGuessIndex = -1;
> + if (*isMisspelled) {
> + mCurGuesses = [[spChecker guessesForWord:cocoaWord] retain];
> + mCurGuessIndex = 0;
> + }
> + }
> +
> + return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::CheckCurrentWordNoSuggest(const PRUnichar* prWord,
> + PRBool* isMisspelled)
> +{
> + nsresult rv = CheckWordInternal(nsDependentString(prWord), isMisspelled, NO);
> + return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::ReplaceWord(const PRUnichar* misspelledWord,
> + const PRUnichar* replaceWord,
> + PRBool allOccurrences)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +// this method IS used
And the others aren't?
> +NS_IMETHODIMP
> +CHEditorSpellCheck::IgnoreWordAllOccurrences(const PRUnichar* prWord)
> +{
> + if (mSpellDocTag < 0)
> + NSLog(@"Whoa, IgnoreWordAllOccurrences: spell tag not set");
> + NSString* word = [NSString stringWith_nsAString:nsDependentString(prWord)];
> + [[NSSpellChecker sharedSpellChecker] ignoreWord:word inSpellDocumentWithTag:mSpellDocTag];
> + return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetPersonalDictionary()
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetPersonalDictionaryWord(PRUnichar **word)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::AddWordToDictionary(const PRUnichar *word)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::RemoveWordFromDictionary(const PRUnichar *word)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetDictionaryList(PRUnichar ***dictionaryList, PRUint32 *count)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetCurrentDictionary(PRUnichar** dictionary)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetCurrentDictionary(const PRUnichar* dictionary)
> +{
> + return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::UninitSpellChecker()
> +{
> + return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetFilter(nsITextServicesFilter* filter)
> +{
> + mTxtSrvFilter = filter;
> + return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetupDoc(PRUint32 *outBlockOffset)
> +{
> + nsresult rv;
> +
> + nsITextServicesDocument::TSDBlockSelectionStatus blockStatus;
> + PRInt32 selOffset;
> + PRInt32 selLength;
> +
> + rv = mTsDoc->LastSelectedBlock(&blockStatus, &selOffset, &selLength);
> + if (NS_SUCCEEDED(rv) && (blockStatus != nsITextServicesDocument::eBlockNotFound))
> + {
> + switch (blockStatus)
> + {
> + case nsITextServicesDocument::eBlockOutside: // No TB in S, but found one before/after S.
> + case nsITextServicesDocument::eBlockPartial: // S begins or ends in TB but extends outside of TB.
> + // the TS doc points to the block we want.
> + *outBlockOffset = selOffset + selLength;
> + break;
> +
> + case nsITextServicesDocument::eBlockInside: // S extends beyond the start and end of TB.
> + // we want the block after this one.
> + rv = mTsDoc->NextBlock();
> + *outBlockOffset = 0;
> + break;
> +
> + case nsITextServicesDocument::eBlockContains: // TB contains entire S.
> + *outBlockOffset = selOffset + selLength;
> + break;
> +
> + case nsITextServicesDocument::eBlockNotFound: // There is no text block (TB) in or before the selection (S).
> + default:
> + NS_NOTREACHED("Shouldn't ever get this status");
> + }
> + }
> + else //failed to get last sel block. Just start at beginning
> + {
> + rv = mTsDoc->FirstBlock();
> + *outBlockOffset = 0;
> + }
> +
> + return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::EnsureConverter()
> +{
> + nsresult res = NS_OK;
> + if (!mWordUtil)
> + {
> + nsCOMPtr<mozISpellI18NManager> manager(do_GetService("@mozilla.org/spellchecker/i18nmanager;1", &res));
> + if (manager && NS_SUCCEEDED(res))
> + {
> + nsXPIDLString language;
> + res = manager->GetUtil(language.get(), getter_AddRefs(mWordUtil));
> + }
> + }
> + return res;
> +}
General comments:
I find it a little weird that both CHBrowserView and CHEditorSpellCheck are talking to the Cocoa spell checker.
Can we push all the interaction with NSSpellChecker into one class?
I'm concerned about adding all that code to CHBrowserView, especially since it sucks in so many non-embedding
headers. And, we have to avoid including higher-level headers in lower-level files.
I'd also be interested to hear about how the inline spell check "feels" compared to Cocoa's. Does it behave exactly
the same way, or does it feel odd and clunky?
Attachment #211692 -
Flags: superreview-
Assignee | ||
Comment 73•19 years ago
|
||
I was going to work on refactoring this so that it could be used by firefox as well and get it landed. Brett said the core stuff should be on the 18branch by the end of this week.
I'll start with the patch julian posted and clean that up and report back.
Assignee | ||
Comment 74•19 years ago
|
||
julian, what was the reasoning behind replacing nsEditorSpellCheck rather than reimplementing just the code behind nsISpellChecker? Is it because you needed all that additional contextual info?
I'm just worried about copying all that code, it's more to maintain and get out of sync.
Comment 75•19 years ago
|
||
(In reply to comment #74)
> julian, what was the reasoning behind replacing nsEditorSpellCheck rather than
> reimplementing just the code behind nsISpellChecker? Is it because you needed
> all that additional contextual info?
>
> I'm just worried about copying all that code, it's more to maintain and get out
> of sync.
>
Hoo boy, let's see. I think I was primarily concerned about all the dictionary stuff in the nsEditorSpellCheck. The CanSpellCheck method requires at least one dictionary, and I was kind of confused about how to handle it. However, I can't think of anything really compelling right now. I looked at the code and I'm not sure what contextual info I gained. I'll let you know if I think of anything else.
Comment 76•19 years ago
|
||
The latest comment on bug 302050 seems to indicate that the core patches from this bug should be going in to gecko 1.8.1. Is this the case?
Updated•19 years ago
|
Flags: blocking1.8.1?
Comment 77•19 years ago
|
||
Blocker as dep for blocker, and general goodness.
Flags: blocking1.8.1? → blocking1.8.1+
Assignee | ||
Comment 78•19 years ago
|
||
i have a re-implementation of the mozISpellCheckingEngine on top of NSSpellChecker. Much smaller patch, I'll include it a bit later once I get some of the context menu stuff hooked up for camino to make sure that part works as well.
Summary: Add access to Mac OS X global spelling checker → Add access to Mac OS X global spelling checkers
Assignee | ||
Comment 79•19 years ago
|
||
brett: why does the mySpell spellchecker's makefile specifically disable static linkage? Any ideas? I can make it initially just a dylib, but at some point we'd like to make it static for Camino. Is it a license issue with mySpell, which the OSX impl wouldn't have, or is it for some other reason (like ease of disabling)?
Comment 80•19 years ago
|
||
I don't know why it's static. The license should be OK. All the spellchecking stuff is kind of separate, like an extension. I suspect it's that way just for disabling. I don't have any feeling of how it should be.
Comment 81•19 years ago
|
||
Mike, sweet. I'm eager to take a gander at that patch. =)
Assignee | ||
Comment 82•19 years ago
|
||
Here's the gecko part of the patch that replaces the mySpell engine with one based on NSSpellChecker. I'll attach the camino part separately once I get some of the dylib v. static issues dealt with.
Attachment #208497 -
Attachment is obsolete: true
Attachment #211692 -
Attachment is obsolete: true
Attachment #220399 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #220399 -
Flags: review? → review?(mark)
Assignee | ||
Comment 83•19 years ago
|
||
here's the chunk for Camino that builds the suggestion context menu. Nib and project changes will come later, they're not too complicated. Note that this will only build on the trunk, there are api changes needed for the branch, but they're small.
Attachment #220401 -
Flags: review?(mark)
Comment 84•19 years ago
|
||
Be sure you get the spelling actions into Edit->Spelling. I had code in the BroswserView to do this (didn't see any actions in the patch...) Make sure that the spellcheck wraps when you check manually; this didn't happen by default with the nsEditorSpellCheck stuff, I had to take care of that myself.
Comment 85•19 years ago
|
||
Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine
* Ideally, you'd add the NSString category somewhere global and fix bug 334670. (We need mac string utilites in the tree.)
Otherwise, please note in that bug that this category will need to be removed later.
* There's some weird tab/space inconsistencies in the patch... :-)
* Is the personal dictionary in OS X "global"? Will we need to hook up gecko's mozIPersonalDictionary to that later?
Other than those things, r=me fwiw.
Comment 86•19 years ago
|
||
Comment on attachment 220401 [details] [diff] [review]
Camino portion, sans nib and project changes (trunk)
>@@ -3362,9 +3392,20 @@
>
> const int kFrameRelatedItemsTag = 100;
> const int kFrameInapplicableItemsTag = 101;
> const int kSelectionRelatedItemsTag = 102;
>+ const int kSpellingRelatedItemsTag = 103;
>
>+ if (showSpellingItems)
>+ showSpellingItems = [self prepareSpellingSuggestionMenu:result tag:kSpellingRelatedItemsTag];
>+
>+ if (!showSpellingItems) {
>+ // word spelled correctly or not applicable, remove all traces of spelling items
>+ NSMenuItem* selectionItem;
>+ while ((selectionItem = [result itemWithTag:kSpellingRelatedItemsTag]) != nil)
>+ [result removeItem:selectionItem];
>+ }
>+
How about:
if (showSpellingItems)
...
else {
...
}
Also, some weird whitespace inconsistencies in this patch too. 2 space vs 4 space indenting, and some tabs.
>+- (BOOL)prepareSpellingSuggestionMenu:(NSMenu*)inMenu tag:(int)inTag
>+{
>+ #define ENSURE_TRUE(x) if (!x) return NO;
I'm not sure I like the temporary-#define stuff in this method and replaceMispelledWord:.
For this one, perhaps you can define it globally (with the bool return), and for the #define in replaceMispelledWord:, define ENSURE_EXISTS() globally?
...
>+ nsCOMPtr<nsISelection> selection;
>+ editor->GetSelection(getter_AddRefs(selection));
>+ ENSURE_TRUE(selection);
>+ PRInt32 anchorOffset;
>+ selection->GetAnchorOffset(&anchorOffset);
>+ nsCOMPtr<nsIDOMNode> anchorNode;
>+ selection->GetAnchorNode(getter_AddRefs(anchorNode));
This chunk (+ the fetching of the spellchecker) is also done in replaceMispelledWord:, so I think it deserves a GeckoUtils.h helper.
>+ nsCOMPtr<nsIDOMRange> mispelledRange;
>+ inlineChecker->GetMispelledWord(anchorNode, (long)anchorOffset, getter_AddRefs(mispelledRange));
Wanna move this down a bit, where the if (mispelledRange) check is?
>+ nsCOMPtr<nsIEditorSpellCheck> spellCheck;
>+ inlineChecker->GetSpellChecker(getter_AddRefs(spellCheck));
>+ ENSURE_TRUE(spellCheck);
* Is the spellchecker something that is per <editor>? If we're fetching a service, we could cache it.
r=me
Assignee | ||
Comment 87•19 years ago
|
||
How about:
if (showSpellingItems)
...
else {
...
}
Re-read the code, the point is that |showSpellingItems| can change and we need to re-test it. It's not just 1 check that can do with a single if-then-else.
Comment 88•19 years ago
|
||
(In reply to comment #87)
> How about:
>
> if (showSpellingItems)
> ...
> else {
> ...
> }
>
> Re-read the code, the point is that |showSpellingItems| can change and we need
> to re-test it. It's not just 1 check that can do with a single if-then-else.
>
Ah, now I see that, thanks.
Comment 89•19 years ago
|
||
Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine
>Index: osxspell/src/Makefile.in
>+MOZILLA_INTERNAL_API = 1
Do you need that?
>Index: osxspell/src/mozOSXSpell.mm
>+// SetPersonalDictionary
>+//
>+// Hold onto the personal dictionare we're given.
Typo
>+NS_IMETHODIMP mozOSXSpell::Check(const PRUnichar *aWord, PRBool *aResult)
>+{
>+ NS_ENSURE_ARG_POINTER(aWord);
>+ NS_ENSURE_ARG_POINTER(aResult);
>+ *aResult = PR_FALSE;
Detab
>Index: osxspell/src/mozOSXSpellFactory.cpp
>+ * The Initial Developer of the Original Code is Mike Pinkerton.
We usually put Google Inc. there.
>+ * Contributor(s): Mike Pinkerton <mikepinkerton@mac.com>
with your name on a new line? I don't care, I think you know how I feel about licensing...
Attachment #220399 -
Flags: review?(mark) → review+
Comment 90•19 years ago
|
||
*** Bug 339259 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•19 years ago
|
Attachment #220399 -
Flags: superreview?(bryner)
Comment 91•19 years ago
|
||
Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine
>+CPPSRCS = \
>+ mozOSXSpellFactory.cpp \
>+ $(NULL)
>+
>+CMMSRCS = \
>+ mozOSXSpell.mm \
>+ $(NULL)
I don't suppose it would be possible to just combine these files into one .mm file?
>--- /dev/null 1 Jan 1970 00:00:00 -0000
>+++ osxspell/src/mozOSXSpell.h 1 May 2006 16:18:07 -0000
>+class mozOSXSpell : public mozISpellCheckingEngine
>+{
>+public:
>+ NS_DECL_ISUPPORTS
>+ NS_DECL_MOZISPELLCHECKINGENGINE
>+
>+ mozOSXSpell();
>+ virtual ~mozOSXSpell();
>+
>+protected:
Do you intend to subclass this? If not, make the destructor private and nonvirtual, and change protected: to private:.
>+
>+ // NSSpellChecker provides the ability to add words to the local dictionary,
>+ // but it's much easier to let the rest of Gecko handle that via the personal
>+ // dictionary given to us and just be ignorant about new words.
Tabs.
>--- /dev/null 1 Jan 1970 00:00:00 -0000
>+++ osxspell/src/mozOSXSpellFactory.cpp 1 May 2006 16:18:07 -0000
>+static nsModuleComponentInfo components[] = {
>+ { NULL, MOZ_OSXSPELL_CID, MOZ_OSXSPELL_CONTRACTID, mozOSXSpellConstructor }
It wouldn't hurt to have a name here instead of NULL, just for ease of debugging.
Looks good otherwise.
Attachment #220399 -
Flags: superreview?(bryner) → superreview+
Updated•19 years ago
|
Blocks: SpellCheckTracking
Assignee | ||
Comment 92•19 years ago
|
||
Attachment #220399 -
Attachment is obsolete: true
Assignee | ||
Comment 93•19 years ago
|
||
trunk patch checked in there, should be available for firefox, i'll work on adding it to the camino build in the next step. I also need branch approval for that patch, as it seems to compile fine there.
Assignee | ||
Comment 94•19 years ago
|
||
Comment on attachment 224315 [details] [diff] [review]
fixes all issues raised during review, landed on trunk
requesting branch approval
Attachment #224315 -
Flags: approval-branch-1.8.1?
Updated•18 years ago
|
Attachment #224315 -
Flags: approval-branch-1.8.1? → approval-branch-1.8.1+
Assignee | ||
Comment 95•18 years ago
|
||
landed on the branch (only for camino since it can't work with carbon firefox), set the pref on both trunk and branch to enable spellcheck for camino.
i still have to land the patch that hooks up the context menu for suggestions, but that doesn't require project changes and can be done easily (i only have one mac that has XCode2.0 on it).
Comment 96•18 years ago
|
||
Could this have caused bug 341980 (Camino) and bug 341317 (Firefox/core) ?
(tested with trunk builds only)
This doesn't appear to actually *work* on the branch (at least, no little red dots are drawn), or at least not on 10.3.9. Checked a clean profile and the latest branch nightly....
Assignee | ||
Comment 98•18 years ago
|
||
i can't see how it could have caused that regression, these patches do no drawing of their own.
you're right, the branch doesn't work. we're missing spellcheck.xpt which i guess i biffed in the static target. I don't have access to an XCode2.0 machine this week in mtnView, so it will have to wait a week, unless mento can get to it.
Comment 99•18 years ago
|
||
The project file is fine, we've got spellchecker.xpt in both targets. The problem is that osxspell is in use instead of myspell, because the Makefile change wasn't made on the 1.8[.1] branch. Because myspell isn't finding its dictionaries (they were correctly not added to the project file), spell checking is disabled. This patch turns on osxspell when Cocoa widgets are in use, as on the trunk.
Attachment #226615 -
Flags: review?(mikepinkerton)
Assignee | ||
Comment 100•18 years ago
|
||
Comment on attachment 226615 [details] [diff] [review]
Enable osxspell on 1.8 branch (checked in)
wtf. i wonder how this got missed. thanks for catching it.
r/sr=pink
Attachment #226615 -
Flags: review?(mikepinkerton) → review+
Comment 101•18 years ago
|
||
Comment on attachment 226615 [details] [diff] [review]
Enable osxspell on 1.8 branch (checked in)
Checked in on MOZILLA_1_8_BRANCH before 1.8.1b1.
Attachment #226615 -
Attachment description: Enable osxspell on 1.8 branch → Enable osxspell on 1.8 branch (checked in)
Updated•18 years ago
|
No longer blocks: SpellCheckTracking
Comment 102•18 years ago
|
||
In the latest nightly (26-06-06) Camino underlines spellings when they are incorrect (which is excellent btw.) however it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs.
Comment 103•18 years ago
|
||
(In reply to comment #102)
> it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs.
Mike said in comment #95 that he _did not_ check in support for the contextual menu items, etc. etc. etc.
Comment 104•18 years ago
|
||
Is it just me, or does the spell checker(or at least the red underliner) not work in rich text boxes, such as those in GMail, Wordpress, and many forums? Those are generally the ones I need spell checking the most, and the ones that specifically don't work right in Safari(no rich text options).
(In reply to comment #104)
> Is it just me, or does the spell checker(or at least the red underliner) not
> work in rich text boxes, such as those in GMail, Wordpress, and many forums?
That's bug 333038.
Assignee | ||
Comment 106•18 years ago
|
||
Attachment #220401 -
Attachment is obsolete: true
Attachment #220401 -
Flags: review?(mark)
Assignee | ||
Comment 107•18 years ago
|
||
landed the camino part on the trunk, still doing branch builds to get that ready (on my dog-slow pbg4)
Status: NEW → ASSIGNED
Assignee | ||
Comment 108•18 years ago
|
||
context menu suggestions landed on branch as well.
enjoy.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 110•18 years ago
|
||
I'm a bit confused now if this is a Camino-only bug or if morphed into a core bug, affecting Firefox and Tb as well?
Anyway, my current OS X install doesn't provide a Norwegian spell checker. For a while i could add the necessary spell.xpi files to add that functionality.
However, since we hooked in to the the spell checkers provided by OS X, i can't seem to use the spell check functionality by the xpi files anymore. They got installed successfully but i'm not able to activate them.
Am i missing something, or should i file a spin-off bug?
(In reply to comment #110)
> I'm a bit confused now if this is a Camino-only bug or if morphed into a core
> bug, affecting Firefox and Tb as well?
It's a Camino bug that happens to have required a "bit" of Core work.
> Anyway, my current OS X install doesn't provide a Norwegian spell checker. ...
> Am i missing something, or should i file a spin-off bug?
No, we only use the Mac OS X dictionaries in Camino. You should check out cocoAspell <http://cocoaspell.leuski.net/>, which apparently plugs in to the Mac OS X spelling system and offers a wider selection of languages, including Norwegian. (For Hebrew speakers, there's Hebrew Spelling Service <http://www.mitzpettel.com/software/hspell.php> that does the same thing and is tweaked for Hebrew.)
Assignee | ||
Comment 112•18 years ago
|
||
Firefox *trunk* uses the OS X spellchecker as well, and would have this problem.
Comment 113•18 years ago
|
||
(In reply to comment #112)
> Firefox *trunk* uses the OS X spellchecker as well, and would have this
> problem.
>
I've filed bug 345859 for this.
Comment 114•18 years ago
|
||
In case anyone goes looking for trouble in fox-trunk, note that carbonfox uses myspell, while cocoafox uses osxspell. The trunk builds have not been switched to cocoafox yet. All nightly foxes and birds with spell checking are still using myspell.
Sincerely,
The zookeeper
Updated•17 years ago
|
Hardware: Macintosh → All
You need to log in
before you can comment on or make changes to this bug.
Description
•