Composer should use the new find component. The only library should be removed from the build/tree.

moving to 9.2

This requires a new interface for Find and Replace, in a similar way to how we do find now. I think that's too major for 0.9.2

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)

reassign to Akkana since she's removing on this or something similar.

This looks exactly like the old editor replace dialog, but IMHO needs cosmetic work: OK, Cancel and Close are all redundant with each other. I need a way of preventing the dialog from showing OK and Cancel.

These patches are dependant on the new find component introduced by the patch in bug 97157. I'd like to get Simon's review on the xbl part of the patch, since he helped on that. Note that I added a .editor (vs. .editorShell) accessor -- we're planning to remove the editor shell, so I didn't want to add any new code that used it, and this will help other components wean themselves away from it. The implementation of the accessor still uses .editorShell, which will be fixed with the other editor shell work. Simon, if you can sr this, who else (if anyone) should I ask for a review? Charley, can you review the dialog part of the patch, and perhaps suggest a way that I can get rid of those redundant buttons? Worth noting is that the new find component is find only, not replace. The editor is the only component with enough knowledge to replace safely, so I'm doing the replace in editor code, not find code. If there are other components who want to do a replace, the find component sets the selection around the found text, so if they want, they can call Range delete and insert methods. They won't get the benefit of edit rules, but that's the risk they run by not using an editor for text replacement.

Akkana: This fix is missing a new EditorReplace.dtd.

Oops! Thanks for noticing. Here's a new patch.

Comment on attachment 62587 [details] [diff] [review] Updated patch: Fix extra "Ok" button r=cmanske

Oh yea, one comment: Don't we need the license stuff at the top of EditorReplace.dtd?

The new find code (97157) went in, but it's preffed off by default because there wasn't time to test/review it. So this has to wait until new find is switched on.

*** Bug 122717 has been marked as a duplicate of this bug. ***

Comment on attachment 62587 [details] [diff] [review] Updated patch: Fix extra "Ok" button In this line, you don't need the oncommand="close()", you get that for free by specifying dlgtype="cancel" + <button dlgtype="cancel" id="close" label="&closeButton.label;" oncommand="close();"/> fix that and sr=hewitt

Okay, I checked in the part that was reviewed, except for ComposerCommands.js since we can't turn the feature on yet. However, in the meantime, naturally, the patch has broken, probably due to Mike's editor embedding landing. In EdReplace.js, var editorXUL = window.opener.document.getElementById("content-frame"); isn't getting anything, so gFindInst is null, so nothing works. The files like jar.mn and various MANIFESTs also somehow got dropped from the patch somewhere along the way. I've made a new patch which includes these files , and adds the pref in to ComposerCommands.js, but I can't test it because of the "content-frame" problem. Charley, I don't suppose you know offhand how I can get the xul <editor> element now?

Not being able to get the <editor> element by getElementById is news to me! Mike?

Shouldn't it just be window.opener.getElementById("content-frame")? If not, try window.opener.frames[0] or whatever the index is if there is more than one frame (editor, iframe, browser) in the document you are accessing. Not sure if editor is in this array.

If I try that I get: window.opener.getElementById is not a function. Not sure about the frames[]. We're asking mjudge now ...

*** Bug 95461 has been marked as a duplicate of this bug. ***

My addon [http://mozblog.mozdev.org ], make use of frames to setup the window for editor (with the browser). Its a (very) bad hack, but it's the only way I can get <editor> and <browser> on the same page started without stuffing up browsing framed pages. There should be a better way to get windows instance. Right now getElementById live inside document and not window.

Comment on attachment 62253 [details] [diff] [review] Patch: add needed accessors to the <editor> tag's XBL Well, at least part of the problem is that this part of the patch never got reviewed or checked in!

Comment on attachment 68954 [details] [diff] [review] Patch, the next generation r=cmanske

on a new non-debug CVS build, Linux, I'm getting several thousand debug lines in console for a single "find on page". It also changes charset in console on subsequent finds, so i have to reset all the time with ctrl+v + ctrl+o Related to this checking? If so: Can the output be disabled somehow? It's a lot of noise.

Oh, gack. DEBUG_FIND is on in nsFind.cpp. I swear I checked that and turned it off before checkin! But I must have turned it back on again. I'll fix it today as soon as the tree opens, I promise!

the tree is closed untill the freeze, i think. From tinderbox: "Checkins will be Metered by the Sheriff until the freeze. Contact the Sheriff to get scheduled"

The fix went in a few hours ago. Sorry about that. Re the composer stuff: the new composer find dialog is in, but switched off by default. Since new find in the browser is on by default, I put composer find/replace on a new pref: user_pref("editor.new_find", true) will turn it on. It currently sometimes hits a crash during find/replace in tricky pages. The crash is in nsDeque code, and timeless has a fix for that in bug 114166, so I've made this bug dependant on that. I'm not sure I'm always doing the right thing for replace -- e.g. if you bring up the find dialog, enter a find string and a replace string, and click "replace" without first clicking "find", should it find the first instance then replace it? Currently it just replaces the current selection, which is wrong. Also, the dialog is modal, which really sucks (you can't scroll the composer window or change the selection to make it find from a different place). The dialog shouldn't be modal and I may need Charley's help in fixing that. Other than those issues, it just needs testing before we enable it. Leaving on 0.9.9 for the moment, though it's likely that fixing the remaining problems and making it the default will slip to 1.0.

