piers Reporter
	Description • 22 years ago

The Edit Type dialog can get into a state where it can't be dismissed on "OK".

Steps:
1. Open prefs -> Helper Applications;
2. Click "New Type";
3. Fill anything in, but ensure the "When a file of this type is encountered"
box has "Open with default application" checked;
4. Click OK to create the type;
5. Select the type just created and click "Edit";
6. Change the radio to "Open it with" (and choose any application);
7. Click OK.

Actual Results: Dialog isn't dismissed.

Expected Results: Changes should be acepted and the dialog dismissed.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 22 years ago

This basically makes the change I mentioned, Philip.  I rolled in the fix for
bug 189080 because it was hard to disengage the patches, but I could do that if
you'd like.  The fix for bug 189107 is an integral part of this patch, since
the problem there is we first assert a description of "" and then assert one of
"whatever" and then both are present in the DS... however we only read back the
first one.  ;)	With this patch, we assert the first time and change the second
time, so it all works.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 22 years ago

taking; peterv, this is the patch I mentioned needs a sanity check.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 22 years ago

Comment on attachment 111582 [details] [diff] [review]
proposed patch

So to summarize the changes:

1) Add clearExtensions() so that the edit panel can use it (for bug 189080)

2) Make all people who want to "set or update" call changeMIMEStuff instead of
conditioning on this.mUpdateMode

3)  Make changeMIMEStuff deal with the case when the assertion it's being asked
to change does not exist yet.

4) assertMIMEStuff needs to stay since addExtension, eg, still uses it.

5) Change the few callers (saveToDisk setter, useSystemDefault setter,
handleInternal setter) who manually and painfully did checking for existing
assertions before calling changeMIMEStuff to just call changeMIMEStuff, since
that deals with such things now.

	Philip K. Warren
	Comment 4 • 22 years ago

I think this patch looks ok, but I need to test it out a bit.

Two comments:

1) Can't we remove changeMIMEStuff entirely and just have everything call
assertMIMEStuff. That would make more sense to me - having assert/unassert only
instead of what is proposed in this patch - assert (not really needed), change
(works like a combined assert+change), and unassert.

2) This may be too nitpicky, but can we come up with better function names than
assertMIMEStuff/unassertMIMEStuff?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 22 years ago

> Can't we remove changeMIMEStuff entirely

Not easily.  The extension list is stored as a whole bunch of assertions, all
with the same origin, target, and truth value, but different values. 
changeMIMEStuff, as currently written (with my patch, but also without), does
not support leaving all existing assertions with the given origin and target and
adding a new one.  If such an assertion already exists, its value will be
changed.  Note that the _only_ remaining user of assertMIMEStuff is addExtension().

As for the nitpick, I would very much like to move all this code into a service
(nsIXMLMIMEService or something) that will just have two functions on it:

nsIMIMEInfo getMIMEInfo(nsACString extension, nsACString type);
void saveMIMEInfo(nsIMIMEInfo mimeInfo);

Then various callers can just use that service instead of messing with the RDF
directly...

In the meantime I could certainly rename the functions, but I'd rather focus on
fixing the architecture.... ;)

	Philip K. Warren
	Comment 6 • 22 years ago

Comment on attachment 111582 [details] [diff] [review]
proposed patch

>+  clearExtensions: function ()
>+  {
>+    var extensionResource = gRDF.GetResource(NC_RDF("fileExtensions"));
>+    var contentTypeResource = gRDF.GetResource(MIME_URI(this.mimeType));
>+    var extensionTargets  = gDS.GetTargets(contentTypeResource, extensionResource, true);
>+    if (extensionTargets) {
>+      while (extensionTargets.hasMoreElements()) {
>+        var currentExtension = extensionTargets.getNext();
>+        if (currentExtension) {
>+          currentExtension = currentExtension.QueryInterface(Components.interfaces.nsIRDFLiteral);
>+	  this.removeExtension(currentExtension.Value);
>+	}
>+      }
>+    }
>+  },

You could share the extensions code by doing the following:

var extArray = this.extensions.split(" ");
for (i = 0; i < extArray.length; i++) {
  this.removeExtension(extArry[i]);
}

If not, please changes tabs->spaces in the clearExtensions function as it is
currently written.

I have tested this patch, and it does seem to resolve all issues in this bug
and the other two dependencies on this bug. I don't see any regressions or
warnings/errors in the JavaScript console after applying this patch.

r=pkw

	Peter Van der Beken [:peterv]
	Comment 7 • 22 years ago

Comment on attachment 111582 [details] [diff] [review]
proposed patch

>+  clearExtensions: function ()
>+  {
>+    var extensionResource = gRDF.GetResource(NC_RDF("fileExtensions"));
>+    var contentTypeResource = gRDF.GetResource(MIME_URI(this.mimeType));
>+    var extensionTargets  = gDS.GetTargets(contentTypeResource, extensionResource, true);
>+    if (extensionTargets) {
>+      while (extensionTargets.hasMoreElements()) {
>+        var currentExtension = extensionTargets.getNext();
>+        if (currentExtension) {

Isn't this if redundant with the hasMoreElements check above?

>+          currentExtension = currentExtension.QueryInterface(Components.interfaces.nsIRDFLiteral);
>+	  this.removeExtension(currentExtension.Value);

You seem to be the only one using removeExtension. Maybe you could remove that
function and use unassertMIMEStuff directly instead?

There's some tabs in the patch, please remove them before checkin.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 22 years ago

> You could share the extensions code by doing the following:

Yes, I could.  Good idea.

> Isn't this if redundant with the hasMoreElements check above?

You'd think... but there are enumerator impls which need that if check.  :(

> You seem to be the only one using removeExtension. Maybe you could remove
that

I'd rather keep it for now for conceptual simplicity.

> There's some tabs in the patch

All gone.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 22 years ago

fixed

	Chris Petersen
	Comment 10 • 22 years ago

Verified in the Win32 2003-01-23-04 and OSX 2003-01-23-03 trunk builds.