646854 - Let the user change the filter in the file picker when opening a file on MacOS X

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mounir Lamouri (:mounir)]

Follow up of bug 643576: the default file filter is now used instead of all of them but we should let the user to change it...

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v0.1

I'm asking a review but I still have an issue with this patch: the first time you select an element on the list, the filter doesn't change. After, it's working. This also happen if you select the currently selected filter.

Weirdly, using 10.6 API makes it work (calling runModal and setting filters and directory with specific methods).

Comment 2

[Mounir Lamouri (:mounir)]

This URL is going to be easier to use:
data:text/html,<input type='file' accept='image/*'>

Comment 3

[Josh Aas]

Comment on attachment 523623 [details] [diff] [review]
Patch v0.1

I think "setAllowedFileTypes:" is just buggy, though it is possible we're missing something subtle. I'd just code this feature for 10.6+ for now. There is a function for testing for 10.6+ in this file:

widget/src/cocoa/nsToolkit.h

Comment 4

[Josh Aas]

BTW - in case you didn't know this already, you can compile obj-c methods that might not exist on a platform without doing anything special (you might get a warning). Just don't call them, use a run time check.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Thanks for the comments. This patch works on 10.6. I can't try 10.5 but I believe it should work if it compiles.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Created attachment 527239 [details] [diff] [review]
> Patch v1
> 
> Thanks for the comments. This patch works on 10.6. I can't try 10.5 but I
> believe it should work if it compiles.

When I say it should work on 10.5 I mean that the feature shouldn't be enabled and nothing should be broken ;)

Comment 7

[Josh Aas]

Comment on attachment 527239 [details] [diff] [review]
Patch v1

>+  // On 10.6+, we let users change the filters. Unfortunately, some methods
>+  // are not available on 10.5 and without using them it happens to be buggy.
>+  NSPopUpButtonObserver* observer = [[NSPopUpButtonObserver alloc] init];
>+  if (nsToolkit::OnSnowLeopardOrLater()) {
>+    NSView* accessoryView = GetAccessoryView();
>+    [thePanel setAccessoryView:accessoryView];
>+
>+    [observer setPopUpButton:[accessoryView viewWithTag:kSaveTypeControlTag]];
>+    [observer setOpenPanel:thePanel];
>+    [observer setFilePicker:this];
>+
>+    [[NSNotificationCenter defaultCenter]
>+      addObserver:observer
>+      selector:@selector(menuChangedItem:)
>+      name:NSMenuWillSendActionNotification object:nil];
>+  }

Can you move this down to the OnSnowLeopardOrLater() block where you eventually call "[thePanel runModal]"? That would cut down on the chance that an early return leaks the observer object and you'll reduce code by using the same "if" statement for configuring it.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Moving some other calls inside the 10.6+ block as requested on IRC.

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/5c0d5ac6118b

Thanks for the reviews Josh :)

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Looks like this patch triggered what is now the #3 Mac topcrasher on
the trunk:
https://crash-stats.mozilla.com/query/query?product=Firefox&version=Firefox%3A6.0a1&platform=mac&range_value=1&range_unit=weeks&date=04%2F26%2F2011+11%3A19%3A29&query_search=signature&query_type=contains&query=libobjc.A.dylib%400x511c+&reason=&build_id=&process_type=any&hang_type=any&do_query=1

Looking at the crash stacks, it seems like notifications are getting
sent to a deleted NSPopUpButtonObserver, and not when the file picker
is running.  I've currently no idea how this could be happening.  But
if nobody beats me too it, later I'll open a bug on this and
investigate it further.

Comment 13

[Chris Lawson (gone)]

See also bug 652854, but note bug 652854 comment 2.