Last Comment Bug 646854 - Let the user change the filter in the file picker when opening a file on MacOS X
: Let the user change the filter in the file picker when opening a file on MacOS X
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
: Markus Stange [:mstange]
Mentors:
data:text/html,<input type='file' acc...
: 671172 (view as bug list)
Depends on: 652854
Blocks: 651477 651480
  Show dependency treegraph
 
Reported: 2011-03-31 09:03 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-15 02:25 PDT (History)
4 users (show)
mounir: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (11.14 KB, patch)
2011-04-01 10:07 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (11.97 KB, patch)
2011-04-20 05:40 PDT, Mounir Lamouri (:mounir)
jaas: review-
Details | Diff | Splinter Review
Inter diff (3.00 KB, patch)
2011-04-21 07:49 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (11.82 KB, patch)
2011-04-21 07:50 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.2 (11.72 KB, patch)
2011-04-21 09:02 PDT, Mounir Lamouri (:mounir)
jaas: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-03-31 09:03:15 PDT
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) 2011-04-01 10:07:03 PDT
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).
Comment 2 Mounir Lamouri (:mounir) 2011-04-08 09:12:28 PDT
This URL is going to be easier to use:
data:text/html,<input type='file' accept='image/*'>
Comment 3 Josh Aas 2011-04-15 07:26:11 PDT
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 2011-04-15 07:28:45 PDT
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) 2011-04-20 05:40:19 PDT
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.
Comment 6 Mounir Lamouri (:mounir) 2011-04-20 05:41:43 PDT
(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 2011-04-20 10:29:13 PDT
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) 2011-04-21 07:49:30 PDT
Created attachment 527537 [details] [diff] [review]
Inter diff
Comment 9 Mounir Lamouri (:mounir) 2011-04-21 07:50:33 PDT
Created attachment 527538 [details] [diff] [review]
Patch v1.1
Comment 10 Mounir Lamouri (:mounir) 2011-04-21 09:02:58 PDT
Created attachment 527553 [details] [diff] [review]
Patch v1.2

Moving some other calls inside the 10.6+ block as requested on IRC.
Comment 11 Mounir Lamouri (:mounir) 2011-04-22 06:30:03 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/5c0d5ac6118b

Thanks for the reviews Josh :)
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-04-26 11:20:28 PDT
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) 2011-04-26 11:29:34 PDT
See also bug 652854, but note bug 652854 comment 2.
Comment 14 Mounir Lamouri (:mounir) 2011-07-15 02:25:57 PDT
*** Bug 671172 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.