The default bug view has changed. See this FAQ.

Implement 'mediatype' file filters in <upload>

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
8 months ago

People

(Reporter: jhp (no longer active), Assigned: Merle Sterling)

Tracking

({fixed1.8.0.8, fixed1.8.1.1})

Trunk
fixed1.8.0.8, fixed1.8.1.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
Need to implement support for the 'mediatype' attribute on the <upload> element.

Comment 1

11 years ago
Javier suggests that http://lxr.mozilla.org/seamonkey/source/netwerk/mime/public/nsIMIMEService.idl will be a valuable interface to understand in order to implement this in case he isn't the one that eventually does the work.

Updated

11 years ago
Blocks: 326372

Updated

11 years ago
Blocks: 326373

Comment 2

11 years ago
blocks 1.0 2nd ed test case: 8.1.6.a
this blocks bug 322255 though I don't have authority to update the bug

Updated

11 years ago
Blocks: 322255

Updated

11 years ago

Comment 3

11 years ago
Created attachment 230467 [details]
testcase - taken from testuite 8.1.6.a
(Assignee)

Comment 4

11 years ago
Created attachment 233187 [details] [diff] [review]
Handle mediatype attribute

If the mediatype attribute is present we parse the space delimited list of mime types, map the mime types to file extensions using nsIMIMEService, and build a file extension filter string for the file open dialog.

Note that the test case has a typo: image/jpg must be image/jpeg
Attachment #233187 - Flags: review?(aaronr)

Comment 5

