Our JS dialog code uses editorshell heavily. Charley and I have talked about how we need to excise it, but there's no bug tracking it. Now there is.

Glad I found this; I was about to file this bug! Brade thought it would be easier to handle if we did each dialog separately, so I'll file bugs for each set and add dependencies to this one.

Methods used by all dialog must be fixed first.

Comment on attachment 93729 [details] [diff] [review] SharedMethods patch 1 I talked to charley and he's going to fix up some typos and remove some unnecessary checks; hopefully akk or someone can r= if I'm not around

Update on EdDialogCommon.js. This was done with "-w" to make the diff much more readable. Last one was a pain because of new "try/catch" and subsequent indent changes. Rest assured the indendenting is correct in new code.

Comment on attachment 93740 [details] [diff] [review] SharedMethods v2 seems like there is still some cleanup we can do here: + try { + var headList = editor.document.getElementsByTagName("head"); if (headList) return headList.item(0); + } catch (e) {} this could just be: + try { + var headList = editor.document.getElementsByTagName("head"); return headList.item(0); + } catch (e) {} probably also in some other places involving "metaNodes"

removed "if (headlist)"

Comment on attachment 94049 [details] [diff] [review] SharedMethods v3 r=brade

Comment on attachment 94049 [details] [diff] [review] SharedMethods v3 rs=alecf

Comment on attachment 94049 [details] [diff] [review] SharedMethods v3 This patch was checked in

This changes "insert/deleteElement" to "insert/deleteNode", which was missed in previous patch.

Comment on attachment 95111 [details] [diff] [review] Fixup patch v1 r=neil@parkwaycc.co.uk Good catch on Page Properties dialog!

Comment on attachment 95111 [details] [diff] [review] Fixup patch v1 sr=kin@netscape.com

Comment on attachment 95111 [details] [diff] [review] Fixup patch v1 checked into 1.2 trunk

Remove editorShell from editorUtilites and implement "isHTMLEditor()" method.

Fixed stuff noticed by reviewer. Removed isPlainTextEditor since we don't need it.

