Set filter on alarm sound file picker

VERIFIED FIXED

Status

Calendar
Alarms
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Thorsten 'nightrat' Fritz, Assigned: Adam Guthrie)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 238455 [details] [diff] [review]
Untested patch
Assignee: nobody → ispiked
Status: NEW → ASSIGNED
Attachment #238455 - Flags: second-review?(jminta)
Attachment #238455 - Flags: first-review?(mattwillis)
(Assignee)

Comment 2

11 years ago
Make that |#define UNIX_BUT_NOT_MAC|, actually. :)

Comment 3

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

Comment 4

11 years ago
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.

Comment 5

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

Comment 7

11 years ago
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. :\
(Assignee)

Comment 8

11 years ago
Created attachment 238741 [details] [diff] [review]
Filter for only WAVs on all platforms

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 9

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

Comment 11

11 years ago
Created attachment 238849 [details] [diff] [review]
Filter for only WAVs on all platforms v1.1

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 13

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

Comment 14

11 years ago
Created attachment 239290 [details] [diff] [review]
Filter for WAVs and All Files, but default to WAVs
Attachment #238849 - Attachment is obsolete: true
Attachment #239290 - Flags: second-review+
Attachment #239290 - Flags: first-review+
(Assignee)

Updated

11 years ago
OS: Linux → All
Hardware: PC → All
Summary: Setting filter on Linux for alarm sounds → Set filter on alarm sound file picker
Whiteboard: [checkin needed]
(Assignee)

Comment 15

11 years ago
I filed bug 353437 for localizing the filter string.
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
(Reporter)

Comment 17

11 years ago
Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060921 Calendar/0.3a2+
Status: RESOLVED → VERIFIED

Updated

11 years ago
Whiteboard: [litmus testcase wanted]

Comment 18

11 years ago
Litmus testcase 2888 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.