Last Comment Bug 13474 - external processes/filters for textareas
: external processes/filters for textareas
Status: NEW
[Hixie-2.0] | Workaround in comment #48
: helpwanted
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: P3 enhancement with 43 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 38839 72539 91513 98402 100972 146497 166100 174903 187795 207672 241583 280608 357065 389409 (view as bug list)
Depends on:
Blocks: 103767
  Show dependency treegraph
 
Reported: 1999-09-09 13:19 PDT by Gregory.Wooledge
Modified: 2012-07-16 06:29 PDT (History)
54 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first attempt at adding external editing (10.91 KB, patch)
2002-03-21 04:41 PST, Rich Williams
no flags Details | Diff | Splinter Review
second attempt - edits text/html now (11.04 KB, patch)
2002-03-22 12:09 PST, Rich Williams
no flags Details | Diff | Splinter Review
Minor changes (11.18 KB, patch)
2002-04-02 18:29 PST, Akkana Peck
no flags Details | Diff | Splinter Review
slight changes to string / file / filespec code (11.43 KB, patch)
2002-05-29 10:35 PDT, Rich Williams
no flags Details | Diff | Splinter Review
refcnt to isupports / moved addref (11.57 KB, patch)
2002-06-22 04:46 PDT, Rich Williams
no flags Details | Diff | Splinter Review
remove temp files / better file permissions (11.61 KB, patch)
2002-07-01 10:53 PDT, Rich Williams
no flags Details | Diff | Splinter Review

Description Gregory.Wooledge 1999-09-09 13:19:19 PDT
One of my fondest dreams for a web browser is to be able to invoke an external
editor to edit the contents of the textarea.  I often find myself writing large
amounts of text in these textareas, for everything from bug reports (right now)
to articles on web-based forums.  Being able to do this in "rxvt -e vim" or
whatever other text editor I prefer would be greatly beneficial.

This should be bound to a keystroke, not done every time the user focuses on a
textarea -- that would be obnoxious.

Proposed control flow: write the contents of the textarea to a temporary file;
invoke the user's editor with the temporary file as input; check the exit code.
If the exit code is 0 (success) then read the temporary file, replace the
textarea's contents with the file's, and then delete the file.  If the exit code
is non-zero (failure) then leave the temporary file alone and give the user a
dialog box describing what happened.

It would also be of benefit to be able to invoke other processes, instead of
just one.  A user might want to have a key binding for the external editor
("rxvt -e vim") and another for a spell-checker ("rxvt -e ispell -x"), and so
on.  The key bindings and processes should all be user-configurable --
preferably in a menu, but Xdefaults or a text config file will do.  In each
case, the browser should do the same thing (see proposed control flow above),
not caring what manipulations the external program performs on the text.
Comment 1 karnaze (gone) 1999-09-09 13:50:59 PDT
Reassigning to Steve.
Comment 2 buster 1999-09-09 14:05:59 PDT
this is a great request.  we won't have any time for this any time soon, but
it's something anyone could pick up and implement on our architecture.

Setting milestone for M20, resolution to "remind" so we can consider it for
future products. I'll also post a message to the newsgroup to see if anyone is
interested.
Comment 3 phillip 1999-09-09 15:09:59 PDT
updating summary just in case somebody out there does a query on HELPWANTED.
Comment 4 Andreas Franke (gone) 2000-05-10 15:11:31 PDT
*** Bug 38839 has been marked as a duplicate of this bug. ***
Comment 5 Andreas Franke (gone) 2000-05-10 15:28:35 PDT
Bug 38839 is a little more general: Maybe plugins could also be used as a 
customized editor extension.
Comment 6 bsharma 2000-09-26 18:12:41 PDT
Updating QA contact.
Comment 7 Hixie (not reading bugmail) 2000-10-26 14:39:13 PDT
Reopening, moving to Future, and nominating for mozilla1.2. This would be 
brilliant. I am aware of at least two browsers that already do this: Emacs/W3
(which obviously uses Emacs as the editor) and AWEB (which uses the bottom
right corner between the scrollbars as a button to launch your preferred 
editor). This is also typical behaviour of UNIX apps, where there are cannonical
environment variables to support this already.

