Closed Bug 234936 Opened 17 years ago Closed 15 years ago

Don't inline spell check URLs

Categories

(Core :: Spelling checker, defect)

1.8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: mscott, Assigned: brettw)

References

Details

(Keywords: fixed1.8.1, verified1.8.1.3)

Attachments

(1 file, 2 obsolete files)

This patch has been going into the Thunderbird builds since this summer but I
never got it checked into the tree for seamonkey to use to.

It just exposes functionality that already exists in the mozTxtToHTML converter
into a generic method for scanning a text string for a url.
Attached patch the patch (obsolete) — Splinter Review
Attachment #141759 - Flags: superreview?(bienvenu)
Attachment #141759 - Flags: superreview?(bienvenu) → superreview+
fixed for seamonkey now.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird0.6
uhm... how about a heads-up next time?
Comment on attachment 141759 [details] [diff] [review]
the patch

>Index: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

>+NS_IMETHODIMP mozTXTToHTMLConv::FindURLInPlaintext(const PRUnichar * aInString, PRInt32 aInLength, PRInt32 aPos, PRInt32 * aStartPos, PRInt32 * aEndPos)
>+{
>+  // call FindURL on the passed in string
>+  nsString outputHTML; // we'll ignore the generated output HTML

maybe use a nsAutoString instead in the hopes of avoiding a needless heap
allocation?


>Index: netwerk/streamconv/public/mozITXTToHTMLConv.idl

>+  void findURLInPlaintext([const] in wstring text, in long aLength, in long aPos, out long aStartPos, out long aEndPos);

why do you need to declare |text| as const?  it will already be |const| by
virtue of the fact that it is an in-param.  some documentation of this
method would also be nice.  what do the parameters mean?
em, yes. This is a dup of bug 172186 together with bug 116242, which I spent a
good day (IIRC) to fix due to mscott's request, and it also contains a number of
other fixes to the converter. He also said there that my patch would be what
should go into the main codebase eventually. I'd appreciate, if that would
actually happen, and I'd also appreciate to be on the hook for changes to the
converter, esp. API-changes.

Could you please back this out and try to get my old patch in? Thanks. REOPENing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
sorry Ididn't think anyone cared....

i changed it from an nsAutoString to an nsString because I thought most of the
buffers I was passing in where going to be long.

I'll change it back.

> why do you need to declare |text| as const? 

I was just following the pattern used by every other method in that interface.

See 
wstring   scanTXT([const] in wstring text, in unsigned long whattodo);
and
wstring   scanHTML([const] in wstring text, in unsigned long whattodo);

I'll add some comments
FYI, IIRC, the reason why we needed the larger patch was partially explained in
bug 172186 comment 11 (mscott's code would still try to spellcheck
well-delimited URLs like <http://www.bucksch.com> and URLs with punctation like
http://www.bucksch.com, http://www.bucksch.com), and the function is also needed
for "Load URL" context menu in the browser, where the URL is also just a substring.
(sorry, hit commit to early)
Oh, and before you're scared about the big patch in bug 116242, I did a number
of other fixes and lots of indention changes while I was at it.
(In reply to comment #6)
> sorry Ididn't think anyone cared....

no big deal really... i just like to know when API changes go into necko. 
otherwise, i'm caught by surprise if someone starts talking about some API that
i know nothing about but should as the module owner and all ;-)


> i changed it from an nsAutoString to an nsString because I thought most of the
> buffers I was passing in where going to be long.
> 
> I'll change it back.

hmm... that does make sense.  if you want to keep it as nsString for that
reason, i can understand.  perhaps a comment would suffice?  otherwise, some
casual reader might wonder.


> > why do you need to declare |text| as const? 
> 
> I was just following the pattern used by every other method in that interface.
> 
> See 
> wstring   scanTXT([const] in wstring text, in unsigned long whattodo);
> and
> wstring   scanHTML([const] in wstring text, in unsigned long whattodo);
> 
> I'll add some comments

yeah, you're right... the rest of the IDL is equally making a point of declaring
|const| for no good reason.  you know... the problem is that the whole IDL needs
some lov'in :-(
I guess I added the const because the function is not supposed to change the
string, and const garantees that to the caller. Is there a reason for it not to
be const? Maybe you should file a new bug about it.
Attached patch some IDL love.... (obsolete) — Splinter Review
Ben, the reason why an explicit const is not neccessary here has to do with the
fact that you declard all of these text parameter as input (in) parameters. The
IDL compiler automoatically translates that to a const string.
Comment on attachment 141765 [details] [diff] [review]
some IDL love....

sr=darin
Attachment #141765 - Flags: superreview+
mscott, OK. So, can we back this out and get my patch (incl. that const change)
in instead, please? Piling up changes on the current code would make that
harder. I'd say spurious spellcheck warnings for <you@example.com> are more
severe than a spurious |const| ;-).
(In reply to comment #5)
> em, yes. This is a dup of bug 172186 together with bug 116242, ...
Depends on: 172186
(In reply to comment #13)
> mscott, OK. So, can we back this out and get my patch (incl. that const change)
> in instead, please? Piling up changes on the current code would make that
> harder. I'd say spurious spellcheck warnings for <you@example.com> are more
> severe than a spurious |const| ;-).

checkin?  (target is blasted)
*** Bug 344609 has been marked as a duplicate of this bug. ***
Changing product and component because it also affects Firefox now.
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
Target Milestone: Thunderbird0.6 → ---
Version: unspecified → 1.8 Branch
Status: REOPENED → NEW
Flags: blocking1.8.1?
QA Contact: spelling-checker
I don't imagine this blocks release. There are two problems:

There is some code in the word finder file to detect URLs. It is currently commented out becuase there is a bug in it somewhere and it crashes. It is probably easy to fix, but I was in a hurry to get the new word finder working.

If this is fixed, the other problem is with highlighting. This problem might be OK to ship with. Say you have "htp://qwert.com" which is an invalid URL. You get 3 misspellings: "htp" "qwert", and "com". When you add a "t", you get a valid URL and we need to delete any marked misspellings in the range.

This is not really supported by the current spellchecker; it just checks the last character to see if it is in the range, and then deletes the range if it is. We could come up with more elaborate ways to check, but this makes it very complicated. Checking ranges is the slowest thing already, and it needs to be much faster for release. Complicating it is very undesirable.

We might be able to ship with the second bug. Just fixing the first might be less annoying than the current behavior.
> Say you have "htp://qwert.com" which is an invalid URL.

That depends on your definition of "invalid URL". Per Necko's definition, it's a fine URL.
Per drivers, doesn't sound like a blocker for 1.8.1, especially given comment 18.
Flags: blocking1.8.1? → blocking1.8.1-
roc, pkasting, and I discussed this. Once bug 345099 is fixed, we should be able to do much faster range+selection intersections.

Then, we can check for any ranges that intersect the current word and remove all of them. This shouldn't be too hard, I will try to get this working for 2.0 beta 2 if possible.
OS: Windows XP → All
Hardware: PC → All
Summary: don't spell check URLs in mail compose → Don't inline spell check URLs
Target Milestone: --- → mozilla1.8.1beta2
*** Bug 343610 has been marked as a duplicate of this bug. ***
Assignee: mscott → brettw
Depends on: 345099
No longer depends on: 172186
Attached patch PatchSplinter Review
This patch requires the patchs in bug 345099 and bug 345751.
Attachment #141759 - Attachment is obsolete: true
Attachment #141765 - Attachment is obsolete: true
Attachment #230821 - Flags: review?(roc)
Whiteboard: [has patch][needs review]
Attachment #230821 - Flags: review?(roc) → review+
Fixed on trunk.
Whiteboard: [has patch][needs review] → [baking on trunk]
Attachment #230821 - Flags: approval1.8.1?
Whiteboard: [baking on trunk] → [needs approval]
Comment on attachment 230821 [details] [diff] [review]
Patch

approved by schrep for drivers
Attachment #230821 - Flags: approval1.8.1? → approval1.8.1+
Fixed on branch.
Status: NEW → RESOLVED
Closed: 17 years ago15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs approval]
Duplicate of this bug: 297858
Verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 and tested with various urls - no spell check on urls.
Keywords: verified1.8.1.3
You need to log in before you can comment on or make changes to this bug.