Comment on attachment 96052 [details] [diff] [review] EditorUtilities v2 r=brade tho remove the isPlaintextEditor function from editorUtilities.js (I also wouldn't mind if the check for !isHTMLEditor() were at the top in nsButtonPrefListener.observe) be sure to test plaintext mail compose and the ability to open our dialogs in html mail compose.

Ok, "isPlainTextEditor()" is deleted and if (!isHTMLEditor()) was moved as requested.

Comment on attachment 96052 [details] [diff] [review] EditorUtilities v2 + return (editorflags & Components.interfaces.nsIPlaintextEditor.eEditorPlaintextMask); need to make sure this is actually a boolean: + return (editorflags & Components.interfaces.nsIPlaintextEditor.eEditorPlaintextMask)!=0; (I'm speaking from having been bit by similar stuff before, plus you're consistent with the boolean-negated checks in IsInTable/IsInTableCell/etc) sr=alecf with that nit

Comment on attachment 96052 [details] [diff] [review] EditorUtilities v2 This patch checked into 1.2 trunk, minus the "isPlainTextEditor()" method, so alecf's "nit" wasn't necessary.

This patch removes editorShell and also has some cleanup menulist code donated by neil@parkwaycc.co.uk that greatly simplifies the code and makes it more readable.

Replaced "I" etc with const g_I etc and other small changes per reviewers comments.

Comment on attachment 97206 [details] [diff] [review] List Properties Dialog v2 r=brade p.s. Shouldn't Neil be listed as a contributor in this file?

> p.s. Shouldn't Neil be listed as a contributor in this file? I always overlook that, it's not that I'm modest, I just ignore comments :-)

I've added Neil as contributor to both EdListProps.js and EdListProps.xul

Comment on attachment 97206 [details] [diff] [review] List Properties Dialog v2 sr=hewitt

"List Properties Dialog v2" patch checked into 1.2 trunk.

> Created an attachment (id=97206) > List Properties Dialog v2 > > Replaced "I" etc with const g_I etc and other small changes per reviewers > comments. I missed that :-( how is g_I more readable than "I" - why not g_1, g_OL etc?

How about some more readable constants?

Comment on attachment 97206 [details] [diff] [review] List Properties Dialog v2 This was checked in

Comment on attachment 98143 [details] [diff] [review] Form dialogs r=brade

Comment on attachment 97955 [details] [diff] [review] List Properties Dialog v3 :-) r=cmanske

Comment on attachment 99390 [details] [diff] [review] Advanced Edit dialog v1. obsoleting this since it isn't the advanced edit dialog stuff

Comment on attachment 99411 [details] [diff] [review] Link Properties Dialog in EdLinkProps.js, add a blank line in Startup after closing curly for !gActiveEditor Also remove the dump in ChangeLinkLocation. Is this the correct function to call? - editorShell.RemoveTextProperty("a", ""); + EditorRemoveTextProperty("href", ""); r=brade

Comment on attachment 99413 [details] [diff] [review] Advanced Edit dialog v2 r=brade

Comment on attachment 99415 [details] [diff] [review] Link Properties Dialog v1 in EdLinkProps.js, add a blank line in Startup after closing curly for !gActiveEditor I like the blank line in ChangeLinkLocation. r=brade

Comment on attachment 99419 [details] [diff] [review] List Properties Dialog v4 r=brade but my personal bias is to prefix constants with "k" rather than "g" (e.g. "kDecimalCSS")

Comment on attachment 99536 [details] [diff] [review] Insert Characters Dialog v1 r=brade

Comment on attachment 99534 [details] [diff] [review] Image Property Dialog and Overlay v1 r=brade

Comment on attachment 99563 [details] [diff] [review] Insert Table dialog and Table / Cell Properties dialog v1 r=neil@parkwaycc.co.uk on cmanske's changes (some are mine :-) Notes: >-var tagName = "table" >-var tagname = "table" Convert all these to constants for consistency in all dialogs? > var gSelectedCellCount = 0; >+ else >+ gSelectedCellCount = 0; Overkill?

Comment on attachment 98143 [details] [diff] [review] Form dialogs sr=dveditz

Comment on attachment 99413 [details] [diff] [review] Advanced Edit dialog v2 sr=dveditz

Comment on attachment 99415 [details] [diff] [review] Link Properties Dialog v1 sr=dveditz

Comment on attachment 99419 [details] [diff] [review] List Properties Dialog v4 sr=dveditz

Comment on attachment 99534 [details] [diff] [review] Image Property Dialog and Overlay v1 sr=dveditz

Comment on attachment 99536 [details] [diff] [review] Insert Characters Dialog v1 sr=dveditz

A global editor variable "gEditor" was recently added to fix bug 167712, but that is incompatable with the strategy we are using to remove editorShell dependencies. "GetCurrentEditor()" should be used to obtain nsIEditor and nsIHTMLEditor interfaces in dialogs. This fixes that problem so all the above patches will work correctly. Also changes a couple of other minor problems found.

Comment on attachment 99563 [details] [diff] [review] Insert Table dialog and Table / Cell Properties dialog v1 r=neil@parkwaycc.co.uk (neil reviewed above but didn't check the box)

Comment on attachment 99722 [details] [diff] [review] Insert Table dialog and Table / Cell Properties dialog v2 whew! big patch. everything seems to be in order. r=andreww

Comment on attachment 99730 [details] [diff] [review] Page Color properties and Horizontal Rule dialogs r=andreww

Comment on attachment 99735 [details] [diff] [review] EdDialogCommon.js fix v1 r=andreww

Comment on attachment 99722 [details] [diff] [review] Insert Table dialog and Table / Cell Properties dialog v2 sr=sfraser

Comment on attachment 99730 [details] [diff] [review] Page Color properties and Horizontal Rule dialogs sr=sfraser

Comment on attachment 99735 [details] [diff] [review] EdDialogCommon.js fix v1 sr=sfraser

Comment on attachment 98143 [details] [diff] [review] Form dialogs checked into 1.2 trunk

Comment on attachment 99413 [details] [diff] [review] Advanced Edit dialog v2 checked into 1.2 trunk

Comment on attachment 99415 [details] [diff] [review] Link Properties Dialog v1 checked into 1.2 trunk

Comment on attachment 99419 [details] [diff] [review] List Properties Dialog v4 checked into 1.2 trunk

Comment on attachment 99534 [details] [diff] [review] Image Property Dialog and Overlay v1 checked into 1.2 trunk

Comment on attachment 99536 [details] [diff] [review] Insert Characters Dialog v1 checked into 1.2 trunk

Comment on attachment 99722 [details] [diff] [review] Insert Table dialog and Table / Cell Properties dialog v2 checked into 1.2 trunk

Comment on attachment 99730 [details] [diff] [review] Page Color properties and Horizontal Rule dialogs checked into 1.2 trunk

Comment on attachment 99735 [details] [diff] [review] EdDialogCommon.js fix v1 checked into 1.2 trunk

Three dialogs weren't using setShouldTxnSetSelection (shouldn't that be an attribute?) when they should have been, also added extra try/catch on startup for consistency.

Note: this is based on a patch previously reviewed by varga@netscape.com

Also includes removing editorShell from EdDialogTemplate.js, the simplified template for new dialogs.

Includes the usual global var cleanup as well. This must be checked in with "Convert Get/SetDocumentTitle methods v2" in bug 16029 to use new Set/GetDocumentTitle utilities.

This must be checked in with "Convert Get/SetDocumentTitle methods v2" in bug 16029 to use new Set/GetDocumentTitle utilities.

This must be checked in with "Convert Get/SetDocumentTitle methods v2" in bug 16029 to use new Set/GetDocumentTitle utilities.

Note that these dialogs are not currently used, but hopefully they will be revived later!

Forgot to remove "InitEditorShell()". Note that the Publish Progress dialog is a non-modal dialog, which is why we don't check for "GetCurrentEditor()" and close the dialog if not found. Editor interfaces are not used in this dialog. This must be checked in with "Convert Get/SetDocumentTitle methods v2" in bug 169029 to use new Set/GetDocumentTitle utilities. [Same with "Save As Charset" and "Page Properties" patches -- wrong bug cited on those :-( ]

Comment on attachment 99864 [details] [diff] [review] Cleanup for some form dialogs r=cmanske

Comment on attachment 99866 [details] [diff] [review] Selection list r=cmanske I'd prefer if you prefixed global variables with "g". As you can see in other dialogs, I've been converting other older dialogs to that pattern.

Comment on attachment 100317 [details] [diff] [review] list dialog patch This isn't the patch it is labeled as; renaming it and obsoleting it since it has already landed

Comment on attachment 100320 [details] [diff] [review] Page Properties dialog fix this typo: consistenly This isn't a regression in this patch, but BuildRecentMenu seems really inefficient to call here. I know we need to set the pref, but we really don't need to rebuild the menu since this document isn't in that list by design. (I just now filed bug 170522 on this issue.) In UpdateWindowTitle, there is no point getting the scheme until you get inside the "if (filename)" block. Do we need to worry about updating the window title from a dialog? Is there some scenario where a new title won't be reflected on the main window? r=brade with the above

Comment on attachment 100323 [details] [diff] [review] Save As Charset dialog please fix the white space after the "if" in places like this: + if(!gContenttypeElement && (editor.contentsMIMEType != 'text/plain')) as well as this: + if( ! gContenttypeElement ) (I expect to see something like "if (!foo)") r=brade with the above cleanup

Comment on attachment 100326 [details] [diff] [review] Named Anchor Dialog v1 fix the whitespace on this line (add space after if): + if(!gAnchorElement) clarify this comment by using == or "means" or some other word: + // "false" = don't delete selected text when inserting r=brade with the above fixes

Comment on attachment 100328 [details] [diff] [review] Image Map dialogs v1 I don't think you should remove these commented lines (just fix them). I think there was an issue which is why the code remains but is commented out: function setMapName(){ - //imageMap = editorShell.CreateElementWithDefaults("map"); - //dump(imageMap+"\n"); - //imageMap = frameDoc.createElement("map"); - I'd say that is probably true for most of the code removal in this patch

Comment on attachment 100332 [details] [diff] [review] Convert To Table dialog v1 in onAccept, move the setting of editor until you need it (it's odd to see it assigned and not used until the outputToString). I'd expect to see it after the switch. Then again, the switch seems very out of place maybe it should be the code to move? No one seems to care about that until after you've called outputToString (and possibly returned). r=brade if the above is cleaned up

Comment on attachment 100335 [details] [diff] [review] Publishing Dialogs v2 This change doesn't seem right to me. If the page has no title, we shouldn't show anything, should we? Showing "untitled" would be confusing. + if (!title) + title = "("+GetString("untitled")+")"; This line has inconsistent whitespace (missing space after comma): + SetProgressFinished(null,0); I'm not sure if that's correct in this case; I need to review what SetProgressFinished does. r=brade on the rest

RE: Brade comment #87. Yes, I agree that it really isn't needed for the current window, but rebuilding a menu isn't very expensive, so I thought that avoiding the complication of dealing with separate methods wasn't necessary. Not a big deal. I can redo it that way.

Commented-out code not removed; editorShell -> editor conversion done instead as requested.

Comment on attachment 100609 [details] [diff] [review] Image Map dialogs v2 I don't think you should remove the middle part here: - //window.arguments[0] = "test"; //editorShell.editorDocument.body.appendChild(imageMap); //editorShell.InsertElementAtSelection(imageMap, false); + // window.arguments[0] = "test"; //GetCurrentEditor().insertElementAtSelection(imageMap, false); I don't think you should add "var newRect" here unless you are absolutely certain what is going on; don't try to clean up warnings: function createRect(which){ - //newRect = editorShell.CreateElementWithDefaults("area"); + var newRect; r=brade if you fix the above

Previous patch in comment #76 was the wrong one! ColorPicker, Insert HTML Source, Link Checker dialogs v2 Also includes removing editorShell from EdDialogTemplate.js, the simplified template for new dialogs.

Comment on attachment 100614 [details] [diff] [review] ColorPicker, Insert HTML Source, Link Checker v2 EdLinkChecker.js has an error: + if (!objects || objects.Count = 0) should probably be: + if (!objects || 0 == objects.Count()) r=brade with that fix

Comment on attachment 100614 [details] [diff] [review] ColorPicker, Insert HTML Source, Link Checker v2 I beg of you, learn to use xargs. Keep a list of your files, like editor/ui/dialogs/content/EdInsSrc.js editor/ui/dialogs/content/EdLinkChecker.js and then use xargs: cat changed-files.list | xargs cvs diff > foo.patch I'm not going to seperately sr= each of these patches.

I noticed that the display mode constants no longer need to match editor shell. So, I thought they should be numbered after the tab position instead. This makes it easier to change the selected tab when you change mode. I've also hidden the View/Format Toolbar menuitem in source mode. I've also set the "group" attribute which was missing on the mode menuitems. I've also cleaned up SetDisplayMode a bit.

Comment on attachment 100335 [details] [diff] [review] Publishing Dialogs v2 r=brade

Fixed problems in Neil's patch: 1. The "editorOverlay" part of the patch is a bit funky! 2. Previous patch didn't uncheck the previous menuitem correctly. 3. Tabs at window bottom didn't redraw when mode was changed using menu.

Comment on attachment 101132 [details] [diff] [review] Change display mode constants v2 r=cmanske for bulk of change by Neil. Brade looked at it as well.

Comment on attachment 101132 [details] [diff] [review] Change display mode constants v2 OK, so there's a bug in selectedItem for which I am close to landing a fix, but that doesn't explain why you removed my menu correction.

Comment on attachment 101278 [details] [diff] [review] Combined patch for SR carry forward r=

Comment on attachment 101132 [details] [diff] [review] Change display mode constants v2 Ok, I take it all back, menuitem groups only work after the first time the menu has been shown, sigh :-(

Comment on attachment 99864 [details] [diff] [review] Cleanup for some form dialogs see "combined patch" below

Comment on attachment 99866 [details] [diff] [review] Selection list see "combined patch" below for SR

Comment on attachment 100320 [details] [diff] [review] Page Properties dialog see "combined patch" for SR

Comment on attachment 100323 [details] [diff] [review] Save As Charset dialog see "combined patch" for SR

Comment on attachment 100332 [details] [diff] [review] Convert To Table dialog v1 see "combined patch" for SR

Comment on attachment 100335 [details] [diff] [review] Publishing Dialogs v2 see "combined patch" for SR

Comment on attachment 101132 [details] [diff] [review] Change display mode constants v2 see "combined patch" for SR

Comment on attachment 100609 [details] [diff] [review] Image Map dialogs v2 see "combined patch" for SR

Comment on attachment 101278 [details] [diff] [review] Combined patch for SR this looks a little odd: + { + treeBoxObject.invalidateRange(getParentIndex(index), index); + itemArray[index].level = 2; + treeBoxObject.view.selectionChanged(); + } it seems odd to invalidate a range that includes the index row, but then change the level. Was this intentional? will selectionChanged() already cause some kind of repaint? I see this in a few places.. At least add some comments Another bit of possibly confusing code should at least have some comments: + { + itemArray[index].level += itemArray[index + 1].container; + itemArray.splice(index, 0, itemArray.splice(++index, 1)[0]); + } + selectTreeIndex(index, true); lines like this look scary: + while (itemArray[--index].level > 1); because they'll throw an exception if for some reason there is no level > 1 - again, comments would be useful to explain why this will never happen generally the patch looks fine but man you need to comment WAY more. There are whole sections of code that I had to reread like 5 times to figure out what you're doing - especially in the tree code, splicing arrays, etc. Think of it this way: reviews will happen faster because it will take less time to figure out what the heck is going on :) sr=alecf if you comment the code I've quoted, and at least peruse your patch and sprinkle some comments around some of the other code you're adding.

All dialogs are converted and checked in except for Spell Checking. These require a new interface to replace nsIEditorShell interface to spellchecker (bug 168999).

Notes on while (itemArray[--index].level > 1); Since itemArray[0].level = 0 always exists this will always stop at row 0. getParentIndex tests for index = 0 because its level is 0. optgroupObject.prototype.moveUp can't have an index of 0 (reserved for select). gDialog.lastChild index must start at least 1 because row 0 always exists.

Woopee! This removes all remaining editorShell usage, including Spelling dialogs.

Comment on attachment 102950 [details] [diff] [review] Last remove editorShell patch for dialogs v1 in EdDialogCommon.js, you removed a comment that said each editor window should include this file, is that still true? Should the comment remain? in EdDictionary.js, you have a typo "gSpellCheckcer" should be gSpellChecker in EdSpellCheck.js, shouldn't Replace and ReplaceAll be merged into one funtion (which takes a parameter)? r=brade with the above changes

Comment on attachment 102950 [details] [diff] [review] Last remove editorShell patch for dialogs v1 + if (!GetCurrentEditor()) + { + window.close(); return; - dump("EditoreditorShell found for dialog\n"); - + } seems like there should be SOME error for the user if the current editor isn't available, no? Or is that the convention you've used elsewhere? sr=alecf with either a good explanation for me, or some kind of warning to the user.

alecf: In current Composer, you really can never launch a dialog without an editor! So this is an impossible event. In the future, there may be multiple editors per window and the "current" editor changes with focus. In that case, one can conceive of trying to launch a dialog via some command message, but there is no currently-focused editor, so you wouldn't want a dialog in that case. I don't think we every need to tell the user that, but we could put back the dump to see if that situation could really happen. Right now, we have not been doing that for any dialog.

> In that case, one can conceive of trying to launch a dialog via some command > message, but there is no currently-focused editor, so you wouldn't want a > dialog in that case. In which case shouldn't the command be disabled? Or, the command may be intrinsically focus-sensitive (like cut/copy/paste are).

checked into 1.3a trunk

verified with lxr