Closed Bug 352674 Opened 14 years ago Closed 14 years ago

Set filter on alarm sound file picker

Categories

(Calendar :: Alarms, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: thorsten.fritz, Assigned: ispiked)

Details

Attachments

(1 file, 3 obsolete files)

The filter for alarm sounds is set to "All Files".

On Linux i can't play mp3 or ogg files as an alarm sound.. So we should set this filter to specific file types depending on the OS.
Attached patch Untested patch (obsolete) — Splinter Review
Assignee: nobody → ispiked
Status: NEW → ASSIGNED
Attachment #238455 - Flags: second-review?(jminta)
Attachment #238455 - Flags: first-review?(mattwillis)
Make that |#define UNIX_BUT_NOT_MAC|, actually. :)
Comment on attachment 238455 [details] [diff] [review]
Untested patch

I'm a little concerned here about special-casing linux like this.  Are there any docs somewhere that assure us that win/mac can handle all (or nearly all) sounds files?
Well, on Linux we /only/ support WAV files explicitly. On Windows and Mac we pass the sound off to system APIs that try and play it.

Not sure how this works on Mac, but on Windows we tell it to just play a default system sound if it can't play the sound we tell it to.
Seems like using *.mp3 for alarm sound doesn't work on Windows 2000 either.
(In reply to comment #5)
> Seems like using *.mp3 for alarm sound doesn't work on Windows 2000 either.

FWIW: it does on Mac.
Sorry, we tell Windows /not/ to play a default sound if the sound we pass in fails. Also, now that I look at it, Windows can only play WAVs, too. :\
As discussed with jminta on IRC, just filter for WAVs on all platforms for consistency. If someone wants to use an MP3 or other format, they can change it in about:config.

Also, I would've liked to have used "WAV Files" as a description instead of just "*.wavs", but since we're in a string freeze, this isn't possible. :(
Attachment #238455 - Attachment is obsolete: true
Attachment #238741 - Flags: second-review?(jminta)
Attachment #238741 - Flags: first-review?(mattwillis)
Attachment #238455 - Flags: second-review?(jminta)
Attachment #238455 - Flags: first-review?(mattwillis)
Comment on attachment 238741 [details] [diff] [review]
Filter for only WAVs on all platforms

>-        fp.init(window, title, nsIFilePicker.modeOpen);
>+        fp.appendFilter("*.wav", "*.wav");

I think the init() is still required and appendFilter() should just replace existing appendFilters().
Comment on attachment 238741 [details] [diff] [review]
Filter for only WAVs on all platforms

>Index: calendar/base/content/preferences/alarms.js
>===================================================================
>--- calendar/base/content/preferences/alarms.js	19 Jun 2006 18:29:53 -0000	1.1
We're at rev 1.2 after your contribution for bug 344535.
That makes this patch a bit screwy.

>         var title = bundlePreferences.getString("Open");
>-        fp.init(window, title, nsIFilePicker.modeOpen);
>+        fp.appendFilter("*.wav", "*.wav");
> 
>         var ret = fp.show();
As ssitter said, we should leave the fp.init() and simply replace fp.appendFilters() (which is only in rev 1.2)

Please also file a follow-up to fix the string for after the string freeze.
Attachment #238741 - Flags: second-review?(jminta)
Attachment #238741 - Flags: first-review?(mattwillis)
Attachment #238741 - Flags: first-review-
Arg, I really didn't mean to do that, I swear. I guess this is what I get for making patches this late at night.
Attachment #238741 - Attachment is obsolete: true
Attachment #238849 - Flags: second-review?(jminta)
Attachment #238849 - Flags: first-review?(mattwillis)
Comment on attachment 238849 [details] [diff] [review]
Filter for only WAVs on all platforms v1.1

>Index: calendar/base/content/preferences/alarms.js
>===================================================================
>         var bundlePreferences = document.getElementById("bundlePreferences");
>         var title = bundlePreferences.getString("Open");
>-        fp.appendFilters(nsIFilePicker.filterAll);
>+        fp.appendFilter("*.wav", "*.wav");
>         fp.init(window, title, nsIFilePicker.modeOpen);

Looking at other code in lxr, it looks like the "proper order" is:

         fp.init(window, title, nsIFilePicker.modeOpen);
         fp.appendFilter("*.wav", "*.wav");
         fp.appendFilters(nsIFilePicker.filterAll);


http://lxr.mozilla.org/mozilla/search?string=appendFilter


Here's the rub:
The filepicker's not respecting this filter on Mac, and not just for this dialog.


So... r=lilmatt using the above order.
Attachment #238849 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 238849 [details] [diff] [review]
Filter for only WAVs on all platforms v1.1

r2=jminta with the ordering tweak mentioned by lilmatt.
Attachment #238849 - Flags: second-review?(jminta) → second-review+
Attachment #238849 - Attachment is obsolete: true
Attachment #239290 - Flags: second-review+
Attachment #239290 - Flags: first-review+
OS: Linux → All
Hardware: PC → All
Summary: Setting filter on Linux for alarm sounds → Set filter on alarm sound file picker
Whiteboard: [checkin needed]
I filed bug 353437 for localizing the filter string.
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060921 Calendar/0.3a2+
Status: RESOLVED → VERIFIED
Whiteboard: [litmus testcase wanted]
Litmus testcase 2888 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.