Remove editorshell dependencies from mailnews

VERIFIED FIXED in mozilla1.2beta

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: akkzilla, Assigned: akkzilla)

Tracking

Trunk
mozilla1.2beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

17 years ago
As part of the ongoing editorshell removal, we need to remove editorshell
references from mailnews.
(Assignee)

Updated

17 years ago
Blocks: 109380
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
(Assignee)

Updated

17 years ago
Target Milestone: mozilla1.0.1 → mozilla1.2beta
(Assignee)

Comment 1

17 years ago
I have a preliminary patch.  It isn't complete -- doesn't address the creation
or window focus issues -- but it removes all editorshell dependency from
mailnews C++ files, and some from JS files.
(Assignee)

Comment 2

17 years ago
Created attachment 88204 [details] [diff] [review]
First patch: remove all edshell deps from C++, some from JS

Here's the patch.  It isn't ready for one reason: the call to LoadURL in
nsMsgCompose::SetEditor has no corresponding call.  This is part of the
initialization sequence problems which need to be worked out.
(Assignee)

Updated

16 years ago
Depends on: 157104

Comment 3

16 years ago
*** Bug 164967 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Blocks: 110949
(Assignee)

Comment 4

16 years ago
Here's an updated patch ... but it doesn't work yet.  For some reason, during
initialization, ComposeStartup gets called and sets
window.editorShell.editorType = "textmail", but then immediately after that
(according to venkman), just before we call EditorSharedStartup(),
window.editorShell.editorType is "text" and EditorSharedStartup doesn't do the
right things to set up a mail editor.  That's as far as I got ... I'll look at
it more Monday, but any insights would be appreciated!
No longer blocks: 110949
(Assignee)

Updated

16 years ago
Blocks: 110949
(Assignee)

Comment 5

16 years ago
Created attachment 100820 [details] [diff] [review]
Updated patch (compiles but doesn't run)
Attachment #88204 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #100820 - Flags: needs-work+

Comment 6

16 years ago
Comment on attachment 100820 [details] [diff] [review]
Updated patch (compiles but doesn't run)

in nsMsgCompose.cpp, is "bodyText" still needed (around line 960-975)

This change in MsgComposeCommands.js looks suspicious, change it back to
editorShell?  Maybe that is the cause of your problems?
-    window.editorShell.contentWindow.focus();
+    window.editorshell.contentWindow.focus();

Should this line:
+  editor.flags |= nsIPlaintextEditorMail.eEditorReadonlyMask;
be
+  gMsgCompose.editor.flags |= nsIPlaintextEditorMail.eEditorReadonlyMask;

If so, fix these also:
+  editor.flags &= ~nsIPlaintextEditorMail.eEditorReadonlyMask;
+	 var mailEditor =
gEditor.QueryInterface(Components.interfaces.nsIEditorMailSupports);
+      window.editorshell.editor.wrapWidth = gMsgCompose.wrapLength;
Varada, can you help Akkana and Kathy. Thanks
(Assignee)

Comment 8

16 years ago
Created attachment 101338 [details] [diff] [review]
Another attempt (still doesn't work)

I couldn't make the old patch run, so I decided to start from scratch
converting the compose C++ files.  Here is the result, which compiles, but
crashes on startup, in factory code trying to create an NNTP url.  The older
patch was also crashing in factory code, but then it was trying to create an
nsITheme.  I have no idea why anything I've touched here should have anything
to do with factory code or either of those classes.  I have tried rebuilding
(depend) from the top, but I have not tried a clobber build; perhaps I'll try
that next.  If anyone has any ideas, I'd love to hear them.
Attachment #100820 - Attachment is obsolete: true
(Assignee)

Comment 9

16 years ago
Created attachment 101511 [details] [diff] [review]
This one works!

Here's a patch that actually works, for both new messages and replies,
including quotes and signatures.

The order of things during initialization doesn't seem too well defined; I hope
setting the editor from the editorshell in the
nsMsgDocumentStateListener::NotifyDocumentCreated will handle all cases, at
least until Mike's new creation API is checked in and we can really ditch the
editorshell and use a more uniform initialization sequence.  Comments
appreciated from mailnews people who understand the initialization sequence (it
seems different from what we use for composer).

Aside from that, the main remaining use of editorshell is in MsgComposeCommands
for doing editorShell.contentWindow.focus().  Charley, what's the right way to
do this?  In editor.js we do window._content.focus(), but will that be enough
for a mail compose window that has more than one focusable area?  Should
MsgComposeCommands just define a gContentWindow that it gets from the
editorshell at init time, or can it access the gContentWindow from editor.js?

Other than that, everyone please test, review, pick holes in this.
Attachment #101338 - Attachment is obsolete: true

Comment 10

16 years ago
Comment on attachment 101511 [details] [diff] [review]
This one works!

r=brade

My only suggestion is that you add some comments to explain why you the apis
are "noscript":
+	/* Clear the editor */
+	[noscript] void clearEditor();
Attachment #101511 - Flags: review+
(Assignee)

Comment 11

16 years ago
As far as I'm concerned it doesn't have to be noscript; I was just copying the
other methods that are there only for the listeners.  I don't know why they're
all declared noscript; it doesn't look like they use non-scriptable interfaces,
except for notifyStateListeners.

Seth, can you sr this, and can I get a comment from you, Varada, J-F or another
mail person regarding whether you'd prefer the new ClearEditor method to be
noscript or not?  Thanks!
so far, we don't need to call ClearEditor from JS, therefore, it's fine with me
if it's not scriptable!
most of this looks straight forward.

some minor nits and questions:

1)

+  *aEditorShell = m_editorShell;
+  NS_IF_ADDREF(*aEditorShell);

just do:

+  NS_IF_ADDREF(*aEditorShell = m_editorShell);


2)
 
   return rv;
+  bodyText = ToNewUnicode(bodyStr);
 
you should also add:

+  if (!bodyText)
+    return NS_ERROR_OUT_OF_MEMORY;

3)

