Boris Zbarsky [:bzbarsky] Reporter
	Description • 23 years ago

Filing this to implement a UI redesign of the helper app dialog.  mpt, could you
post the spec here?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 23 years ago

adding to tracking bug

	Matthew T (active 1999-2002)
	Comment 3 • 23 years ago

Proposal:

+-----------------------------------------------------------------+
|:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::|
+-----------------------------------------------------------------+
|   .   The file "showattachment.cgi" is of type foo/bar, and     |
|  /!\  Mozilla does not know how to handle this file type.       |
|  """                                                            |
|       You can choose an application or plug-in to open the      |
|       file with, or you can save it to disk.                    |
|                                                                 |
|       [ ] Remember this action for files of this type           |
|                                                                 |
|       (_Save... ) (View as _Text)   ( Cancel ) ((Open With...)) |
+-----------------------------------------------------------------+

For Windows people, that is:

|       [[Open With...]] [ _Save... ] [View as _Text ] [ Cancel ] |
+-----------------------------------------------------------------+

`Open With...' closes the alert and opens a filepicker, starting at Mozilla's
plug-ins folder, for selection of an application or plug-in. Once an application
or plug-in is chosen, it is used to view the file.

`Save...' closes the alert and opens a filepicker for saving the file to disk.

`View as Text' closes the alert and shows the file in a Navigator window as if
it were text/plain (bug 57342). It does not have to be implemented in order for
this bug to be marked fixed.

`Cancel' closes the alert and does not download any of the file.

`Remember this action for files of this type' sets the pref if the user chooses
`Open With...', `Save...', or `View as Text'; such that the next time the user
opens a file of this type, she does not have to go through any alerts.

Possible problems:
*   How to make `Open With...' work with bug 57420, or if we should even try.
*   Whether the filename and type should be on lines of their own for
    scannability, or embedded in the sentence (as shown above) for aesthetics.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 4 • 23 years ago

> `Open With...' closes the alert and opens a filepicker

I'm not sure we can do that.... And I'm not sure we want to.  What if the user
cancels the filepicker?  The way I've seen filepicker used up to now:

1) we bring up filepicker
2) we check return value from filepicker and if it was not canceled and a file is
   selected we proceed.

So my suggestion here would be to keep the dialog open, open the filepicker,
then on OK from the filepicker close both.
> `Save...' closes the alert and opens a filepicker for saving the file to disk.

see my comments above

> `View as Text'

I think I know how to do this one....  on a one-shot basis.  Basically by
changing the MIMEInfo object to have type text/plain and to be handled
internally. I'm not sure how to make mozilla remember that it should always do
that for a given mimetype.

> `Remember this action for files of this type'

We just model this after the code in prefs that edits a helper app entry, I
think....

>  Whether the filename and type should be on lines of their own

Would it make sense to put them in the sentence but emphasize them (eg italics
or bold)?  Or is that a really bad idea?

	Matthew T (active 1999-2002)
	Comment 5 • 23 years ago

> What if the user cancels the filepicker?

Then we do nothing with the file. See my comment in bug 40555.

> then on OK from the filepicker close both.

That would be icky. See Blake's comment in bug 40555.

> Would it make sense to put them in the sentence but emphasize them (eg italics
> or bold)?  Or is that a really bad idea?

Generally, if you use unusually emphasized text in an alert, you're doing
something wrong.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 6 • 23 years ago

I guess my concern is the following scenario (there are a few variations on it):

1)  User opens a URL and gets our dialog.
2)  User picks ((Open With...)).3)  User browses for something to open it with
and does not find anything.
4)  User hits cancel.
Now what happens?

There's a separate technical question: is it possible for the dialog to go away
but for js from the dialog to keep executing?

	timeless
	Comment 7 • 23 years ago

I agree bz's view that we should keep the dialog. one reason is to remind the 
user about the file that they are trying to open.

However I'm not going to demand this for macos, I haven't thought about it 
there enough.

On windows, Start>Run>Browse leaves a window trail.
as does Start>Settings>Control Panel>Add/Remove Programs>Add/Remove Windows 
Components.
as does Open With>Other.

	sairuh (rarely reading bugmail)
	Comment 8 • 23 years ago

was going thru my email and i got a quick mockup from german, which i'll attach
here soon [since it didn't make it to a public forum]. he's currently on
vacation till after 25 june, after which he said he'd be updating his spec on
mozilla.org.

	sairuh (rarely reading bugmail)
	Comment 10 • 23 years ago

had a mtg today concerning this dialog. here's what i picked up [other folx who
attended: pls correct me if i misinterpreted stuff. thx!]:

1. looking at/going by german's proposed ui:

* remove the "Open (using XXXX)" radio button. instead, just prefill the "Open
with..." textfield, if the helper app is already defined.

* if the user changes the helper app [via the Choose button], that setting will
*not* persist. if user wants such a setting to persist, s/he should change it
either via the Helper Applications preferences panel, or thru the Advanced
button [see next point].

* add an Advanced button to the dialog. this would act similarly [iirc] to the
Set Default button in the current dialog UI.

* the "Always show this dialog..." checkbox should be selected by default.

2. ben will work on the front-end, and mscott will work on the back-end.
[subject to change. *shrug*]

3. anything else i'm missing concerning this particular feature...?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 11 • 23 years ago

> 3. anything else i'm missing concerning this particular feature...?

1) German's ui proposal does not include a decent way to "view as text" which
   would be an incredibly useful feature to have.

2) How does unchecking the "always ask" checkbox interact with handlers that we
   get from the system mime registry?  What do we want to do with such handlers
    -- always prompt, never prompt, sometimes prompt based on other info from
   the system?

4) If the dialog comes up because we have no helper defined and the user selects 
   a helper and unchecks the "always ask" box, we _should_ persist the helper
   selection.  Using the Advanced button to get mozilla to remember your helper
   selection seems very counter-intuitive to me.

Just my thoughts.

	sairuh (rarely reading bugmail)
	Comment 12 • 23 years ago

mscott has filed bug 88066 for a Quick Redesign of the helper app dialog...

	Matthew T (active 1999-2002)
	Comment 13 • 23 years ago

> 4)  User hits cancel.
> Now what happens?

We do nothing. In the unlikely event that the user has no app to open the file 
with but *still* wants to download it, she can click the link again (or choose 
`Reload').

> german's proposed helper app-downloading dialog

Unfortunately, that design perpetuates the the basic mistake (also present in 
the current alert) of using radio buttons for actions. Not only does this 
confuse the usual meaning of a radio button, it also requires two clicks (radio 
button, then `OK' button) instead of one to make a choice in the alert.

But enough of that, that's bug 88066. Boris, are you able to implement the 
design in this bug?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 14 • 23 years ago

I think I can implement everything except persisting the "View as Text"
option...  that would take some work.

	Matthew T (active 1999-2002)
	Comment 15 • 23 years ago

Even without `View' as Text', it would be a huge improvement over what we have 
now. Go for it!

	Ilgaz Öcal
	Comment 16 • 23 years ago

Why don't use system mime database at all? It isn't a multiplatform issue, so if 
system has a database for MIMEs somehow (Like Windows or previous NS 4.x) 
browser should use that info somehow and shouldn't ask what to do with file.
E.g. asking what to do with a .cgi as given example shows how annoying it can 
be.
As Nav. 4.x users could have a choice to _manually_ set a mime type if they are 
advanced enough and they could save a live radio to disk etc.

PS: Adobe has a "save a copy" option on its PDF viewer plugin (5.x)

Now the point is, if user _granted_ a plugin/helper app to install, they don't 
want to get that question.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 18 • 23 years ago

I'm not going to get to this anytime soon.  I've given it a shot or two, but I
can never seem to get it to work cleanly...  :( Over to law.

	Bill Law Assignee
	Comment 19 • 23 years ago

->mozilla1.0 for more triaging

Can somebody summarize the current status of this feature request,please?

	Peter Trudelle
	Comment 20 • 23 years ago

->098, for consideration as part of helper app feature work for MachV

	Bill Law Assignee
	Comment 21 • 23 years ago

The proposal here doesn't seem to say anything about *remembering* the
application chosen via Open With... (except in the case where the "Remember this
action for files of this type" checkbox is checked).

Wouldn't it be useful to, at a minimum, have the Open With... button open the
file picker to the application last chosen for this file type?

Would it be better still to offer a faster path (no file picker) to run the same
application as chosen previously?

What about helping the user find the application that is associated with this
file type via the OS settings?

	Peter Trudelle
	Comment 22 • 23 years ago

This should really be discussed in the newsgroup, before we start quoting each
other, but I'll try to address Bill's points in order without it:

1) Remembering the associated app is implicit when users say 'Don't ask me
again'. Using several different apps to open the same type of file is the <1%
case, so we should provide a greased path (i.e., minimal clicks, no 'Advanced'
or other dialog needed) for letting the user specify the app once, then stop
bothering them.

2) Going to the file in the open dialog would be a slight improvement, but why
should we even demand that users find the apps they want in the file system?
Hardly any of usknow where our apps are stored.  We should instead present a
list of apps, as Windows Explorer (Open With...) and Finder (don't remember what
Apple calls this dialog) do with unknown file types.

3) If users have previously selected an app to use to open the file type, but
then checked "Ask me every time", then sure, a fast path to that choice would be
nice.

Not sure what OS settings you mean, but if its similar to what I mentioned in
(2), then yes.

	Bill Law Assignee
	Comment 23 • 23 years ago

*** Bug 111035 has been marked as a duplicate of this bug. ***

	Bill Law Assignee
	Comment 24 • 23 years ago

Please see the proposed "redesign" in bug 111035 (which I've closed as a 
duplicate of this one).  It solves the problems I see with this proposal and 
accomplishes much of the desired improvement, I think.

I really need a concrete proposal that everybody is happy with.

	Akkana Peck
	Comment 25 • 23 years ago

It would be wonderful to get the fixes mentioned in bug 111035.  The biggest
problems with the current setup aren't design problems, they're things that
claim to work but are broken (e.g. "always ask" being present but ignored).  The
proposal in 111035 would fix that.

The New Type dialog does have a design issue, but that's covered by bug 88458.

	Peter Trudelle
	Comment 26 • 23 years ago

I largely agree with Sarah's summary in bug 111035, with one exception, a subtle
interaction detail that I think is important.  When the user has chosen an app
to handle the type, the checkbox should immediately default to remembering the
decision, so the user is not subject to any further interrogation.  This would
require a dialog more similar to German's proposal, the Choose dialog would have
to return to the helper app dlg, which would already have the checkbox set so as
to remember the choice.  This would also require adding OK/Cancel buttons to
dismiss the helper app dlg.  This would provide a greased path for the 99% case
where the user always wants the same app to handle that file type.  It also
allows for handling  the case where the user sets the checkbox so as to
remember, then cancels out of the choose dlg.  Also, I still think the Choose
dlg should not be a filepicker, since that requires users to either know where
the apps are in the file system, or to hunt around for them.  Instead, we should
present a list of valid choices.

BTW, Bill: we do *not* have to make everyone happy, even if that were possible.

	Bill Law Assignee
	Comment 27 • 23 years ago

Not today...

	Bill Law Assignee
	Comment 28 • 23 years ago

Working on this stuff next.

	Peter Trudelle
	Comment 29 • 23 years ago

nsbeta1+ per ADT triage team

	Claudius Gayle
	Comment 30 • 23 years ago

adt triage: this says 'out' on the PRD does it still need to be fixed?

	sairuh (rarely reading bugmail)
	Comment 31 • 23 years ago

the 'out' on the PRD is, i believe, referring to something else that's more
broad/involved in terms of development. this particular bug, on the other hand,
could be resolved more quickly/easily, iirc (by removing the Advanced button,
per the two attachments). er, so i would hope. :)

	Bill Law Assignee
	Comment 32 • 23 years ago

I've finished this task, for the most part, and want to run the design by
interested parties before going any further.

This first screen shot is of the new and improved helper app dialog.  The
visible changes are:

1. Added a plain "Open it" choice (in addition to the existing "open with" and
"save to disk" choices).  This choice means "open with whatever default
applicattion is configured on your system," meaning in the Windows file type
list (registry), the Mac internet config settings, or Linux "mailcap" file
(note that I'm speaking of the Linux case in the abstract because I don't fully
understand how that works or what the correct terminology might be).

2. Changed the wording for the choices to "open," "open with," and "save to
disk."	This terminology is used consistently on the helper app pref panel and
dialogs (which I'll be attaching shortly).

3. Added a <textbox> for entering the application name (to be disabled on Mac).


Behavior changes are:
1. Upon initial display for a "newly encountered" content type (one for which
the user has never pressed Ok on this dialog), the "always ask me" checkbox is
*unchecked*.  This means that the user choice will be remembered, and used
without further prompting, the next time the user encounters data of this type.
 Of course, that will only happen if they push Ok.  If they press Cancel, then
we pretend it never happened.

2. After pressing Ok, an entry will be added to mimeTypes.rdf for this content
type (if no entry existed previously), and that entry will reflect the user
input on this dialog.  If we create a fresh entry, then the description and
extension fields will be blank.

3. For mime types for which we don't currently have an entry in our
mimeTypes.rdf data source, the default choice will be "open."  There will be an
exception on Linux where the default will be "save to disk" in cases where no
application is specified in the mailcap file.

4. Executable files will continue to go straight past this dialog to a
filepicker (after which the file will be saved to disk).

I'd like feedback on the changes to this dialog, particularly refinement of the
layout and wording.

	Bill Law Assignee
	Comment 33 • 23 years ago

This is a screen shot of the updated Edit Type dialog that appears when the
user clicks the Edit... button on the helper app pref panel.

Cosmetic changes:
1. The description and extensions attributes are now editable fields.
2. Added the plain "open" choice (as per the helper app dialog).
3. Changed wording on choices to match helper app dialog.
4. Replaced "Handled By" caption with wordier "When downloading a file of this
type, I want to:" prompt text.
5. Changed the New Type... button logic to open this same dialog (with title
"New Type") so the user can fully specify all the attributes.

The behavior of this dialog (in both "edit" or "new" modes) is unchanged.

I'm looking for comments on the layout or wording on this dialog.

	Bill Law Assignee
	Comment 34 • 23 years ago

re: comment 32

I forgot to mention that I removed the Advanced... button from the dialog.  It 
is no longer necessary since the changes always go directly to the 
mimeTypes.rdf data source.  The downside is that to enter extensions and/or a 
description for the type, you have to go to prefs.

	av (gone)
	Comment 35 • 23 years ago

I would make 'Open it' more specific, like 'Open with default application'.
Otherwise it may be taken as 'open with Netscape' (or whatever the product name
can be).

Logicwise, 'Open it' thing may not be needed at all. 'Open with' is sufficient
if it's text box will be prefilled with the default application.

	Bill Law Assignee
	Comment 36 • 23 years ago

Lastly, here's the new look for the helper app pref panel.

The big change is the removal of the "Reset" button.  This is now longer needed
because the things that used to get "reset" are now sucked in and converted to
regular mimeTypes.rdf data source entries.  That is, those mime types will
magically appear in this panel, at which point they can be editted in the
conventional way (to check the "always ask" setting, for example).

The only other changes here are in the labels.	Also, the text displayed next
to "Handling:" is different (to better match the open/open with/save wording in
the edit type and helper app dialogs.  This value will be one of:

 - Open files of this type
 - Open files of this type with %S
 - Save files of this type to disk

%S will be replaced with the "short leaf name" of the user-specified
applciation (e.g., "acrord32.exe").

	sairuh (rarely reading bugmail)
	Comment 37 • 23 years ago

whoops, something wrong with attachment 74195 [details] --get an xml parsing error...

anyhow, i agree with av in comment 35 --prolly best to remove the plain "open
it" radiobutton and use what we have now, the radiobutton with "open it with
[application]" followed by the "choose" button. (and for unknown types, use the
current "open with an application" etc.)

	Alex Bishop
	Comment 38 • 23 years ago

I agree with av that the 'Open it' option in the helper app dialogue (attachment
74183) should state the application that it will launch or, if that is not
possible, at least state something along the lines of "Open it with the default
application". For the Edit Type dialogue (attachment 74188 [details]) it should always be
"Open it with the default application" because, presumably, the default
application can be changed without this setting being altered (by changing the
OS default).

Going back to the helper app dialogue (attachment 74183 [details]), I don't think that the
file type ("WinZip File") needs to be in quotes. Also the MIME type
[application/zip] would look clearer in round brackets (there's another bug
filed about that but it would be simple to fix it here).

In the Edit Type dialogue (attachment 74188 [details]), I think "When downloading a file
of this type, I want to:" would read better as "When I download a file of this
type:". Also, for the checkbox, I'm concerned about the term 'handling'. How
about rephrasing it as, "Always ask me when I download a file of this type"?

I much prefer the new layout but I'm still partial to MPT's suggestion of
single-click buttons (comment 3).

I can't comment on the prefs panel (attachment 74195 [details]) as it doesn't seem to load
(I get an XML parsing error).

	Alex Bishop
	Comment 39 • 23 years ago

In response to av (comment 35) and sariuh (comment 37) about removing 'Open it'.

Imagine the following scenario: I click on a link to an MP3 and the dialogue
pops up. My default player is Windows Media Player but I take advantage of the
'Open it with' option to select Winamp. However, I check the 'Always ask' box.
The next time I click on an MP3, the dialogue will return. Will I be presented
with both the default 'Open it' option (WMP) and the 'Open it with' option
filled with Winamp? If so, then removing the plain 'Open it' option wouldn't be
viable.

I'm not quite up on how the helper app dialogue will behave, so forgive me if
I've got the wrong end of the stick.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 40 • 23 years ago

> Added a plain "Open it" choice

Hmm... So on Windows that does ::ShellExecute and on mac something similar...  I
guess it would make sense for mailcap code to return a MIME info filled in with
"useSystemDefault" for the preferredAction when we call GetFromMIMEType and
GetFromExtension and there is a helper?  Then we can enable the "Open" radio for
that value of the preferredAction on Linux (or XP and just have the
OSHelperAppHandlers for Win/Mac always return a mime info with that set as a
backstop).  We should coordinate this....  What time frame are you looking to
land this in?

> If we create a fresh entry, then the description and extension fields will be
> blank.

This seems like a bad idea.  Why not fill in the extension field from the MIME
info or if that's empty the extension from the URL?  Why not fill in the
description field from the MIME info?

	Bill Law Assignee
	Comment 41 • 23 years ago

sorry about that!

	Bill Law Assignee
	Comment 42 • 23 years ago

re: comment 35

Yes, I considered that.  But German's "spec" used "open" and "open with" and I 
thought "default application" might be a little to vague.  The Win32 desktop 
context menu uses "open" and "open with" the same way, sort of.  But I can be 
swayed on this one.  I think other people weighed in later, which I'll respond 
to shortly.

As for eliminating the plain "open" entirely: I was loathe to do that because 
then it becomes hard to switch back to "the default app" without knowing what 
the default app was.

	Bill Law Assignee
	Comment 43 • 23 years ago

re: comment 37

So how do you feel about losing the ability to switch back to the "default"?

Worse, the way the dialog was previously implemented, once you pushed "Choose" 
(and then OK in the file picker), you couldn't even change your mind before 
pressing Ok (you were "committed" to that application, or some other specific 
one).

We need an explicit choice (I think) to adequately distinguish this 
new "useSystemDefault" setting for the mime type.

re: comment 38

The wording change I can see.  I got weary so I didn't try to identify the 
default application (and I'm not positive we always can).

Your comment about the Edit Type dialog struck me as a little odd.  The default 
application is just as fleeting on the helper app dialog as it is on that one.  
Why the distinction?  I imagine it's because the pref dialog seems 
more "permanent" while what you choose on the helper app dialog is more of 
an "immediate" thing?  But remember that making changes on the helper app 
dialog will be exactly the same as making those changes on the Edit Type 
dialog, so I'm not sure the distinction is all that accurate or useful.

re: "WinZip File" not being in quotes

I didn't change that code at all.  I wasn't aware of any major bugs on that.  
Not that I can't change it, I suppose.  Likewise the square brackets vs. 
parentheses.

re: "When I download a file of this type:"

Sounds better; originally I used "encountering" rather than "downloading" so it 
better covered the case where the user just clicked on a link without any 
intent whatsoever to "download" (i.e., save to disk).  But I'll switch to your 
suggesting till a better one comes along.

re: single click buttons

Doesn't work, in my mind (unless you can answer the questions I threw out when 
first digesting that proposal).

Thanks for the input.

	Bill Law Assignee
	Comment 44 • 23 years ago

re: comment 39

There will always be the plain "open it" choice (soon to be "open it with the 
default application" or some such).  It would be the default choice the first 
time, "open with (winamp)" would be the default choice the next time.  And I 
agree with your conclusion (we need the plain "open it (with default app)" 
choice).

You seem to have grasped how it works just fine, which is a good sign, I 
believe :-).

	av (gone)
	Comment 45 • 23 years ago

Re: Alex Bishop comment #39

If you mean it would be useful to remember both default app and the one the user
used last time, why to limit ourselves to those two? We can just make the
textbox dropdown box and fill it with everything we ever used to handle this
mime type. And again, no need for plain 'Open it'.

	Bill Law Assignee
	Comment 46 • 23 years ago

re: comment 40

I concur with everything in your first comment.  I'm looking to land ASAP; just 
need to upload patch and get approval (plus fine-tuning based on feedback).  I 
might need some help with the Linux details (and Mac, also; I don't know 
exactly how that works but I think it will mesh with this).

There are other helper app bugs pending (the IsExecute business, among others) 
and I'd like to make sure I at least haven't made those worse (although I'd 
like to fix as many problems as possible while we're at it).

re:
>This seems like a bad idea.  Why not fill in the extension field from the MIME
>info or if that's empty the extension from the URL?  Why not fill in the
>description field from the MIME info?

Well, it's not a *bad* idea.  Just not as good as it could be, perhaps.

But I just fixed a bug late today where I was losing the description/extensions 
for some already existing mimeTypes.rdf entries.  It now does what you suggest 
for new ones, so maybe it was a bad idea :-).  The one concern I have is that 
we get an extension/mime-type mismatch and that ends up overriding an extension-
to-mime-type mapping in the OS settings.  But maybe we manage that problem 
already (I know it seems like deja-vu all over again)?

	Bill Law Assignee
	Comment 47 • 23 years ago

re: comment 45

I think that's going too far.  There is no infrastructure in the RDF that 
reads/writes the mime infos to manage a list of applications and we haven't 
time nor energy to put that in place.

nsIMIMEInfo has 3 states: useSystemDefault, useHelperApp, and saveToDisk 
(actually 4, but handleInternally doesn't apply).  There's only one set of 
attributes to store a specific helper app.

I think this newly-implemented scheme does as well as we can with what we're 
given.

Not that it isn't a good idea.

	Bill Law Assignee
	Comment 48 • 23 years ago

Here's the patch.  Boris, if you could take this out for a spin on your Linux
helper app test bed, it would be really cool.  You're probably the only person
capable of reviewing much of it too, if I could impose.

An annotated patch file (html with interspersed explanations of the interesting
stuff) is coming momentarily.

	Bill Law Assignee
	Comment 49 • 23 years ago

This should be much easier to digest.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 50 • 23 years ago

re: comment 46:

> I'm looking to land ASAP

Ok.  The reason I asked is that I may have no time to work no the Linux part of
this myself in the foreseeable future.   I'm certainly willing to answer any
questions you have and all that.

> The one concern I have is that we get an extension/mime-type mismatch

Hmm.  True.  So my suggestion of using the extension from the URL _is_ a bad
idea given that thought.  :)  The extension from the MIME info should still be
safe. After all, if we get to this point and we have a MIME info and nothing in
prefs for this type then the MIME info came from the system to start with.  So
unless the system has a mapping ".foo --> application/bar" and also a mapping
"application/baz --> .foo" we should be set.  I can't think of a way that any of
the 3 platforms involved would actually have such a mapping.

The reason I think we should put the extension in (which I realized I forgot to
mention in my original comment): if there is an extension in the MIME info and
we pick a non-default-helper app to run the temp file may need the extension to
be handled correctly by the app.

	m_mozilla
	Comment 51 • 23 years ago

> unless the system has a mapping ".foo --> application/bar"
> and also a mapping "application/baz --> .foo" we should be set. 
> I can't think of a way that any of the 3 platforms
> involved would actually have such a mapping.


I've seen something *simmilar* that I thought I'd toss out there in case anyone
makes a significant connection with some potentian problem in this helper app
business.

I'm not sure if Mac OS is normally aware of the gazillion file endings my
installation currently recognizes, or if installing Graphic Converter caused the
registration of butloads of image mime types. In any event, my OS 9 install had
oodles and oodles of obscure image types registered, along with their file
extensions. I'm talking about hundreds of image types I've never encountered in
real life.

Now that I'm on OS X, there isn't currently any UI to access that stored
extension to mime mapping, so I can no longer edit this information as easily
(requires booting into OS 9). However, I can still control which app opens the
indicated extension/mime-type.

I've run into several cases where a perl file gets recognized as a certain image
type due to the extension. My OS is firmly convinced that various types of perl
files are actually image files. If I do nothing, the OS wants to open them in
Graphic Converter. Instead, I tell the OS "open all files of this type in
BBEdit" (my text editor). Now the OS still thinks that they're image files, but
it tries to open that type of image into BBEdit. It works acceptably. The only
downside is that the icon in the Finder is a generic document icon instead of a
BBEdit text document icon.

The problem is that Mac OS X users cannot currently (readily) modify their file
extension to mime type mappings. They *can* modify their mime type to default
application mappings.... but may or may not know how to do so (get info on an
example item).

maybe not relevant, but just in case...

-matt

	Tim Powell
	Comment 52 • 23 years ago

re: comment 38

re: "WinZip File" not being in quotes

I didn't change that code at all.  I wasn't aware of any major bugs on that.  
Not that I can't change it, I suppose.  Likewise the square brackets vs. 
parentheses.

I'd leave the quotes. I believe that is important to set off the exact name. 
Switching to parentheses makes sense, though. The bug mentioned is bug 102354. 
Mark it a duplicate if you intend to fix it here.

I'm concerned about switching the behavior of the checkbox so you have to check 
it to make sure you see it again. This seems unsafe for security reasons and 
also because it breaks behavior of all previous releases and is the opposite of 
IE. I also have no idea how you'd go about making the dialog visible again if 
you wanted to. It would be more consistent with the other alerts in Mozilla to 
have a checkbox marked by default like this:

[x] Always ask before opening this type of file 

It should require explicit user action for Mozilla to remember the application 
associated with a file type. MPT's original suggestion had a similar reqirement.

	Peter Trudelle
	Comment 53 • 23 years ago

The scenario here is that the user encounters a file that the browser doesn't
know how to display, so the browser is asking what program it should use to help
it.  This isn't something typical users even want to see once, since they expect
the browser to be able to find some way to display any link they click on, so
they sure don't want to see it every time.  Once they have chosen a helper app,
they are extremely unlikely to ever want to change that association.  They
shouldn't have to take an extra, explicit action for the browser to remember
that they want Photoshop to open their .tiff files, for example. This isn't like
other alerts, which indicate an error every time they appear.  This is something
that is solved the first time, but would keep coming up, because the software
would be too stupid to remember what the user has already explicitly specified.
 If the program was really smart, it would detect that Photoshop was installed,
offer to use it the first time such a file was encountered, and remember it
without being asked. 

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 54 • 23 years ago

Ok..  I'll try to give this a run on the Unix side today or tomorrow.  I _may_
have time to review this before Friday, but if I don't, I won't be able to get
to it until sometime in mid-April (I'll be out of town till then).....

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 55 • 23 years ago

*** Bug 132993 has been marked as a duplicate of this bug. ***

	gabriel
	Comment 56 • 23 years ago

From bug 132993:

I click on a link with a specific filetype, e.g. an mp3

Th dialog pops up and I select 'open using' /usr/bin/xmms, and then de-select
'always ask'

I would now expect all such files to be opened with xmms. However, instead I am
now always asked for a file location to download the file to. Further, there is
no way to remove this association without editing prefs.js.


I hope the fix for this will address these issues.

	Bill Law Assignee
	Comment 57 • 23 years ago

What you just described is bug 93173.

	Bill Law Assignee
	Comment 58 • 23 years ago

Please don't invest time in reviewing this one.  Boris has already given me
feedback and I'm working on another patch, which I'll be attaching soon.

	Bill Law Assignee
	Comment 59 • 23 years ago

*This* is worth looking at to understand the changes.  I probably won't be
attaching another version of this file (since subsequent changes are mostly
just implementation fixes).

	Akkana Peck
	Comment 60 • 23 years ago

If I apply the patch, rebuild, remove /etc/mailcap and /etc/mime.types and pare
my ~/.mailcap down to a minumum to remove anything audio related, click the
button in the helper apps pref panel to clear the save prefs (I don't have any
audio types defined there), then go to a page and click on an audio link, it
lets me specify an app and everything works fine (I un-checked "always ask"). 
If I then go to another page and click on the same type of audio link, I get a
save as dialog, when it should just play the sound.  The only way to get it to
play the sound now seems to be to bring up the prefs dialog, clear the
file-opening prefs again, and start over.  The new type did not get added to the
list of types in the helper apps pref panel.

	Bill Law Assignee
	Comment 61 • 23 years ago

Akkan,
Something must have gone awry when you applied the patch.  There shouldn't even
be a "Reset" button on the Helper Applications pref panel any longer (I'm
presuming that's what you mean by "the button in the helper apps pref panel to
clear the save prefs").

Can you clarify: "If I then go to another page and click on the same type of
audio link, I get a save as dialog"?  Does that mean you see the "helper app
dialog" asking whether you want to open/save?  What are the settings on that
dialog (mime type, default radio button selection, helper app info, "always ask"
checkbox setting)?

	Akkana Peck
	Comment 62 • 23 years ago

No, I didn't get the helper app dialog asking what I wanted to do; I got the
save-as file selection dialog, which didn't give me a choice to open it in my
selected app or any other, and which doesn't show me mime type or the other items.

With the new model, if I've chosen a helper app or "save as" for a type, and
want to change it later, how do I get back the helper app dialog without the
Reset button?  Not that I liked the reset button (not very intuitive or
discoverable), but what have we replaced it with?

	Bill Law Assignee
	Comment 63 • 23 years ago

If you didn't see the helper app dialog, then one of the following is happening:

1. You're using old code and hitting the "isExecutable" bug that causes us to go
straight to "save to disk" (this is fixed with my patch).
2. You've got an entry in mimeTypes.rdf that indicates you want to save content
of that type to disk, and, you've unchecked the "always ask me" checkbox for
that type.

The "Reset" button is no longer needed because you can directly edit the "always
ask me" setting for each and every mime type individually (each that you've told
Mozilla how to handle).  It used to be that we ignored the "always ask me"
setting that you'd see in the Edit Type dialog and you could only reset that for
*all* mime types via the Reset button.

	Bill Law Assignee
	Comment 64 • 23 years ago

Here's Boris's comments on the patch v2.0.  I'm fixing everything.

Comments on the patch:

------------------------------------------------------------

---- pref-applications-edit.xul ----


> @@ -93,25 +129,22 @@
> var data = 0;
> if (handlerInfo.handleInternal == "true")
> data = 0;
> else if (handlerInfo.saveToDisk == "true")
> data = 1;
> +      else if (handlerInfo.useSystemDefault == "true")
> +        data = 4;
> else 
> data = 2;


I know you're just tweaking the code, but could we use pretty names like
nsIMIMEInfo.useSystemDefault for those constants (instead of "4")?


> +      // If not updating (i.e., a newly encountered mime type),
> +      // then update extension list and description.
> +      if (!entry.mUpdateMode) {
> +        var extCount           = { value: 0 };
> +        var extArray           = { value: null };
> +        info.GetFileExtensions(extCount, extArray);
> +        for (var i = 0; i < extArray.length; i++) {


This seems wrong.. first off, extArray is an object, not an array, no?
extArray.value is the array?

Second, if there are no extensions GetFileExtensions will return null
for the array and 0 for the count.  So you want to use
|i < extCount.value| in the loop, I think.



> +      handlerInfo.description = gContentType.value;


This is just using the existing code, but renaming gContentType to
something that doesn't get confused with gMIMEField (like gDescription,
maybe) would be good.  Even better would be changing the id on the
<textbox> too, as long as you're touching that code anyway....

gExtensionLabel is also misnamed now that it's a <textbox>, not a
<label>.


> +      else if (gHandlerGroup.value == "4")
> +        handlerInfo.useSystemDefault = true;


I realize this is also just following the existing code but giving those
radio elements sane values like "useSystemDefault" instead of magic
constants may be nice... (though I'd understand not doing it as part of
this patch).


> </hbox>
> +  </vbox>


Would it make sense to reindent the code in the <vbox>?  Or at least to
indent the <vbox> only by 1 instead of 2 so it's not lined up with the
<hbox> like that?

I can't run a Moz from over here, so I can't look at what the UI changes
look like in real life but they seem fine from the code....

---- pref-applications.js ----


> +      var neverAskSave = unescape(prefBranch.getCharPref("browser.helperApps
> .neverAsk.saveToDisk")).split(",");
> +      var neverAskOpen = unescape(prefBranch.getCharPref("browser.helperApps
> .neverAsk.openFile")).split(",");


getCharPref will throw an exception if the pref is not set.  This code
needs to handle that (by setting neverAskSave/neverAskOpen to empty
arrays if the relevant pref is not set or something along those lines).

Also, don't we want to split _then_ unescape in the loop in case there's
an escaped comma in one of the types (unlikely, but it'd do odd things
if it happened).


> +      // Don't need these any more!
> +      try { prefBranch.clearUserPref("browser.helperApps.neverAsk.saveToDisk
> "); } catch(e) {}
> +      try { prefBranch.clearUserPref("browser.helperApps.neverAsk.openFile")
> ; } catch(e) {}


We're assuming users don't go back to old Mozilla (or Netscape) versions
here, right?

---- pref-applications.xul ----


> +                  prefstring="pref.application.disable_button.new_type"/>

...

> +                  prefstring="pref.application.disable_button.edit"/>

...

> +                  prefstring="pref.application.disable_button.remove"/>


I know those are in the current code, but do we really need them for
anything?  All those buttons do is trigger stuff oncommand....

---- nsHelperAppDlg.js ----


> Another new method, this one simply returns whether the user has changed the 
> helper app since
> using the "Choose..." button.
> <pre>
> +    // Returns true iff the user-specified helper app has been modified.
> +    appChanged: function() {
> +        return this.helperAppChoice() == this.mLauncher.MIMEInfo.preferredAp
> plicationHandler;
> +    },
> +
> </pre>


This seems odd to me, since the oncommand() on the "Choose..." button
does not set this.mLauncher.MIMEInfo.preferredApplicationHandler.  Or is
it just the text of the annotation that's wrong?  The comment and code
jibe with how you use the function...


> +            this.mDialog.openDialog( "chrome://communicator/content/pref/pre
> f-applications-edit.xul",
> +                                     "_blank",
> +                                     "chrome,modal=yes,resizable=no",
> +                                     this );


Do we really want to pass |this| as the last arg?  Or
|this.mLauncher.MIMEInfo|?  


> +        if ( !helperApp || !helperApp.exists() ) {


Should we add "|| !helperApp.isExecutable()"?  Or is that just asking
for trouble?

---- nsHelperAppDlg.dtd----


> +<!ENTITY alwaysAsk.label       "Always show this dialog before handling f
> iles of this type">
> +<!ENTITY alwaysAsk.accesskey   "k">


Is it me, or is there no "k" in that label?


---- nsExternalHelperAppService.cpp ----


> +  else if (url)


So this is when we have both a MIME info for the _type_ we got on the
channel and a url to work with.


> +    // If neither description nor app are specified, then we try to get
> +    // these from the per-platform OS settings based on the file extension.
> +    if (defaultDescription.IsEmpty() && !defaultApp)
> +    {
> +      nsCOMPtr<nsIMIMEInfo> osInfo;
> +      url->GetFileExtension(fileExtension);


As at the beginning of the mail, why are we using the _url_ extension
instead of the extension that the MIME type mapped to?


> +        // Extract default application and default description.
> +        osInfo->GetDefaultApplicationHandler(getter_AddRefs(defaultApp));
> +        osInfo->GetDefaultDescription(getter_Copies(defaultDescription));
> +        // Copy to result mime info object.
> +        mimeInfo->SetDefaultApplicationHandler(defaultApp);
> +        mimeInfo->SetDefaultDescription(defaultDescription.get());


I'm a little confused.  Shouldn't GetFromMIMEType set this data (even
if it sets "useSystemDefault" as the handling method)?


> +  if (trueString.Equals(stringValue))
> +        aMIMEInfo-&gt;SetAlwaysAskBeforeHandling(PR_TRUE);
> +   else
> +    aMIMEInfo-&gt;SetAlwaysAskBeforeHandling(PR_FALSE);


That indentation is odd-looking.


> +      if (appl)
> +      {
> +        nsCOMPtr&lt;nsILocalFile&gt; file = do_QueryInterface(appl);
> +        if (file)


do_QueryInterface is null-safe, so that outer |if| is unnecessary.


> +      nsUnescape(NS_CONST_CAST(char*,(const char*)prefCString.get()));


See http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsEscape.h#159.
You may want to use those "string-friendly" versions here?


> +      prefValue = prefCString; // i should be able to avoid this string copy


See CaseInsensitiveFindInReadable in nsReadableUtils.h.  The code would
be something like:

nsACString::const_iterator start, end;
prefCString.beginReading(start);
prefCString.endReading(end);
if (CaseInsensitiveFindInReadable(aContentType, start, end))
  return false;


> +      PRInt32 pos = prefValue.Find(aContentType, PR_TRUE);
> +      if (pos >= 0)


If you _do_ keep this code, could you test |pos != kNotFound| instead?

---- nsIMIMEInfo.idl ----


> +	/* Returns a nsIFile that points to the application that is associated
> +     * by default with this content type.  This will usually be specified in
> +     * the platform settings somehow.  This is not always guaranteed
> +	 * to be set!!
> +	 */


This is indented oddly.

	Bill Law Assignee
	Comment 65 • 23 years ago

Adding Mike Kaply to cc: list.  Mike, can you look at the OS/2 changes and 
maybe test out this patch there?  If you prefer, wait till I attach the new 
patch with fixes for Boris's comments.  Thanks.

	Akkana Peck
	Comment 66 • 23 years ago

I tried again, and saw exactly the same behavior.  So I removed all my modified
files and updated them back to the trunk, to try re-applying the patch, but it
doesn't apply any more -- lots of things have changed out from under it.  Maybe
it didn't apply right the first time, either, and I missed the error messages. 
I'll wait for the new patch.  My mimeTypes.rdf has only the two types that show
up in the helper apps dialog, nothing related to audio.

	Ian Pottinger
	Comment 67 • 23 years ago

adding self to cc list

	Matthew T (active 1999-2002)
	Comment 68 • 23 years ago

Since this bug is well down the track implementing a design which is only 
trivially worse than what we have now, but which is considerably worse than the 
design this bug was filed for (my fault entirely, for not keeping up with 
bugmail), I've filed bug 136619 on implementing the original design after this 
has landed.

	Peter Trudelle
	Comment 69 • 23 years ago

Mass moving nsbeta1+/adt3 bugs assigned to Navigator team engineers out to
target milestone 1.0.1.  Please confine your attentions to driving down our list
of TM 1.0 bugs for beta.  Better to help, debug, or test one of them than fix
one of these.

	scottputterman
	Comment 70 • 23 years ago

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

	scottputterman
	Comment 71 • 23 years ago

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

	Philip K. Warren
	Comment 72 • 22 years ago

Any updated status on this bug?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 73 • 22 years ago

renominating.  This fixes bug 93173 and as such should be viewed as being far
more than an enhancement... Bug 93173 is a major usability problem with our
helper app handling...

	Samir Gehani
	Comment 74 • 22 years ago

Nav triage team: nsbeta1-

	Samir Gehani
	Comment 75 • 22 years ago

Nav triage team: nsbeta1+/adt2

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 76 • 22 years ago

*** Bug 177229 has been marked as a duplicate of this bug. ***

	Peter Trudelle
	Comment 77 • 22 years ago

*** Bug 111035 has been marked as a duplicate of this bug. ***

	Philip K. Warren
	Comment 78 • 22 years ago

This is the closest approximation I could get to the v2.0 patch which applies
to the trunk. Some sections of code were removed and thus not included in the
most recent patch. In addition, some changes were necessary, including:

1) Replacing all usage of getUnicodeXXX to match the current nsIFile interface.

2) Replacing gTree usage in pref-applications.js since it was moved from a tree
to a listbox.

	Philip K. Warren
	Comment 79 • 22 years ago

I have a patch coming up which addresses bzbarsky's first review of the v2.0
patch...

	Philip K. Warren
	Comment 80 • 22 years ago

This is the same as Patch v2.0 (updated to trunk), but addresses initial review
by bzbarsky.

pref-applications-edit.xul:

1) I replaced all hard coded numbers in the file with constants from
nsIMIMEInfo. Also removed the following section:

-      if (handlerInfo.handleInternal == "true")
-	 data = 0;

After this statement, the selectedItem is assigned to the xul radio element
with value=data - but there was no xul element with data=0. If this code path
had ever been followed, it would have surely generated a javascript error.

2) extArray.value was indeed the array. I changed the usage to reflect this.

3) Renamed gContentType to gDescription. Changed name of textbox element.
Renamed gExtensionLabel to gExtension.

4) Done as part of 1).

5) Didn't make this change. Perhaps someone with more experience with XUL could
suggest a better fix?

pref-applications.js:

1) Added exceptions around getCharPref. Split by ',' when retrieved from pref,
and then unescape the separated strings below.

2) Didn't remove this section. Need to figure out a method to preserve the
prefs, but only to add them to mimeTypes.rdf once.

pref-applications.xul:

1) Removed these prefstrings.

nsHelperAppDlg.js:

1) Changed this to:

return this.helperAppChoice() !=
this.mLauncher.MIMEInfo.preferredApplicationHandler

I believe this is the intended behavior.

2) Yes - we do need to pass 'this' - not changed.

3) Didn't make this change.

nsHelperAppDlg.dtd:

1) There is no 'k' in the label. I changed it to 'a' for now, but this can be
changed.

nsExternalHelperAppService.cpp:

1) Not really a suggested fix. Yes - this is when there is a MIME info and a
URL.

2) Left this as it was written. I believe on an operating system level, it
makes more sense to use the extension of the filename, instead of the potential
extension(s) associated with the MIME type in Mozilla's mimeTypes.rdf.

3) Perhaps Bill can best answer this question. I left it as it was originally
written by him.

4) Indentation seems ok now.

5) This entire section of code was removed with the patch to Bug #147142.
Didn't	add this code when merging the patch to the trunk.

6) Changed to nsUnescapeURL. Please check that is is used correctly.

7) Used CaseInsensitiveFindInReadable as you have suggested.

8) Code removed - followed suggestion from 7).

nsIMIMEInfo.idl:

1) Fixed whitespace.

Current problems with patch:

1) When selecting the 'default' application for a mime type (e.g.
application/pdf) and clicking ok, the helper application is not launched the
first time. It is launched on subsequent times.

2) Manually entering in a path for the helper application doesn't seem to be
working correctly. Entering in an application that exists on the system results
in a 'file not found' message when clicking 'OK'. If the helper application is
chosen through the 'Choose' dialog, everything works properly.

3) The width of the useSystemDefault radio label in nsHelperAppDlg.xul is
currently too small. Looking for help from someone more familiar with XUL to
fix this problem.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 81 • 22 years ago

Some review comments on the current patch....  I'm not sure what's causing the 
problems described in comment 80 and it's hard to tell without a build to try 
with...  Unfortunately, I have no tree till Jan 5 (hence all the requests for 
screenshots).

*** nsHelperAppDlg.js ***

Does the dialog come up with the OK button enabled when the "open with default"
or "open using" radios are preselected?  What about the "Browse" button?  It
looks like initAppAndSaveToDiskValues is not calling this.toggleChoice()
somewhere (see also Bill's comments on his first patch).

In openWithDefaultOK(), move the "var result;" out of the else clause to before
the if.  While the code as written is correct JS and will not even trigger
warnings, it's pretty confusing...


*** nsHelperAppDlg.xul ***

Any chance of a screenshot?


*** nsExternalHelperAppService.cpp ***

Drop the NS_ENSURE_ARGs from GetMIMEInfoForMimeTypeFromOS and
GetMIMEInfoForExtensionFromOS and just
NS_PRECONDITION(aMIMEInfo, "Null out param"); instead, please.

> +    if (!mHelperAppService->MIMETypeIsInDataSource(MIMEType.get()))

NS_ASSERTION(mHelperAppService, "Not initialized properly");

right before that line.

> +  mHelperAppService = aHelperAppService;
> +  NS_IF_ADDREF(mHelperAppService);

Have Init() return an error if aHelperAppService is null, please.  And check
that the callers react correctly to Init() failing...

> +  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(prefs, &rv);

prefs->GetBranch("", getterAddRefs(prefBranch));

See the prefs stuff below for more details on what I think the right branch
is....


*** nsHelperAppRDF.h ***

The new define's value is not lined up with the other values... not enough
spaces?  Tabs?


*** unix/nsOSHelperAppService.cpp ***

Please move the "+ nsresult rv" declarations down to the first place where "rv"
is assigned to... right now it looks like those could be used uninitialized...

Same for OS/2.


*** win/nsOSHelperAppService.cpp ***

The comment about "nsIMIMEService method over-rides" should probably just say
"method overrides" -- these are no longer nsIMIMEService methods.

Same for all the nsOSHelperAppService.h files.


*** pref-applications-edit.xul ***

See comments on pref-applications.js below regarding prefservice stuff.  The
getBoolPref should be in a try/catch.

A screenshot of this dialog would be nice (with those changes).


*** pref-applications.js ***

> +    var prefBranch = Components.classes
["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPrefBranch);

While this works for now, I think something like this is more encouraged:

var prefService = Components.classes["@mozilla.org/preferences;1"].getService
(Components.interfaces.nsIPrefService);
var prefBranch = prefService.getBranch("");

And the whole thing needs to be in a try/catch with a return out of the catch,
since getService will throw when it fails instead of just returning null...

Further, you may want getBranch("browser.helperApps.neverAsk.") (with the
trailing '.') here, since all the prefs you are looking for are in that branch;
then the getCharPref and clearUserPref calls can just use "saveToDisk" and
"openFile".

It looks to me like

+    var newEntries = new Array;

would be better written as:

+    var newEntries = {};

since that would be more consistent with the way it's used (to map types to
actions).

xmlSinkObserver may be better as a const than a var.


*** pref-applications.xul ***

Any chance of a screenshot of what the pref panel looks like with this patch?

-   <textbox id="handler" readonly="true" style="width: 175px" />
+   <textbox id="handler" crop="right" readonly="true" style="width: 175px" />

I'm not sure we need to add "crop" to this (it's been changed from a <label> to
a <textbox> already to try to address the "stuff not fitting" issue...

*** pref-applications-edit.dtd ***

This should get the localization note about "MIME" from pref-applications.dtd

> +<!ENTITY  handling.label               "When I open a file of this type:">

Would this make more sense as "When a file of this type is encountered:" ?


*** pref-applications.dtd ***

We no longer need the localization note if we remove the "mimeType" entity.

> +<!ENTITY handle                         "When opened:">

Would it make more sense to make this "Handling:" or "When encountered:"
instead of "When openened:"?  "When opened: Open these files in foo" and "When
opened: Save these files to disk" both look really odd...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 82 • 22 years ago

*** Bug 188001 has been marked as a duplicate of this bug. ***

	Philip K. Warren
	Comment 83 • 22 years ago

Screenshot of the new nsHelperAppDlg.xul interface.

	Philip K. Warren
	Comment 84 • 22 years ago

Screenshot of the new pref-applications-edit.xul interface.

	Philip K. Warren
	Comment 85 • 22 years ago

Latest patch updated to trunk and addresses latest review comments from Boris.
Also fixed the three problems I reported earlier in comment 80.

New problem I am seeing in debug builds is the following (triggered while
viewing attachments with either the default handler or a user specified
handler):

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.doc.getElementById(aDownloadID) has no

properties" {file:
"file:///home/pkw/builds/trunk/mozilla/obj-debug/dist/bin/components/nsDownloadProgressListener.js"
line: 99}]' when calling method:
[nsIDownloadProgressListener::onProgressChange]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame ::
file:///home/pkw/builds/trunk/mozilla/obj-debug/dist/bin/components/nsHelperAppDlg.js
:: anonymous :: line 589"  data: yes]
************************************************************

Need to investigate this problem.

	Tim Powell
	Comment 86 • 22 years ago

I had hoped this would include a fix for bug 57342, but I guess that can be 
fixed afterwards.

	Philip K. Warren
	Comment 87 • 22 years ago

Address review by timeless sent over email.

	Philip K. Warren
	Comment 88 • 22 years ago

Patch updated to apply to the trunk after all of yesterday's changes.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 89 • 22 years ago

Comment on attachment 111106 [details] [diff] [review]
Patch v6

** nsHelperAppDlg.js **

In initDefaultApp() the first if() test should be ==, not !=.

When the helper app dialog first comes up, the selected radio button is "Open
with default application" (if such exists) but the textfield and "Choose"
button for the "Open with" option are both enabled.  This is wrong.  As I said
in comment 81, initAppAndSaveToDiskValues() needs to call this.toggleChoice()
(probably right before it does the mac-specific disabling).

>   this.mLauncher.MIMEInfo.applicationDescription = "";

This should set a real description (and file a bug on me to move the leafname
fallback into the nsIMIMEInfo impl).


** pref-applications.xul **

Right after the <caption label="&file;"/>, make the hbox flex="1".  Make all
the rows in the grid align="center".  And make the <textbox> in the third row
flex="1".  Remove the style attribute that sets its width; it will now flex
properly.


** pref-applications.js **

You need to change the line:

var gMIMETypeField  = null;

to 

var gMIMEDescField  = null;

no?  I get a strict JS warning for the undefined var...

xpfe/components/prefwindow/resources/content/overrideHandler.js

The line (423):

    if (urns[i][2]) {

should be:

    if ("2" in urns[i] && urns[i][2]) {

to prevent the strict JS warning this triggers.


** nsExternalHelperAppService.cpp **

> -  // always ask --> these fields aren't stored in the data source anymore

leave the "// always ask" part, ok?  ;)

r/sr=bzbarsky with these changes

timeless, could you please review?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 90 • 22 years ago

Oh, and to clarify, I just spent a few hours testing this; the only issues I
caught are either ones already existing that have bugs or are fixed by making
the changes in comment 89

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 91 • 22 years ago

Comment on attachment 111106 [details] [diff] [review]
Patch v6

review comments, mostly copied out of irc. please attach a new patch for them.
please also address bz's comment.

<biesi> bz: -handlerExists=A helper already exists for the MIME type '%mime%'.
Do you want to replace it? 
<biesi> +handlerExists=A helper already exists for the MIME type '%mime%'.  Do
you want to replace it? 
<biesi> bz: this seems, like, useless?

<biesi> pkw: +		  result = this.mLauncher.MIMEInfo.defaultDescription
||
<biesi> +		      this.mLauncher.MIMEInfo.defaultApp;
<biesi> pkw: why is it enough to have a description?
<pkw> biesi: you are correct - it should just be correct if an app is defined

also, defaultApp in that line should be changed to defaultApplicationHandler

<biesi> pkw: what's this:
<biesi>      updateApplicationName: function(newValue)
<biesi>      {
<biesi> pkw: the annotated patch shows it as being removed

<biesi> this.dialogElement( "appPath" ).value.search(/\S/) != -1;
<biesi> you only want to know if this matches, I think you can write it this
way:
<biesi> /\S/.test( this.dialogElement( "appPath" ).value );

<biesi> pkw: // For "open with," we need [...]
<biesi> I think the , belongs after the "

<biesi> pkw: around this: +	   if ( this.dialogElement( "saveToDisk"
).selected ) {
<biesi> pkw: for save and system default, you check if the new value differs
from the current value before setting preferredAction
<biesi> pkw: then you should also check in the "open with" part if the value is
different and set only then


+	 var helperApp;
+	 // Verify typed app path, if necessary.
+	 if ( this.dialogElement( "openUsing" ).selected ) {
+	     helperApp = this.helperAppChoice();
any special reason for declaring the variable outside the if?


+		 // Select and focus the input field.
what does this do on macos, where the input field is disabled?
<pkw> pkw: i assume we could test if its disabled before calling select+focus
<biesi> yeah, that's probably a good idea

<biesi> pkw: -<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<biesi> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
<biesi> pkw: please don't introduce trailing whitespace...

<biesi> bz/pkw, question, what happens if I deactivate the "Always ask" thingy
<biesi> pkw: can you label it as "Remember my choice"?
<pkw> i think that would make for a good additional bug


<biesi> pkw: deliminated does not look like an english word, and
www.dictionary.com agrees with me on that
<biesi> pkw: "delimited" is probably better

<biesi> pkw: +	      entry.appDisplayName = "";
<biesi> is there nothing better you can use?
<biesi> bz: around line 200 in pref-applications-edit.xul

<biesi> pkw: +	      entry.appDisplayName = "";
<biesi> bz: around 200 in pref-applications-edit.xul
<bz> set that to the applicationDescription or defaultApplicationDescription
<bz> but only if you set an app....

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 92 • 22 years ago

oh actually, just for me you need not attach a new patch, address my comments
and you have r=biesi

	Philip K. Warren
	Comment 93 • 22 years ago

Patch which addresses bz and biesi's latest comments.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 94 • 22 years ago

Comment on attachment 111310 [details] [diff] [review]
Patch v7

better, thanks

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 95 • 22 years ago

Comment on attachment 111310 [details] [diff] [review]
Patch v7

sr=bzbarsky.

I've landed the patch, per pkw's request. Here's hoping nothing breaks too
badly.	;)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 96 • 22 years ago

It builds, ship it.

Please file separate bugs on any problems that this causes, okee?  And cc me and
pkw on them.

Also, it'd be nice to retriage file handling bugs at this point -- a brief
search when I looked for dups of stuff today showed a couple more bugs this
fixed... ;)

	Andreas Kunz
	Comment 97 • 22 years ago

The retriage might start with helper app bugs:
bug 51395, bug 79543, bug 79897, bug 88330, (bug 143663?), bug 176062 and bug
180942.
Most or all of them work now.

Unfortunately bug 91828 was not adressed with this patch...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 98 • 22 years ago

*** Bug 194549 has been marked as a duplicate of this bug. ***

	Chris Petersen
	Comment 99 • 22 years ago

Verified