Open
Bug 120380
Opened 23 years ago
Updated 2 years ago
needsterminal flag in mailcap must be respected
Categories
(Firefox :: File Handling, defect)
Tracking
()
NEW
People
(Reporter: robbe, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: helpwanted)
Attachments
(1 obsolete file)
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.)
Comment 1•23 years ago
|
||
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
Closed: 23 years ago
Resolution: --- → DUPLICATE
![]() |
||
Comment 3•23 years ago
|
||
reopening. Caillon, what does this have to do with PSM?
Status: VERIFIED → UNCONFIRMED
Resolution: DUPLICATE → ---
![]() |
||
Comment 4•23 years ago
|
||
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
Comment 5•23 years ago
|
||
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 7•23 years ago
|
||
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?
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.
![]() |
||
Comment 9•23 years ago
|
||
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•23 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?
![]() |
||
Comment 11•23 years ago
|
||
> 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•23 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.
![]() |
||
Comment 13•23 years ago
|
||
> 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•23 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•23 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. :-)
![]() |
||
Comment 16•23 years ago
|
||
> 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.
![]() |
||
Updated•23 years ago
|
Keywords: helpwanted
Target Milestone: mozilla1.0 → Future
![]() |
||
Comment 17•23 years ago
|
||
Not happening until I finish thesis unless someone steps up and does it.
Updated•22 years ago
|
QA Contact: sairuh → petersen
Comment 18•21 years ago
|
||
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
![]() |
||
Comment 19•21 years ago
|
||
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
Comment 22•16 years ago
|
||
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•16 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.
![]() |
||
Comment 24•16 years ago
|
||
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.
Updated•16 years ago
|
Assignee: bzbarsky → nobody
QA Contact: chrispetersen → file-handling
Updated•16 years ago
|
Assignee: nobody → mh+mozilla
Comment 25•16 years ago
|
||
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!
Updated•16 years ago
|
Keywords: helpwanted
Comment 26•16 years ago
|
||
... 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. ;)
Comment 27•16 years ago
|
||
Filed bug #442629
Updated•16 years ago
|
Attachment #326044 -
Attachment is obsolete: true
Attachment #326044 -
Flags: review?(bzbarsky)
Updated•8 years ago
|
Product: Core → Firefox
Version: Trunk → unspecified
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•