needsterminal flag in mailcap must be respected

NEW
Unassigned

Status

()

Firefox
File Handling
16 years ago
a year ago

People

(Reporter: robbe, Unassigned)

Tracking

(Blocks: 1 bug, {helpwanted})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

16 years ago
Some handlers in my mailcap file output to stdout, i.e. they need to be run
inside an X terminal. Example:

  text/*; less '%s'; needsterminal

Mozilla 0.9.7 (Linux) just runs the command passing on whatever stdout it might
have itself (often a log file or /dev/null)

I think the 'x-mozilla-flag' is already supported, so grokking another one
should not be too difficult. Sticking "xterm -e" before the respective command
should be neither. (Actually the exact terminal to use would better come from a
pref.)
install the psm package (via installer or download the mozilla-psm.rpm)

*** This bug has been marked as a duplicate of 47617 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE
v
Status: RESOLVED → VERIFIED
reopening.  Caillon, what does this have to do with PSM?
Status: VERIFIED → UNCONFIRMED
Resolution: DUPLICATE → ---
I've been meaning to do this anyway.  As you note, parsing for the flag is
trivial (that system is already in place).  The hard part is finding which
terminal to use....

I suppose we could look for "xterm", "eterm", "gterm", "kterm" in PATH?

Targeting to 1.0, but I don't really have the time to figure out how to do this
in a decently XP way.  Whichever method we use, it should work reasonably on all
Unices, BeOS, QNX, and VMS (these are the platforms that use the
mailcap/mime.types code).  Granted, BeOS should be using a separate module
anyway.... robbe@orcus.priv.at, any help you can offer in defining the correct
behavior would be much appreciated.
Assignee: law → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Ummm, weird....  when I duped this the bug had something about unknown socket
type errors.  Bugzilla has been displaying bugs incorrectly today (as bkor and
rkaa both have seen in #mozillazine).

Comment 6

16 years ago
Wouldn't it be better to look at the TERM environment variable?
Yes, it would.  What do we do when it's empty?

The final default, of course, is that we simply ignore the needsterminal
directive... or that the mailcap entry does not get counted as "matching". 
Which is better?

Comment 8

16 years ago
The more I think about it the more I realize that environment variables 
won't work. TERM might say xterm even for rxvt, etc. I think mozilla 
will have to have a pref with xterm -e as a default.

You might be able to shoehorn this in as a "helper app." That's what 4.x 
does. See the entries for telnet, etc.
TERM is completely out.  sshing into another machine will set TERM to vt100,
which is utterly divorced from what we want.

I would rather have mozilla do "the right thing" without user interaction as
much as possible, but allow users to override.  So if we have a pref it would
simply be an override for the codepath used by default and would be unset in new
profiles...

"The right thing" is starting up _some_ terminal with the app running inside. 
So we want to walk the PATH and look for commonly-used terminal apps.

I don't really like this solution, though... any other ideas?  :)

Comment 10

16 years ago
Walking the PATH is treacherous.

First, there's no reason to assume that mozilla's PATH is the same as 
the user's. You might find the wrong version of some program or not find 
what the user wants.

Then there's the question of actually walking it. There's no library 
function that let's you find a file. You either have to do an execvp (or 
one of its relatives) and deal with failures or you have to roll your 
own code. (See my latest attachment to bug 57866 for one way to do it. 
It's not as easy as it looks.)

Personally, I see nothing wrong with a default of "xterm -e" that can be 
overridden by the user. Aren't user prefs applied on a per-profile basis 
anyway?
> Walking the PATH is treacherous.

That's why I dislike my solution.  :)

Unfortunately, there is no good way to do the "xterm -e" solution without
running into the same problems...

Let me put it this way.  To start up an application from within mozilla in a
reasonable way I need two things:

1)  A full path to the application
2)  A char** of arguments to pass to the application

So for "xterm -e" these would be "/usr/X11R6/bin/xterm" and ["-e", handler]

The problem is storing this info in the prefs.  Storing full path is easy, but
this will vary by platform.  Storing switches is not....

Both xterm, rxvt, gnome-terminal use -e for starting up an app different from
the shell.  I have no other terminals here to test.  What do eterm/kterm use? 
Would assuming that it's always "-e" be valid enough to implement (possibly as a
separate pref)?

Comment 12

16 years ago
Why do you need absolute paths? The location of any executable is 
system-dependent and changeable. You can't assume that someone won't 
change the location of a given file. That's why PATH exists in the first 
place. As I said before, there are standard ways to do that.

I don't see the problem with prefs either. You can store an arbitrary
string in one and parse it any way you like.

I suspect that most xterm equivalents honor -e because xterm does. Now 
that I've thought on it a little more, it too simplistic to say that 
xteem -e will suffice. Many users find they can't read an default xterm. 
The fonts are too small/large. The window's the wrong size. It's the 
wrong color, etc.

BTW, you should consider passing your own environment on an exec.
> Why do you need absolute paths?

Because the accepted way to spawn off subprocesses in Mozilla is nsIProcess,
which uses an nsIFile, which needs an absolute path.  So at some point I need an
absolute path.

> That's why PATH exists in the first place.

Right.  Which is why I would need to look for the executable in the PATH just
like, say, the shell would.  See the code in 
uriloader/exthandler/unix/nsOSHelperAppService.cpp that already does this for
mailcap stuff...

> You can store an arbitrary string in one and parse it any way you like.

Sure.  Until someone comes along and assumes they can stick arbitrary shell
stuff in that pref.... :)  And no, code that uses system() is _not_ likely to
pass review.

I would rather not do a full tokenizer and parser to break up things like

"xterm -title 'foo bar baz' -geometry 500x30+20+20 -display :1.0 -e"

into things I can pass to the system.

> Many users find they can't read an default xterm. 

Isn't that what X resources are for?  So that all that info does not have to be
respecified on the command line by every friggin' thing that wants to use an xterm?
(Reporter)

Comment 14

16 years ago
My opinion is that walking the PATH trying to find one of a set of popular X
terminal emulators is probably still your best bet. Since the mailcap code
already has to find absolute paths for the real handler applications (cf. my
example which specifies "less" not "/usr/bin/less") this should not be too much
work.

Providing pref overrides is a must because if more than one emulator is
installed, the one we end up picking will invariably be the one some users hate.
Of course, if more distributors would adopt Debian's "x-terminal-emulator"
symlink ...

I'm sure most emulators support "-e" for compatibility, but there may be older
apps around that don't (CDE, SunView, OpenLook, whatever). So another pref for
the correct switch may be in order. Default: "-e". Finally, I agree with Boris
that providing for arbitrary switches for geometry etc. is too much. xterm
supports resources well, and many modern terminals have persistent preference
settings themselves. If all else fails, users should write a short shell wrapper
and use that.

Comment 15

16 years ago
Boris: where does mozilla spawn a process? If there's no way to use a
well-known library call like execvp then that's a bug. This is all
unix-specific code and it should be as efficient as possible.

I agree that parsing strings is a waste of time. System() _would_ be
the quickest solution but I know why people dislike it. You could do
something like this:
  const char *shell = PR_getenv("SHELL");
  if (!shell) shell = "/bin/sh";
  Argv[0] = shell;
  Argv[1] = "-c";
  Argv[2] = "exec xterm -random_args ...";
which handles all the shell stuff, but it *is* sleazy.

You're gonna hate me but I think the PATH stuff in
nsOSHelperAppService.cpp is buggy. I'll file a bug and assign it to you.

As for X resources, I don't think you can assume that resources for
user-initiated apps are always applicable to mozilla-initiated ones,
e.g. the resource equivalents of -sl, -tn, -wf, or -ziconbeep. Since
there are command line equivalents for all resources, the user may be
using command line args and the resources may well be suffering bitrot.

robbe: if you're going to require the user to write wrapper scripts in
certain circumstances then it's a simple step to require wrappers in all
circumstances and not handle needsterminal at all. :-)
> where does mozilla spawn a process?

First you Init() it:
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#66

Then you Run() it :
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#194

The code is pretty heavily macro-ized and #ifdef-ed

> If there's no way to use a well-known library call like execvp

I didn't say there is no way.... just that it requires more work to make sure
it's right.  I don't know all that would go into properly forking and exec-ing a
multi-threaded program on various unix flavors.  I don't even know if it's any
different than forking/exec-ing anything else.

> This is all unix-specific code and it should be as efficient as possible.

It's not exactly unix-specific.  This code also runs on VMS and others. 
Basically, all mozilla platforms other than Win32/MacOS/OS2 use this code.

> You're gonna hate me but I think the PATH stuff in nsOSHelperAppService.cpp is 
> buggy.

Why would I hate you?  It was a pretty quick hack at the time....  bugs on that
are welcome.  I _do_ think we should have all that in a library somewhere, though.

There are basically two parts to this bug:

1)  Add a flag to nsIMIMEInfo that indicates that a terminal is needed, parse
    for "needsterminal", and set the flag.  This part I can do easily.
2)  When spawning the helper, if the flag is set do something special.  This is
    the part we're discussing, and any help on this (including contributed or
    example code) would be much appreciated.  I just don't have all that much
    time to spend on this, unfortunately.  :(  But I _would_ like it to happen,
    for my own use.
Keywords: helpwanted
Target Milestone: mozilla1.0 → Future
Not happening until I finish thesis unless someone steps up and does it.
Blocks: 173106
QA Contact: sairuh → petersen
This is really a significant issue: without a terminal, running "less" as a
helper app doesn't do anyone any good (and can cause trouble, since the process
is running without any way to communicate with the user).

It would be great if Mozilla could be configured to spawm a terminal to run this
sort of helper (as discussed above) -- but in the meantime Mozilla should just
ignore "mailcap" entries with the "newsterminal" flag.  That way, it would skip
the entry for "less" and move on to some X-aware application that can handle the
display on it's own (i.e a later mailcap entry that doesn't have "needsterminal"
set).  Hopefully, that solution is a lot easier to implement.

Nathan

Actually, now that bug 78919 is fixed this might not be too bad to do (just
hardcoding xterm for the time being).  You're right that just having Mozilla
skip entries that have the needsterminal flag would be pretty simple too; if
someone is interested in implementing that I can point them to the code in question.
Depends on: 78919
Duplicate of this bug: 352373
Duplicate of this bug: 296199
Created attachment 326044 [details] [diff] [review]
skip needsterminal entries

While I was digging the mailcap handling code for some other issue, I saw this could be done very easily.
Attachment #326044 - Flags: review?(bzbarsky)

Comment 23

9 years ago
Bug 296199 has been marked as a duplicate of this bug, but fixing bug 57342 would also be a solution (and even better in this particular case of a text file). Now, I think these bugs are closely related.

One problem is that mailcap is used for different things. For instance, for a MUA with a text interface, something like

  text/*; less '%s'; needsterminal

is useful. Now, Firefox doesn't need a terminal to display some text: it could treat it like text/plain (possibly with the way to apply some filter in order to convert it into HTML/XML + CSS to benefit from coloring and so on). So, by default, instead of looking for a terminal, it could use its own window. Of course, some file formats need a conversion before they can be displayed; this is handled in 3 different ways in a mailcap file:

1. An entry with often a test like
    test=test -n "$DISPLAY"
No problem for Firefox: it can execute the application without needing a terminal...

2. An entry with the copiousoutput flag. In such a case, Firefox should run the tool and display the output in its own window.

3. An entry with the needsterminal flag, generally using an interactive text application (e.g. a pager like "less"). In such a case, Firefox needs a terminal.
So...  That doesn't actually fix this bug as filed (which is about running such commands in a terminal, not skipping them completely).

I agree that it's an improvement over the current behavior, but it should really go in a different bug.

Also, the indentation in the patch is clearly off (tabs?) and it would make it a lot easier to review if it had at last 8 lines of context.
Assignee: bzbarsky → nobody
QA Contact: chrispetersen → file-handling
Assignee: nobody → mh+mozilla
Oh, I just saw bz's comment... Mike, would you mind opening a new bug for your patch, as per bz's request? That will allow this fix to be reviewed quicker, and we can get it in the tree sooner. Thanks!
Assignee: mh+mozilla → nobody
Keywords: helpwanted
Priority: P3 → --
Target Milestone: Future → ---
Keywords: helpwanted
... oh, and thanks again for the fix. Really appreciate it! I had been wondering why "less" and other things were being proposed to me as open with handlers. ;)
Filed bug #442629
Attachment #326044 - Attachment is obsolete: true
Attachment #326044 - Flags: review?(bzbarsky)
Component: File Handling → File Handling
Product: Core → Firefox
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.