Closed Bug 646854 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mounir, Assigned: mounir)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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...
Attached patch Patch v0.1 (obsolete) — Splinter Review
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).
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #523623 - Flags: review?(joshmoz)
Whiteboard: [needs review]
This URL is going to be easier to use:
data:text/html,<input type='file' accept='image/*'>
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
Attachment #523623 - Flags: review?(joshmoz)
Whiteboard: [needs review]
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #523623 - Attachment is obsolete: true
Attachment #527239 - Flags: review?(joshmoz)
Whiteboard: [needs review]
(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 ;)
Blocks: 651477
Blocks: 651480
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.
Attachment #527239 - Flags: review?(joshmoz) → review-
Attached patch Inter diff (obsolete) — Splinter Review
Attachment #527537 - Flags: review?(joshmoz)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #527239 - Attachment is obsolete: true
Attachment #527538 - Flags: review?(joshmoz)
Attachment #527537 - Flags: review?(joshmoz)
Attached patch Patch v1.2Splinter Review
Moving some other calls inside the 10.6+ block as requested on IRC.
Attachment #527537 - Attachment is obsolete: true
Attachment #527538 - Attachment is obsolete: true
Attachment #527538 - Flags: review?(joshmoz)
Attachment #527553 - Flags: review?(joshmoz)
Attachment #527553 - Flags: review?(joshmoz) → review+
Whiteboard: [needs review]
Whiteboard: [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/5c0d5ac6118b

Thanks for the reviews Josh :)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
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.
Duplicate of this bug: 671172
You need to log in before you can comment on or make changes to this bug.