+      var textEd = gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail);
+      textEd.wrapWidth = gMsgCompose.wrapLength;

since this is in a try block, why not just:

gMsgCompose.editor.QueryInterface(nsIPlaintextEditorMail).wrapWidth =
gMsgCompose.wrapLength

Question:  Why is one wrapWidth and one wrapLength? 

please test this well.  in addition to reply and new window (plain and html) and
signature stuff, make sure to to test the cached compose window, plain and html
(remember, not enabled by default on linux).

if everything tests ok, sr=sspitzer
(Assignee)

Comment 14

16 years ago
Created attachment 102035 [details] [diff] [review]
Tweaked to address Seth's comments

This has the changes Seth suggested.

> Question:  Why is one wrapWidth and one wrapLength? 

That's the way the APIs are written.  I haven't introduced any new APIs here,
I'm just trying to shift away from using the old obsolete one.

> please test this well.  in addition to reply and new window (plain and html)
and
> signature stuff, make sure to to test the cached compose window, plain and
html
> (remember, not enabled by default on linux).

I've tested plain and html, reply and new, first and second windows, on linux
and they all work fine.  It appears that it's not possible to test the recycled
compose window on linux; I tried 
pref("mail.compose.max_recycled_windows", 1);
and the first window worked fine, the second had problems that seem quite
similar to some of the problems described in bug 130581 and bug 137698 (which
is the reason that behavior is disabled on linux), third and subsequent
invocations of the window worked fine.

Kathy says she might be able to test on mac.
Attachment #101511 - Attachment is obsolete: true
>> Question:  Why is one wrapWidth and one wrapLength? 
>That's the way the APIs are written.  I haven't introduced any new APIs here,
>I'm just trying to shift away from using the old obsolete one.

ok, so beyond the scope of your bug.  but is it worth logging another bug?

>and the first window worked fine, the second had problems that seem quite
>similar to some of the problems described in bug 130581 and bug 137698 (which
>is the reason that behavior is disabled on linux), third and subsequent
>invocations of the window worked fine.

the cached compose window is a slippery fish and can be confusing to test.  (I
still need to write up a document about it for both eng / and QA).

before you check in, we need test on win & mac (both use the cached compose
window).  I'll go write up a quick doc about the cached compose window and put
it on mozilla.org.
note visible yet, but see
http://www.mozilla.org/mailnews/arch/compose/cached.html for some info on the
cached compose window.
(Assignee)

Comment 17

16 years ago
Created attachment 102053 [details] [diff] [review]
Works for relaunch of cached windows

I talked to J-F a little about how the recycling works, and found out that the
editor flags were getting reset before gMsgCompose had gotten set to point to
the new editor.  Re-ordering those lines in gComposeRecyclingListener::onReopen
fixes the problem with the recycled window.  Works for me, on linux, and for
Kathy (thanks, Kathy!) on mac, plain and html.	This should be ready for sr
now.
Attachment #102035 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
Comment on attachment 102053 [details] [diff] [review]
Works for relaunch of cached windows

Kathy tested it on mac (I tested on linux) and gave her r=brade on IRC.
Attachment #102053 - Flags: review+
Comment on attachment 102053 [details] [diff] [review]
Works for relaunch of cached windows

assuming that {signature,nosig} x {reply,new} x {html,plaintext} x
{cached,notcached} x {win,mac,linux} all work, sr=sspitzer

(yikes, testing a mail compose change is non-trivial)
Attachment #102053 - Flags: superreview+
moved the suggested test matrix to mozilla.org

see http://www.mozilla.org/mailnews/arch/compose/testing.html

(probably not there yet, wait an hour or two)
(Assignee)

Comment 21

16 years ago
The remaining issues are:
1. initialization (cmanske is working on that).
2. Printing (spun off into bug 174391).
3. Content area focus (spun off into bug 174389).

So this bug can be marked fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 22

16 years ago
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.