Reassigning QA to se, since this is not Bindu's area (right?).
Comment 8 ravi 2001-03-20 08:03:41 PST
*** Bug 72539 has been marked as a duplicate of this bug. ***
Comment 9 David Coppit 2001-07-10 13:36:41 PDT
I think we could all agree on the benefit of this request. However, I think it 
is wrong to mark bug #72539 as a duplicate of this one. I desperately want the 
ability to shell out to another editor when composing emails, but am willing to 
wait for the more general solution described here. This suggest subsumes 72539, 
but that doesn't mean that 72539 shouldn't also be considered for the near-term.

Here's my prediction: no one is going to implement this any time soon because 
of the nontrivial effort required, and in the meantime we all curse the buggy 
editor we already have. Instead, with 5% of the work we can get 90% of the 
benefit by allowing people to set an external editor preference. (At least I 
spend 90% of my typing time in Mozilla composing emails.)
Comment 10 ravi 2001-07-11 09:17:47 PDT
i would agree with david coppit that the ability to use an
external editor in mailnews is significant enough (along
with being a standard "way of life" on unix systems) that
it merits particular consideration and development, apart
from the larger development effort it might be a part of.
i realize(d) that the developers have only limited time
and hence i am willing to wait for the larger issue to be
resolved if that is the only way to get the external editor
functionality.
Comment 11 Dave 2001-07-19 12:58:19 PDT
*** Bug 91513 has been marked as a duplicate of this bug. ***
Comment 12 Peter Lairo 2001-08-28 01:18:50 PDT
This bug would be less important if the text editing of Mozilla wasn't so horrible.
Comment 13 Dave 2001-09-05 14:47:48 PDT
*** Bug 98402 has been marked as a duplicate of this bug. ***
Comment 14 R.K.Aa. 2001-09-21 10:38:02 PDT
*** Bug 100972 has been marked as a duplicate of this bug. ***
Comment 15 Kevin McCluskey (gone) 2001-10-04 16:30:38 PDT
Build reassigning Buster's bugs to Marc.
Comment 16 Akkana Peck 2001-10-22 18:28:45 PDT
See also bug 103767, which covers the same issue.
Comment 17 Matt C. 2001-12-13 12:24:28 PST
I would also like to stress that bug #72539 is more than just a duplicate of
this one. It shares a basic likeness, but serves a more immediate (and perhaps
popular) purpose.

Free #72539!  :)
Comment 18 Rich Williams 2002-03-21 04:41:17 PST
Created attachment 75373 [details] [diff] [review]
first attempt at adding external editing

This patch adds external editing, roughly as described in the original
report. It's my first real attempt at adding anything to Mozilla, so
it's sure to be full of mistakes etc. I think the main thing wrong
with the patch is that it adds way too much code to
nsEditorCommands.cpp - most of the code should probably be in a
separate file.

This patch adds a new editor command, binds it to control-# for
textareas and editors (you can use it to compose e-mails) - bindings
are unix only currently, since that's all I can build/test. The
command writes the contents of the editor to a temporary file
(text/plain - iso-8859-1), starts a new thread, runs the external
editor, waits for it to finish and then replaces the text.

- VISUAL/EDITOR must be a full path currently.
- editing HTML doesn't quite work - it's saved as text and comes back as text
- there are probably platform specific issues

I've tested this on Linux (RH7.2) the latest cvs, with gnuclient and
it works for me - I'm writing this in emacs (in fact, I noticed a
mistake, and re-edited it!)
Comment 19 Rich Williams 2002-03-22 12:09:20 PST
Created attachment 75630 [details] [diff] [review]
second attempt - edits text/html now