11 years ago
(In reply to comment #4)
> Created an attachment (id=233187) [edit]
> Handle mediatype attribute
> 
> If the mediatype attribute is present we parse the space delimited list of mime
> types, map the mime types to file extensions using nsIMIMEService, and build a
> file extension filter string for the file open dialog.
> 
> Note that the test case has a typo: image/jpg must be image/jpeg
> 

I'd propose to wait for the bug 347327 since your patch ingores mediatype element but mentioned bug adds its support. Later you can see mozType:mediatype attribute on upload element (taking into account I should do a little modification in my patch to support upload :)). What do you think?

Updated

11 years ago
Assignee: jhpedemonte → msterlin

Comment 6

11 years ago
I guess @mediatype gives a hint how to render upload control, i.e. we should have several xf:upload bindings for different types of @mediatype. Do we want to have this? If true then I guess we should post new bug since current bug is not about it. Right?

Comment 7

11 years ago
(In reply to comment #6)
> I guess @mediatype gives a hint how to render upload control, i.e. we should
> have several xf:upload bindings for different types of @mediatype. Do we want
> to have this? If true then I guess we should post new bug since current bug is
> not about it. Right?
> 

I can't think of any examples where the control would be different based on the mime type.  Do you have an idea of which ones would be different?
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

11 years ago
(In reply to comment #5)
> I'd propose to wait for the bug 347327 since your patch ingores mediatype
> element but mentioned bug adds its support. Later you can see mozType:mediatype
> attribute on upload element (taking into account I should do a little
> modification in my patch to support upload :)). What do you think?

I think it could go either way.  I could wait for bug 347327 to be complete and then change the two lines of code in my patch that simply gets the mediatype attribute to check for the mediatype element instead OR we can check in this bug and the change could be made as part of bug 347327.  Given that this bug fixes many of the failing test cases in the test suite, I would prefer the latter.

Comment 9

11 years ago
(In reply to comment #8)
> (In reply to comment #5)
> > I'd propose to wait for the bug 347327 since your patch ingores mediatype
> > element but mentioned bug adds its support. Later you can see mozType:mediatype
> > attribute on upload element (taking into account I should do a little
> > modification in my patch to support upload :)). What do you think?
> 
> I think it could go either way.  I could wait for bug 347327 to be complete and
> then change the two lines of code in my patch that simply gets the mediatype
> attribute to check for the mediatype element instead OR we can check in this
> bug and the change could be made as part of bug 347327.  Given that this bug
> fixes many of the failing test cases in the test suite, I would prefer the
> latter.
> 

I'd vote that this bug wait for 347327, which is already in the review process.

Comment 10

11 years ago
(In reply to comment #7)

> I can't think of any examples where the control would be different based on the
> mime type.  Do you have an idea of which ones would be different?
> 

Nothing special, I'm just looking at specs (http://www.w3.org/TR/xforms/index-all.html#ui-upload). Here there is upload's picture for mediatype="image/*". I don't know do we need something like this? At least the showed upload control looks friendly I guess.

Comment 11

11 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > I'd propose to wait for the bug 347327 since your patch ingores mediatype
> > > element but mentioned bug adds its support. Later you can see mozType:mediatype
> > > attribute on upload element (taking into account I should do a little
> > > modification in my patch to support upload :)). What do you think?
> > 
> > I think it could go either way.  I could wait for bug 347327 to be complete and
> > then change the two lines of code in my patch that simply gets the mediatype
> > attribute to check for the mediatype element instead OR we can check in this
> > bug and the change could be made as part of bug 347327.  Given that this bug
> > fixes many of the failing test cases in the test suite, I would prefer the
> > latter.
> > 
> 
> I'd vote that this bug wait for 347327, which is already in the review process.
> 

Perhaps I'm missing something, bug 347327 deals with mediatype element on output element.  This bug deals with mediatype attribute on the upload element.  Where is the conflict?

Comment 12

11 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=233187) [edit]
> > Handle mediatype attribute
> > 
> > If the mediatype attribute is present we parse the space delimited list of mime
> > types, map the mime types to file extensions using nsIMIMEService, and build a
> > file extension filter string for the file open dialog.
> > 
> > Note that the test case has a typo: image/jpg must be image/jpeg
> > 
> 
> I'd propose to wait for the bug 347327 since your patch ingores mediatype
> element but mentioned bug adds its support. Later you can see mozType:mediatype
> attribute on upload element (taking into account I should do a little
> modification in my patch to support upload :)). What do you think?
> 

Well, upload's mediatype element has been supported for sometime.  This bug doesn't deal with that node.

See http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsUploadElement.cpp#342

Comment 13

11 years ago
(In reply to comment #11)

> Perhaps I'm missing something, bug 347327 deals with mediatype element on
> output element.

Not only, bug 347327 deals with @mediatype attribute too ;)

>  This bug deals with mediatype attribute on the upload element.
>  Where is the conflict?
> 

When we deal with some mediatype issues then we should take into account attribute and element I guess since mediatype can be specified by @mediatype attribute or by mediatype element both.

Comment 14

11 years ago
The more I look at this, the more I realize that there is no overlap as far as 1.0 goes.  In XForms 1.0, xf:mediatype has no affect on xf:upload (though xf:upload does affect xf:mediatype's bound value).  In XForms 1.1, xf:mediatype affects the control that contains it.  While in XForms 1.1 or 1.2 there might possibly be overlap (an upload without @mediatype might be able to inherit this from a contained xf:mediatype in the future), there isn't yet.  So let's keep mediatype as it pertains to upload and mediatype as it pertains to output seperate.  We can merge them together if a future XForms spec changes that seperation.

Comment 15

11 years ago
Comment on attachment 233187 [details] [diff] [review]
Handle mediatype attribute

>Index: nsXFormsUploadElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUploadElement.cpp,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 nsXFormsUploadElement.cpp
>--- nsXFormsUploadElement.cpp	10 Aug 2006 21:41:25 -0000	1.18
>+++ nsXFormsUploadElement.cpp	11 Aug 2006 00:34:15 -0000
>@@ -202,19 +202,65 @@ nsXFormsUploadElement::PickFile()
>   nsCOMPtr<nsIFilePicker> filePicker =
>             do_CreateInstance("@mozilla.org/filepicker;1");
>   if (!filePicker)
>     return NS_ERROR_FAILURE;
> 
>   rv = filePicker->Init(internal, filepickerTitle, nsIFilePicker::modeOpen);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // set filter "All Files"
>-  // XXX implement "@mediatype" file filters
>-  filePicker->AppendFilters(nsIFilePicker::filterAll);
>+  // Set the file picker filters based on the mediatype attribute.
>+  nsAutoString mediaType;
>+  mElement->GetAttribute(NS_LITERAL_STRING("mediatype"), mediaType);
>+
>+  if (mediaType.IsEmpty()) {
>+    filePicker->AppendFilters(nsIFilePicker::filterAll);
>+  }
>+  else {

nit: else goes on line above to keep style consistent

>+    // The mediatype attribute contains a space delimited list of mime types.
>+    nsresult rv;
>+    nsCOMPtr<nsIMIMEService> mimeService =
>+      do_GetService("@mozilla.org/mime;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsAString::const_iterator start, end, iter;
>+    mediaType.BeginReading(start);
>+    mediaType.BeginReading(iter);
>+    mediaType.EndReading(end);
>+
>+    nsAutoString fileFilter;
>+    nsAutoString mimeType;
>+    while (iter != end) {
>+      if (FindCharInReadable(' ', iter, end)) {
>+         mimeType = Substring(start, iter);
>+         // Skip the space.
>+         ++iter;
>+         // Save the starting position for the next mime type (if any).
>+         start = iter;
>+      }
>+      else {
>+        // Only 1 media type or we've reached the end of the list.
>+        mimeType = Substring(start, end);
>+      }
>+
>+      // Map the mime type to a file extension and add the extension to the file
>+      // type filter for the file dialog.
>+      nsCAutoString fileExtension;
>+      rv = mimeService->GetPrimaryExtension(NS_ConvertUTF16toUTF8(mimeType),
>+                                            EmptyCString(), fileExtension);
>+      if (NS_SUCCEEDED(rv) && !fileExtension.IsEmpty()) {
>+        fileFilter.AppendLiteral("*.");
>+        fileFilter.Append(NS_ConvertUTF8toUTF16(fileExtension));
>+        fileFilter.AppendLiteral(";");
>+      }
>+    }
>+
>+    // Append the file extension filter.
>+    filePicker->AppendFilter(fileFilter, fileFilter);
>+  }

I'm thinking maybe the file filter should be 'all files' if we don't find any mime types.  The file picker shows all the files anyhow and it just looks weird if the "files of type" field is blank.

With those, r=me
Attachment #233187 - Flags: review?(aaronr) → review+
(Assignee)

Comment 16

11 years ago
Created attachment 233818 [details] [diff] [review]
patch

Always add 'All Files' as a filter.  Do not append the semicolon to the last filter in the list.
Attachment #233187 - Attachment is obsolete: true
Attachment #233818 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #233818 - Flags: review?(Olli.Pettay) → review+

Comment 17

11 years ago
Created attachment 234798 [details]
testcase - corrected

corrected testcase per merle's suggestion
Attachment #230467 - Attachment is obsolete: true

Comment 18

11 years ago
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 19

11 years ago
checked into 1.8.0 branch on 2006/09/21
Keywords: fixed1.8.0.8

Comment 20

10 years ago
checked into 1.8 branch on 2006/11/21
Keywords: fixed1.8.1.1
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.