The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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...
(Assignee)

Comment 1

6 years ago
Created attachment 523623 [details] [diff] [review]
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).
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #523623 - Flags: review?(joshmoz)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]

Updated

6 years ago
(Assignee)

Comment 2

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

Comment 3

6 years ago
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)

Updated

6 years ago
Whiteboard: [needs review]

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
Attachment #523623 - Attachment is obsolete: true
Attachment #527239 - Flags: review?(joshmoz)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Comment 6

6 years ago
(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 ;)
(Assignee)

Updated

6 years ago
Blocks: 651477
(Assignee)

Updated

6 years ago
Blocks: 651480

Comment 7

6 years ago
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-
(Assignee)

Comment 8

6 years ago
Created attachment 527537 [details] [diff] [review]
Inter diff
Attachment #527537 - Flags: review?(joshmoz)
(Assignee)

Comment 9

6 years ago
Created attachment 527538 [details] [diff] [review]
Patch v1.1
Attachment #527239 - Attachment is obsolete: true
Attachment #527538 - Flags: review?(joshmoz)
(Assignee)

Updated

6 years ago
Attachment #527537 - Flags: review?(joshmoz)
(Assignee)

Comment 10

6 years ago
Created attachment 527553 [details] [diff] [review]
Patch v1.2

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)

Updated

6 years ago
Attachment #527553 - Flags: review?(joshmoz) → review+

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Updated

6 years ago
Whiteboard: [fixed in cedar]
(Assignee)

Comment 11

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

Thanks for the reviews Josh :)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.

Comment 13

6 years ago
See also bug 652854, but note bug 652854 comment 2.
Depends on: 652854
(Assignee)

Updated

6 years ago
Duplicate of this bug: 671172
You need to log in before you can comment on or make changes to this bug.