This patch now edits html correctly - although for
some reason (which I haven't really looked into) if
you change things outside the body, the edits don't
seem to take. 

[attachment 75373 [details] [diff] [review] is obsoleted by this, but I don't
have permission to make that change]
Comment 20 Akkana Peck 2002-04-02 18:29:58 PST
Created attachment 77371 [details] [diff] [review]
Minor changes

This is really very cool.  I had a few problems with it, and I've taken the
liberty of updating the patch to show my changes -- take a look and see what
you think about my changes, and attach a new patch if you have updates or if
you disagree with what I changed (in that case I'll mark mine obsolete).

1. The keybinding didn't work for me because on my keyboard, # is a shifted
key, so I needed shift, added to the modifiers in the binding.	(Yes, our key
bindings should offer a way of ignoring that, but unfortunately they don't
yet.)  If # is an unshifted key on your keyboard, my suggestion is to have
both, your unshifted bindings and my shifted, listed in the bindings file.
2. NULL doesn't work on all platforms for pointers, so use 0 or nsnull instead;
our usual convention is to use !variable instead of variable == 0, so I changed
it to that.
3. Our coding conventions say classes (even hidden ones) should be named
nsClassName.  (I've seen a few cases where moz has been substituted, if you
strongly object to using ns.)
4. Used PR_GetEnv instead of the Unix-specific getenv, and added a mac default
of bbedit.  Unfortunately the default of "vi" won't actually work since our
launcher doesn't search $PATH.	I'm tempted to argue for /bin/vi or
/usr/bin/X11/gvim, but that will probably only work on linux (maybe not all
linux) so I won't argue too hard for that.  I'd also suggest that it would be
nice to check a moz pref in addition to the environment variable, but that's a
very minor issue and could wait 'til later.
5. A minor bummer is that there seems to be no way to run a gui editor -- all I
can do is run vi in the window where I started mozilla (which wouldn't be
useful if I started mozilla from a windowmanager taskbar or menu).  If I set
VISUAL to /usr/bin/X11/gvim, it forked immediately and
nsExternalEditorLauncher::HandleEvent was called before the window even came
up.  If I set VISUAL to "/usr/bin/X11/gvim -f", that didn't work because it
treated the whole string as a filename and nothing was launched.  I suspect
this may limit usefulness of the feature for a lot of people (though it's still
better than nothing).  Do our launcher classes really provide no way to search
the path and include arguments?
Comment 21 Christopher Hoess (gone) 2002-04-02 19:15:30 PST
Reassigning to Rick, who's provided a patch.
Comment 22 Hixie (not reading bugmail) 2002-05-16 10:15:57 PDT
Will this work if my $EDITOR is emacsclient?
Comment 23 R.K.Aa. 2002-05-23 14:04:36 PDT
*** Bug 146497 has been marked as a duplicate of this bug. ***
Comment 24 Dan Allen 2002-05-29 06:25:14 PDT
If this was in mozilla 1.0 I will have died and gone to heaven.  On a PII 333
typing in textareas of large posts is slow and annoying, and I make tons of
spelling errors because if it...If I could type in an external editor, all my
worries would be gone!!!  This is very key.
Comment 25 Akkana Peck 2002-05-29 09:50:38 PDT
We need a reviewed patch before we can move on to the next step.  Rich, can you
review my changes to your patch, or, if you want to change anything, post
another patch and I'll review it?  Or does someone else want to review?  One we
have a review we can move on to the next steps (getting an sr, then checking in).
Comment 26 Rich Williams 2002-05-29 10:04:04 PDT
Looks like some other changes broke some of this (string +
file/filespec) stuff. I fixed those things up and I've got another
patch now - it still doesn't fix the PATH / searching thing, but
perhaps that can wait for now. 

(I'll attach a new patch shortly)
Comment 27 Rich Williams 2002-05-29 10:35:53 PDT
Created attachment 85456 [details] [diff] [review]
slight changes to string / file / filespec code

Ok, this is based on patch 77371, but has minor changes made to 
make things compile again (some slight string/file/filespec changes)

Still doesn't search the path, or break out arguments.

akkana can you review please?

(this obsoletes 77371, but I couldn't make that change)
Comment 28 Wolfgang Rosenauer [:wolfiR] 2002-06-20 05:24:41 PDT
I built mozilla branch 1.0 now with the latest patch. 
The editor (gvim) is started and the text is copied.
But the Text is not written back to the textarea if I save and exit
gvim :-(
The return-code should be 0, so I assume a bug in the patch?
Comment 29 Rich Williams 2002-06-20 07:46:58 PDT
I just installed gvim to reproduce this - gvim forks at startup
unless you run it with the -f option. Since this patch doesn't
parse arguments at all, you'll have to specify the 'f' option in
guioptions in your .vimrc file to make it work with gvim at the
moment.

(I'm writing this in gvim)
Comment 30 Akkana Peck 2002-06-21 18:08:27 PDT
[typing this in gvim :-]

It works, though it took me a while to figure out how ... apparently it doesn't
work if I setenv VISUAL 'gvim -f', because the code gets an nsILocalFile based
on the whole string, not just the first word.  The only way I found was to write
a script that calls gvim -f, and setenv VISUAL vimscript. It would be better if
it only used the first word when getting the nsILocalFile, so users didn't have
to take the extra step.

I might have made the tmp file base name something a little more unique like
"nsedit" instead of "edit", but CreateUnique should ensure that there isn't a
conflict so that's not very important.

More worrisome, I get asserts:

###!!! ASSERTION: nsEditor not thread-safe: 'owningThread ==
NS_CurrentThread()', file nsDebug.cpp, line 533
###!!! ASSERTION: nsEditor not thread-safe: 'owningThread ==
NS_CurrentThread()', file nsDebug.cpp, line 533

(I get the one about nsEditor only the first time in a session, but the
nsProcess one every time I edit.)

It seems to work despite the assertions, but I'd feel more comfortable if we
understood why these were happening. Cc'ing some potential sr'ers who might also
know more about this assertion.

Finally, I'm not sure I understand the lifespan of the nsExternalEditorLauncher*
eel allocated in line 925. When it's created it does a NS_INIT_REFCNT (which I
notice is defined in nsISupportsObsolete.h -- I'm told that means you should use
NS_INIT_ISUPPORTS instead, so you should probably change that) which sets the
ref count to 0. It's addref'ed once at the end of Run.  It's released once in
DestroyEELEvent. So it looks like the counts are balanced and there's no memory
leak, except that from creation to when the process is being run, the refcount
is zero. Did I miss any?

A better model would be to addref it in nsLaunchExternalCommand::DoCommand just
after creating it, and remove the addref in Run.

I'd give it r=akkana with those two changes (REFCNT to ISUPPORTS and move the
addref) if anyone can put my mind at ease regarding the non-thread-safe
assertions -- are those something we need to worry about?

(Hmm, vi wrapped at my usual wrap column -- I hope that doesn't mean this is
going to go to bugzilla with long-short lines ...  I'd better go back into vi
and take out the linebreaks.)
Comment 31 Rich Williams 2002-06-22 04:46:31 PDT
Created attachment 88771 [details] [diff] [review]
refcnt to isupports / moved addref

Ok, I've changed the NS_INIT_REFCNT to NS_INIT_ISUPPORTS and I've
moved the addref. Fixing the 'gvim -f' problem is going to need a
bit more string manipulation to break out the arguments, which I 
think we can address later.
Comment 32 Akkana Peck 2002-06-24 13:48:55 PDT
I'm still hoping to get a comment from someone regarding the thread-safety assert.

Dbaron (on irc) says that these asserts are probably meaningful, and will mean
random crashes especially on dual-CPU machines due to refcounts being incorrect.

NS_CheckThreadSafe in nsDebug.cpp has a long comment in it saying that thread
safety checking is turned on temporarily, until we branch, and we're accepting
the consequent slowdown on linux for safety but will turn it back off after the
branch.  This comment was written on 3/8/2000 by Warren.

What's the deal?  Adding some more folks who might know something about
nsProcess::Run and its thread-safety or lack thereof.  

Also, shouldn't we change that Warren comment if we're intentionally leaving
checking on by default?  Anyone know how much of a slowdown we're actually
seeing?  Shouldn't we turn it off at least in non-debug builds?
Comment 33 Doug Turner (:dougt) 2002-06-24 14:42:41 PDT
The problem is that someone is accessing the nsEditor from a thread other than
the thread that created the nsEditor.  Most of time, this assertion points out a
race condition or worse.  Identify where this object is being instantiated and
then accessed.  Think about why this is happening and if it should be avoided. 
Also think about if you can get away with only the nsISupports interface needs
to be implemented in a thread safe way, or if your object totally need some work.  


yes, we should remove the comment as this stuff is going to stay on in debug builds.
Comment 34 Wolfgang Rosenauer [:wolfiR] 2002-06-28 03:41:40 PDT
I'm using recent 1.0 branch builds together with this patch.
I would suggest to create files with mode 0600. Not with 0700.

The main problem is, that (at least on my system) the /tmp/edit* files are
not deleted after successful editing.
Comment 35 Akkana Peck 2002-07-01 10:13:59 PDT
Yes, Wolfgang, good point.  I also noticed that, but then forgot it when making
review comments.  We should unlink the temp file after reading it.
Comment 36 Rich Williams 2002-07-01 10:53:27 PDT
Created attachment 89803 [details] [diff] [review]
remove temp files / better file permissions

Ok, this patch should fix those problems - I think originally
I was using a different file object which auto-deleted it, since
I'm sure I remember it working properly once. 

I've put the ref counting back how it was - from comment #30, the
answer is yes, you did miss one - I'd forgotten how it worked as
well. 

The nsIThread holds a reference to nsExternalEditorLauncher as well -
so it's only at refcount zero between the constructor call and the
call to eel->Init. I could put a addref/release around the call to
Init, but it didn't seem worth it. Let me know if you disagree.

The addref to eel->event is to keep the nsExternalEditorLauncher alive
until the event was processed (which is why it was released in the
destroy event). When Run returns, the object is released by the
thread, and cleans up properly.
Comment 37 Boris Zbarsky [:bz] 2002-07-01 21:12:45 PDT
OK... a few minor things:

> +    mFile->Remove(false);

PR_FALSE, please

> +  // create a nsILocalFile for that - ideally should search PATH?

Yeah.  For one thing, the case when EDITOR/VISUAL are not set will fail with
this code...

You want to check that |app->Exists()| before trying to launch it, no?  May also
want to check that it's IsExecutable().

> +  // convert it 

The assumption that the data is all ASCII is pretty major... I don't think we
can assume that...  

> +  // insert it back into the editor

Assert on an unknown editor type so it'll be easier to debug if we ever have a
third editor type?

> nsLaunchExternalCommand::DoCommand

Check that you're getting the command you think you're getting?

All those error returns from nsExternalEditorLauncher::HandleEvent() look like
what to the user?  They should at least tell the user the name of the file the
data is in so that they can try to recover it, no?

Major problem, mo:

The nsExternalEditorLauncher has a ref to mThread, right?  And mThread has a ref
to the nsExternalEditorLauncher, right?  How are they ever going to go away?

Comment 38 Boris Zbarsky [:bz] 2002-07-01 21:14:31 PDT
Almost forgot.  There's some really dumb PATH-walking code in
uriloader/exthandler/unix/nsOSHelperAppService.cpp.  It may make sense to
prettify it some, make it a little more careful, and put it somewhere where this
code can use it too...
Comment 39 Wolfgang Rosenauer [:wolfiR] 2002-08-29 02:32:39 PDT
is there a patch available for Moz 1.1?
The patch applies after a few format modifications but it doesn't compile.
Comment 40 Mats Palmgren (vacation) 2002-09-01 18:52:44 PDT
*** Bug 166100 has been marked as a duplicate of this bug. ***
Comment 41 u69748 2002-10-20 01:58:14 PDT
Does this bug concern about mail composer too?
If so, please change the Summary suitable one.
Comment 42 timeless 2002-10-20 05:48:00 PDT
Comment on attachment 89803 [details] [diff] [review]
remove temp files / better file permissions

>Index: editor/libeditor/base/nsEditorCommands.cpp

+#ifdef XP_MAC
+NS_NAMED_LITERAL_STRING(DEFAULT_EDITOR, "bbedit"); /* does this really work?
*/
+#elseif defined(XP_BEOS)
+NS_NAMED_LITERAL_STRING(DEFAULT_EDITOR, "StyleEdit"); /* this has the gvim
forking problem when StyleEdit is already running. (it works fine if StyleEdit
isn't running.) Someone can read the docs and solve it later. */
+#elseif defined(XP_UNIX)
+NS_NAMED_LITERAL_STRING(DEFAULT_EDITOR, "vi");
+#elseif defined(XP_WIN)
+NS_NAMED_LITERAL_STRING(DEFAULT_EDITOR, "notepad");
+#elseif defined(XP_OS2)
+NS_NAMED_LITERAL_STRING(DEFAULT_EDITOR, "ped"); /*i think this is right, i'll
check later */
+#endif

the following comments are no longer relevant:
if (!editor /*add:*/|| !*editor) /*nspr's getenv documentation talks about how
barely it wraps the native version and that you can't rely on certain
behaviors.

case: "FOO="	   GetEnv FOO could return 0 or ""
case: unset FOO    GetEnv FOO could return 0 or ""*/

/* for if () #if things you should wrap the block in {}s even if you only have
two. */

>+NS_IMETHODIMP
>+nsExternalEditorLauncher::Run()
>+{
>+  nsresult rv = NS_OK;
>+
>+  // decide which editor to use
>+  const char* editor = PR_GetEnv("VISUAL");
nsAutoString editor(NS_ConvertUTF8toUCS2(PR_GetEnv("VISUAL")));
>+  if (!editor)
>+    editor = PR_GetEnv("EDITOR");
if (editor.IsEmpty())
    editor = NS_ConvertUTF8toUCS2(PR_GetEnv("EDITOR"));
>+  if (!editor)
if (editor.IsEmpty())
    editor = DEFAULT_EDITOR;

/* isn't there also a /etc/mailcap option for this sort of thing? */

>+  // create a nsILocalFile for that - ideally should search PATH?
>+  nsCOMPtr<nsILocalFile> app;
>+  rv = NS_NewLocalFile(NS_ConvertASCIItoUCS2(editor), 
>+                       PR_FALSE, getter_AddRefs(app));
  rv = NS_NewLocalFile(editor, PR_FALSE, getter_AddRefs(app));

>+  // check the exit code
>+  PRInt32 exitCode;
>+  rv = mProcess->GetExitValue(&exitCode);
>+  if (NS_FAILED(rv))
>+    return rv;
>+  if (exitCode)
>+    return NS_OK;
shouldn't we log the failure somewhere? jsconsole would be a good choice.

>+  // create a new event
>+  nsExternalEditorLauncherEvent* event = PR_NEW(nsExternalEditorLauncherEvent);
i'm not sure i like the use of PR_NEW(someClass). given the way you're using
it,
a. why not use |new|
ai. you could write a constructor which takes eel as a param
or
b. label the class as a struct which is essentially how you're using it (yes i
know structs are basically classes whose default access is public, but at least
mallocing structs doesn't look wrong).

what you have:
+class nsExternalEditorLauncherEvent : public PLEvent
+{
+public:
+  nsExternalEditorLauncher* eel;
+};

>+  if (!event) 
>+    return NS_ERROR_FAILURE;
>+  PL_InitEvent(event, nsnull,
>+               (PLHandleEventProc) ::HandleEELEvent,
>+               (PLDestroyEventProc) ::DestroyEELEvent);
>+
>+  // put a pointer to ourselves in there
>+  event->eel = this;
>+  NS_ADDREF(event->eel);
>+  // post the event
>+  rv = queue->PostEvent(event);
>+
>+  return rv;
>+}

>+nsresult
>+nsExternalEditorLauncher::HandleEvent() {

>+  PRUint32 available;
>+  rv = inputStream->Available(&available);

>+  // read in the data
>+  char* text = new char[available + 1];
this is bad if available + 1 == 0. 

>+  if (!text) 
>+    return NS_ERROR_OUT_OF_MEMORY;
specifically, it's bad here:
>+  text[available] = 0;

>+  PRUint32 read;
>+  rv = inputStream->Read(text, available, &read);
this approach doesn't scale very well, why not put this into a loop and read
data in blocks of X bytes?
you might consider avoiding using malloc
>+  if (NS_FAILED(rv)) {
>+    delete[] text;
>+    return rv;
>+  }
>+  if (read != available) {
>+    delete[] text;
>+    return NS_ERROR_FAILURE;
>+  }
>+  rv = inputStream->Close();
>+  if (NS_FAILED(rv)) {
>+    delete[] text;
>+    return rv;
>+  }
>+
>+  // convert it 
>+  nsAutoString string;
>+  string.Assign(NS_ConvertASCIItoUCS2(text));
bz is of course correct. (simple example) on windows nt, notepad's output can
be UTF8 or UCS2. reading it in as Ascii and then converting it to ucs2 is
murder.  I'd suggest using an intl detection class to select the correct
encoding.
>+  delete[] text;
>+
>+  // select all the text
>+  rv = mEditor->SelectAll();
you can unify this with the early return case.
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  // insert it back into the editor
>+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(mEditor);
>+  if (htmlEditor) 
>+    rv = htmlEditor->InsertHTML(string);
/*this else makes me uncomfortable, how about returning here, i.e.*/
    return htmlEditor->InsertHTML(string);
>+  else {
>+    nsCOMPtr<nsIPlaintextEditor> plainTextEditor = do_QueryInterface(mEditor);
>+    if (plainTextEditor) {
>+      rv = plainTextEditor->InsertText(string);
>+    }
>+  }
as bz said, definitely add an NS_ERROR("unrecognized editor...") here.
>+  return rv;
>+}

>+NS_IMETHODIMP
>+nsLaunchExternalCommand::DoCommand(const nsAString & aCommandName, nsISupports *aCommandRefCon)
>+{

>+  // tell the editor to write the data out
>+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(editor);
>+  rv = editor->OutputToStream(outputStream, 
>+                              (htmlEditor ? NS_LITERAL_STRING("text/html")
>+                                          : NS_LITERAL_STRING("text/plain")), 
>+                              NS_LITERAL_STRING("ISO-8859-1"), 0);
for text/plain you can't make this assumption, it's perfectly valid for the
field to have Chinese, Hebrew, Japanese, Russian, or some other content
(perhaps all of the above).  For the html editor, however, it might actually be
best to ask it to encode all the non 8859-1 characters as entities (which i
suppose is what happens today).

Given that most editors on unix are probably 8bit, i guess UTF8 makes the most
sense. note that you can't assume that the editor will maintain the encoding or
that it will even do the editing. on one cvs system i never got around to
changing the editor cvs would spawn, so when i did a cvs commit, i would open
another connection, edit the file it created and save it. and then quit the
editor that cvs spawned. i'm not sure if this would work in your current
scheme, but i like it, and it would allow people to setup a dummy editor for
cases where their app forks/changes threads (gvim, StyleEdit).
>+}
Comment 43 Dave 2002-10-20 10:53:55 PDT
Emura, this bug concerns text areas. If mail and composer have text areas, it
concerns them also.
Comment 44 u69748 2002-10-20 20:54:59 PDT
What is "textarea" ?

When I read summary and comment #1, I could not image this bug
also concerned about mail composition. I thought this bug concerned
HTML FORM: <textarea> only.

Following bugs are marked as duplicated bugs of this bug.
We should clarify this bug covers those bugs in summary.

Bug 72539  [RFE] external editor for mail composition
Bug 91513  I would like to use a different editor to compose messages
Bug 98402  Please support using an alternate editor for the mail composer
Bug 100972  No support for external mail/html editor programs
Bug 166100  allow for the use of an external editor in (text only) mail composition
Bug 175475  Need external text editor interface for mail edit.
Comment 45 R.K.Aa. 2002-11-13 03:51:02 PST
*** Bug 174903 has been marked as a duplicate of this bug. ***
Comment 46 Boris Zbarsky [:bz] 2003-01-05 12:28:30 PST
*** Bug 187795 has been marked as a duplicate of this bug. ***
Comment 47 Jo Hermans 2003-05-30 13:20:29 PDT
*** Bug 207672 has been marked as a duplicate of this bug. ***
Comment 48 Prognathous 2004-02-18 09:05:48 PST
Until this bug is fixed, the Mozex extension can serve as a reasonable
workaround: http://mozex.mozdev.org

"Mozex is an extension which allows the user to use external programs for these
actions:

    * view page source
    * edit content of textareas (possibly utilizing a spell-checker in the text
editor)
    * handle mailto, news, telnet and FTP links
    * download files

Mozex works with both Mozilla and Firebird"

Prog.
Comment 49 Rick Bradley 2004-12-11 09:30:30 PST
Mozex appears to be unmaintained and no longer works with the 1.0 Firefox
extensions manager, evidently.  Launchy, the other oft-cited alternative, while
it supports "editors" appears to support them only for viewing source.  I ended
up at this bug precisely to get back the Mozex functionality that's gone missing.
Comment 50 Rick Bradley 2004-12-11 09:55:51 PST
In fact, I spoke too soon.  Sorry for the spam, but there is a repackaging of
Mozex for the new EM by a third party, here: 
http://www.extensionsmirror.nl/lofiversion/index.php/t70.html.
Comment 51 Gary Lawrence Murphy 2005-02-07 17:42:33 PST
mozex does indeed work with Mozilla 1.7 and FF 1.0 and also provides some
additional external-callouts (mailto, downloads, new protocols), but it's
awkward for the textarea purpose; you need to navigate a right-click menu
sub-option to engage, and left-click focus to close out the operation, a small
price to pay, but nonetheless not as smooth as it would be if intrinsic to the
mozilla code.

Mozex extensions are discussed in Bug 280608 (which also references this bug)
Comment 52 Gary Lawrence Murphy 2005-02-08 07:05:50 PST
*** Bug 280608 has been marked as a duplicate of this bug. ***
Comment 53 bugzill.20.ce 2006-10-16 06:29:42 PDT
This bug exists now longer then 7 years.. Does rdw@perldog.com still work on this one?
Comment 54 Ed Hume 2006-10-17 20:24:43 PDT
Now that Mozilla has broken into Firefox and Thunderbird, perhpas the bug discussion should be split.

But perhaps not: I would use t-bird if I could use MS Word as my text editor; and web-based mail would be easier if I could use Word with it as well.
Comment 55 Krishna Sethuraman 2007-01-07 19:29:39 PST
(In reply to comment #51)
> but it's awkward for the textarea purpose

Mozex currently supports choosing a hotkey to edit the textarea.  Works for me with xemacs's winclient.exe using Firefox under Windows XP to edit trac wiki pages.
Comment 56 Jesse Ruderman 2007-07-24 20:38:38 PDT
*** Bug 389409 has been marked as a duplicate of this bug. ***
Comment 57 zug_treno 2008-02-27 07:01:50 PST
*** Bug 241583 has been marked as a duplicate of this bug. ***
Comment 58 zug_treno 2008-02-27 07:02:01 PST
*** Bug 357065 has been marked as a duplicate of this bug. ***
Comment 59 Vicki Brown 2008-06-04 14:24:16 PDT
Mozex works (but it's complicated on Mac OS X, or was).

It's All Text works (although it "thinks in Unix" in terms of how to find the editor to use.)

Both prove that using an external isn't too difficult. If that code could be moved into the Core, then everyone could have this functionality for free without installing Yet Another Plugin.  And maybe I could have a textarea external editor in Camino where it is sorely needed (and plugins don't work.)

Please put this functionality into Mozilla. please please

Note You need to log in before you can comment on or make changes to this bug.