*** Bug 97689 has been marked as a duplicate of this bug. ***

Akkana, to have the dialog non-modal, launch it with the 'dependent' flag. This will keep it on top, but allow access to the buffer underneath. Example here: http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/findUtils.js#61 window.openDialog("chrome://global/content/finddialog.xul", "_blank", "chrome,resizable=no,dependent=yes", findInst);

Dialogs that change document should use "Close" instead of "Cancel"

This needs some more code to be written (to get the selection in JS and compare it against the pattern) so 0.9.9 probably isn't realistic -- bumping to 1.0.

New patch, which in the replace case, checks the selection to see if we're already pointing at a match, and if we're not, first does a find before replacing. To do this, I also added return values to some of the dialogs which weren't returning any consistent value before. Charley: does it matter what value is returned by the onXXX functions called from the buttons in the dialog? If so, I'll need to restructure this slightly to make the onXXX functions return the right value (and use different routines to return true or false for internal purposes). I've also removed modal and added dependant=yes as Brian suggested; but it doesn't work (the dialog doesn't stay on top of the editor window). That's still better than it being modal, since we don't want it to prevent the user from selecting in the editor window, but it sure would be nice if we could keep the dialog on top. This could use more testing than I've given it (I haven't yet written a tough test case for replace) but I think it's pretty close to ready.

The "dependant=yes" seems to work for the Find dialog, doesn't it? Could this be a Linux-only problem? Re: return values from onLoad: Be careful! you can use your own return values for that method or other "internal" methods, but returning true from a method tied to one of the buttons like "Close" will close the dialog.

dependant=yes should read dependent=yes ^ ^ It's a typo. Fixing that should do it. Also, using just 'dependent' should work.

Comment on attachment 71407 [details] [diff] [review] Check the selection before replacing, and find first if necessary This fix is contingent on fixing the nsIEditor.idl for getting the seleciton. Just for conistency of style: we usually use the "gDialog" global just for XUL elements in the dialog. Thus I would have use "gEditor" instead of gReplaceDialog.editor.

in isSpace, could you add a comment to explain that '/a0' is an nbsp?

removing myself from the cc list

This patch: 1. adds Charley's new UI. 2. Adds the comment Kathy requested. 3. Adds a fix for an infinite-loop problem if you do "replace all" with wraparound set where the replacement can trigger more matches, e.g. replace a with aa. (It also provides an example of how to use nsIFind from JS, which someone asked about in another bug.) If you find from the middle of a document with wrap on, it can show two problems: 1. Sometimes it does one more replace than it should, if the first match is in the same node as the selection is when you start. I think the code here is right and this may be a bug in nsFind, but I have to investigate further. 2. Sometimes it ends up with both a selection AND a blinking caret, not in the same place. That shouldn't be possible and suggests an editor bug, and I wonder if the first problem could possibly be resulting from the second? I'd like to get a review on this and check it in anyway (since I expect the fix will be somewhere other than these files). And consider whether we're ready to turn the feature on by default, or should we wait until these two problems are better understood?

Comment on attachment 73269 [details] [diff] [review] improved patch r=cmanske I'd prefer using "findInput" and "replaceInput" instead of "findKey" and "replaceKey"

I renamed Key to Input, and noticed that the enabling wasn't working right for the new button (or, sometimes, for the other replace buttons). Also, we were getting the text field's value too often, which is a performance problem. Here's a fix (otherwise just like the previous patch).

Charley had some minor cleanup he wanted done, so I've done that.

Oops, attached the wrong file (older version). Here's the real one.

Comment on attachment 73337 [details] [diff] [review] Really same patch with some cleanups r=cmanske. Note that gEditor.select depends on fix in 128903.

The latest patch has some bogus capitalization changes ResetModificationCount() to resetModificationCount(). Those are part of another bug, NOT part of this patch. (Sorry, juggling too many patches in the same tree.)

Comment on attachment 73337 [details] [diff] [review] Really same patch with some cleanups >+// Test to see if a character is whitespace, including a0 (&nbsp;). >+function isSpace(ch) >+{ >+ return (ch == ' ' || ch == '\t' || ch == '\n' || ch == '\a0'); > } This seems like a big potential performance issue since for each char through your loops you've got interpreted function call overhead plus several interpreted string compares. If you can re-write this using a pre-compiled regexp you'd probably get quite a boost. For example, instead of using isSpace(x) you could use /\s/.test(x) which would at least move the compares out of the interpreter if not the function call. \s includes the 0xa0 char you're concerned about, plus a couple others. >+ // Unfortunately, because of whitespace we can't just check >+ // whether (selStr == specStr), but have to loop ourselves. >+ // N chars of whitespace in specStr can match any M >= N in selStr. oh, I see. Drat. If you didn't care that M >= N but instead considered any batch of whitespace equal to any other you could do much better. I now see that the actual finding is done elsewhere in a native component so I guess it's not as big a deal as I thought. but still, here's what I was thinking: specArray = specStr.match(/\S+/g); selArray = selStr.match(/\S+/g); if ( specArray.length != selArray.length) matches = false; else for (i=0; i<selArray.length; i++) { if (selArray[i] != specArray[i]) { matches = false; break; } } If leading/trailing space is significant you could use selStr.split(/\s+/) instead (note lower vs uppercase s). >+ // They're both whitespace, so skip past whitespace in both strings >+ while (isSpace(specStr.charAt(i)) && i < specLen) >+ ++i; >+ while (isSpace(selStr.charAt(j)) && j < selLen) >+ ++j; So any given chunk of whitespace doesn't have to match or exceed its equivalent, it's the total number of whitespace chars if there are multiple chunks of whitespace. The only enforcement of M>=N seems to be in the total length of the strings I'm a little confused that onReplaceAll seems to be doing something completely different. Why?

I found the problem that was making replace-all sometimes do a few extra replaces at the end. This patch (when combined with the JS patch) fixes the last known bug in replace all, and means that we should be clear to enable the feature by default.

Explanation of the nsFind.cpp changes: - The SkipNode change is actually for bug 129971: the find code was sometimes imappropriately skipping nodes that came before or after comment nodes. Since that current SkipNode code checks parentage, positioning the iterator shouldn't be necessary any more. - I noticed that due to a shuffling of routines inside nsFind.cpp, I'd ended up with a situation where a method that's declared to return nsresult was actually returning PR_TRUE and PR_FALSE. Bogus, so I changed them all to NS_OK, making sure they all called ResetAll() first, and making sure that the range return was properly initialized to null. - The end offset wasn't being checked in the case where the end container is a text node. The iterator properly stopped at the last node, but we didn't stop text comparisons when we got to the end offset. That's why ReplaceAll wasn't stopping soon enough.

dveditz: Are you sure that \s andr \S includes the nbsp character a0 as whitespace? I thought I'd tried that and established that it didn't, and I know Charley and I checked his existing isSpace routine and it didn't handle that character. Charley just checked Flanagan, "JavaScript, the definitive Guide" (O'Reilly) and it said "whitespace" = [ \t\n\r\f\v]. If there is a faster JS routine which includes the a0 character, then that would be a big help. But checking for a0 is very important here. Yes, the code would be a lot simpler if we didn't insist that M >= N and just matched any amount of whitespace, but Kin says that's important (he rejected an earlier version of nsFind.cpp which made that assumption. The reason onReplaceAll is so different from the other routines: first, onReplaceAll doesn't care whether the current selection matches the pattern, because it's going to go through the whole document. So that code doesn't have to be there. (nsFind does essentially the same thing, matching whitespace and so forth, in C++ under the hood.) What ReplaceAll does need to care about is that it start from the current position, go to the end of the document, then start from the beginning of the document and end up at the current position. nsIWebBrowserFind (which is what the rest of the find/replace dialog code uses) doesn't have any way of specifying that; it will just keep looping after it gets back to the starting point until it doesn't find any more matches, and in certain cases (e.g. replace "a" with "aa") it will just keep finding matches forever. The lower level nsIFind interface doesn't have that problem because it lets us specify start and end ranges explicitly, so it's needed for ReplaceAll while the rest of the options can use the simpler higher-level interface.

Comment on attachment 74372 [details] [diff] [review] Patch to nsFind to make it stop at the right place r=cmanske

CC'ing kin In js 1.5 \s matches a0, the O'Reilly book covers 1.3 I believe. try the following URL: javascript:/\s/.test("\xa0") Unfortunately your algorithm doesn't match the space constraints as kin explained them in the newsgroup, where "a b c" is allowed to match "a b c" but not "a b c". Each group of spaces are dealt with on their own. Personally I'd bag it and count all spaces equally since it's simpler, but if you're going to care about spaces I think you have to do it as Kin says nsFind does it. So here's another attempt: specArray = specStr.match(/\S+|\s+/g); selArray = selStr.match(/\S+|\s+/g); if ( specArray.length != selArray.length) matches = false; else for (i=0; i<selArray.length; i++) { if (selArray[i] != specArray[i]) { if ( selArray[i][0].test(/\S/) || specArray[i][0].test(/\S/) ) { // capital \S, not a space chunk -- match fails matches = false; break; } else if ( selArray[i].length < specArray[i].length ) { // if it's a space chunk then we only care that sel be // at least as long as spec (XXX or did I reverse it?) matches = false; break; } } }

Comment on attachment 74372 [details] [diff] [review] Patch to nsFind to make it stop at the right place sr=dveditz for the nsFind patch

I tried the suggested code, but while testing it I got: chrome://editor/content/EdReplace.js line 191: selArray[i][0].test is not a function Line 191 is the line: if ( selArray[i][0].test(/\S/) || specArray[i][0].test(/\S/) ) I'm not clear under what circumstances my version will fail? I've been testing on a page that has three spaces, e.g. open the editor and type "Here is a page with<sp><sp><sp>three contiguous spaces" (or just make an html file containing that with a " &nbsp; " block in it and edit that), then bring up the find dialog, search for "th th" (it finds the three-space block), type a substitute pattern, then try doing replace with these patterns: th th (one space, succeeds) th th (three spaces, succeeds) th th (four spaces, fails with "not found") I believe you that my algorithm is missing something, I'm just not sure where or what might be a better test? Each group of spaces should be dealt with indepdendantly, shouldn't they?

Sorry, I blew it. test() is a RegExp method. You could, for example, do /\s/.test(...) For strings there's match and search which is what I should've used, search being faster when determining true or false because it doesn't have to copy stuff to construct an array. attempt 3 using search if ( selArray[i][0].search(/\s/) == -1 || specArray[i][0].search(/\s/) == -1 ) { // (lowercase \s) not a space chunk -- match fails or using test: if ( /\S/.test(selArray[i][0]) || /\S/.test(specArray[i][0]) ) { // (uppercase \S) not a space chunk -- match fails Not sure which is faster/better, but the search form might be more maintainable in the future.

Comment on attachment 74444 [details] [diff] [review] Patch with dveditz' changes >+ // if it's a space chunk then we only care that sel be >+ // at least as long as spec (XXX or did I reverse it?) heh, the idea was for you to double check me and remove the (XXX...) part of the comment. sr=dveditz if you clean up that comment problem (and the uppercase one above)

Comment on attachment 74444 [details] [diff] [review] Patch with dveditz' changes r=cmanske

Comment on attachment 74444 [details] [diff] [review] Patch with dveditz' changes a=asa (on behalf of drivers) for checkin to the 1.0 trunk

The fix is in and enabled! All that remains now is to remove the old find code. That means taking the #ifdef TEXT_SVCS_TEST code out of nsWebBrowserFind, removing the two prefs from all.js and ComposerCommands.js, and cvs removing the following files (as well as removing them from appropriate Makefiles): editor/txtsvc/public/nsIFindAndReplace.idl editor/txtsvc/nsFindAndReplace.cpp editor/txtsvc/nsFindAndReplace.h xpfe/components/find/resources/replacedialog.xul xpfe/components/find/resources/replacedialog.js xpfe/components/find/resources/locale/en-US/replacedialog.dtd

Bug 126312 already covers removing the old find code; I'm going to close this bug out and let that one cover the removal. Should I nominate that bug for moz 1.0?

verified.