Closed Bug 466276 Opened 11 years ago Closed 7 years ago

.caf files grayed out (not selectable) in Preferences > General > Use the following sound file > Browse...

Categories

(Thunderbird :: Preferences, defect)

PowerPC
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: tom, Assigned: foss)

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081006 Shredder/3.0a3

The problem in Thunderbird 2.0.0.18 of Apple .caf sound files (as new mail notifications) not playing has been fixed in Shredder 3.0a. However, .caf files can't be selected in the Browse... dialog box.

However, in Thunderbird 2.0.0.18, the files are selectable but don't play.

I know they play because a .caf file was already selected in my Thunderbird 2.0.0.18 profile, which Shredder imported correctly into the preferences. I just cannot change the sound selected in Shredder 3.0a.

Reproducible: Always

Steps to Reproduce:
1. Start Shredder.
2. Go to Preferences....
3. Under When new messages arrive: check Play a sound.
4. Select Use the following sound file.
5. Click Browse....
6. Find a .caf file. For example:
Macintosh HD:Library:Audio:Apple Loops:Apple:iLife Sound Effects:Ambience:Big Waterfall.caf.
7. All .caf files are grayed out and not clickable. 
Actual Results:  
.caf sound files cannot be selected.

Expected Results:  
Files should be enabled and selectable.

A workaround is to open the profile in an older version of Thunderbird, change the sound file to play, and then reopen it in Shredder. However, this would be inconvenient for end users.
I don't have Mac, but it doesn't seem to have been fixed.

I can only find *.wav in the file-picker:
http://mxr.mozilla.org/comm-central/source/mail/components/preferences/general.js#147

By the way, beta2 has just been released.
Version: unspecified → Trunk
Confirming with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090610 Shredder/3.0b3pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch A fix (obsolete) — Splinter Review
A possible fix - ain't sure it's the right approach - Mark thoughts ?
Attachment #382783 - Flags: review?(bugzilla)
I think the approach is fine, but we want to make it mac-only. Could use ifdefs, but we may have a better way lying around.
(In reply to comment #4)
> I think the approach is fine, but we want to make it mac-only. Could use
> ifdefs, but we may have a better way lying around.

Would removing the filter be better ?
How about something like this:
fp.appendFilter("Audio Files", "*.wav; *.caf");
Where "Audio Files" should be l10n, but I don't really understand the code.
Attachment #382783 - Flags: review?(bugzilla)
Comment on attachment 382783 [details] [diff] [review]
A fix

iirc we discussed this over irc and said that doing a mac specific version of the filter would be better.
Attached patch patch (obsolete) — Splinter Review
Adding filters only on the MacOS platform.
Assignee: nobody → leofigueres
Attachment #655700 - Flags: review?(bwinton)
Comment on attachment 655700 [details] [diff] [review]
patch

This isn't the greatest UI (I would prefer a "Sound Files" which listed all of the types ;), but it'll do.

r=me.

Thanks,
Blake.
Attachment #655700 - Flags: review?(bwinton) → review+
Comment on attachment 655700 [details] [diff] [review]
patch

># HG changeset patch
># Parent 29fbdc5bfaadbbbfb34540e12fe466d41adeee53
># User Javi Rueda <leofigueres@yahoo.com>
>Bug 466276 - .caf files grayed out (not selectable) in Preferences > General > Use the following sound file > Browse...
>
>diff --git a/mail/components/preferences/general.js b/mail/components/preferences/general.js
>--- a/mail/components/preferences/general.js
>+++ b/mail/components/preferences/general.js
>@@ -112,16 +112,17 @@ var gGeneralPane = {
>     // XXX todo, persist the last sound directory and pass it in
>     fp.init(window, document.getElementById("bundlePreferences").getString("soundFilePickerTitle"), nsIFilePicker.modeOpen);
>     fp.appendFilter("*.wav", "*.wav");
>     
>     // On Mac, allow AIFF files too
>     if (Application.platformIsMac) {
>       fp.appendFilter("*.aif", "*.aif");
>       fp.appendFilter("*.aiff", "*.aiff");
>+      fp.appendFilter("*.caf", ".caf");
Shouldn't the 2nd argument be "*.caf"?
(In reply to Ian Neal from comment #10)
> Comment on attachment 655700 [details] [diff] [review]
> patch
> 
> ># HG changeset patch
> ># Parent 29fbdc5bfaadbbbfb34540e12fe466d41adeee53
> ># User Javi Rueda <leofigueres@yahoo.com>
> >Bug 466276 - .caf files grayed out (not selectable) in Preferences > General > Use the following sound file > Browse...
> >
> >diff --git a/mail/components/preferences/general.js b/mail/components/preferences/general.js
> >--- a/mail/components/preferences/general.js
> >+++ b/mail/components/preferences/general.js
> >@@ -112,16 +112,17 @@ var gGeneralPane = {
> >     // XXX todo, persist the last sound directory and pass it in
> >     fp.init(window, document.getElementById("bundlePreferences").getString("soundFilePickerTitle"), nsIFilePicker.modeOpen);
> >     fp.appendFilter("*.wav", "*.wav");
> >     
> >     // On Mac, allow AIFF files too
> >     if (Application.platformIsMac) {
> >       fp.appendFilter("*.aif", "*.aif");
> >       fp.appendFilter("*.aiff", "*.aiff");
> >+      fp.appendFilter("*.caf", ".caf");
> Shouldn't the 2nd argument be "*.caf"?

Oops. Yes, it is true. I am going to fix it in a while. Sorry for the mistake.

I am not a Macintosh user so I was unable to test it :-(
I would like to work on the improvement from Blake in comment 9. Patch will not be ready for the next merge, anyway.
Attached patch patch 2 (obsolete) — Splinter Review
All files extensions have been grouped so they could be shown as "Sound files".

The string "Sound files" has been created. This will need localization on the other supported languages.

As line length was longer than 80 chararacters, code directly affected was refactorised using variable bundlePrefs and soundFilesText.

Note that this patch was tested on a Windows system, not on a MacOS X. I am not able to cross-compile it, either.
Attachment #655700 - Attachment is obsolete: true
Attachment #658031 - Flags: review?(bwinton)
Comment on attachment 658031 [details] [diff] [review]
patch 2

>+++ b/mail/components/preferences/general.js
>@@ -106,23 +106,24 @@ var gGeneralPane = {
>-    // On Mac, allow AIFF files too
>-    if (Application.platformIsMac) {
>-      fp.appendFilter("*.aif", "*.aif");
>-      fp.appendFilter("*.aiff", "*.aiff");
>-    }
>+    // On Mac, allow AIFF and CAF files too
>+    var bundlePrefs = document.getElementById("bundlePreferences");
>+    var soundFilesText = bundlePrefs.getString("soundFilesDescription");
>+    if (Application.platformIsMac)
>+      fp.appendFilter(soundFilesText, "*.wav; *.aif; *.aiff; *.caf");
>+    else
>+      fp.appendFilter(soundFilesText, "*.wav");

I like it!  I especially like how, on Mac, since there's only one filter now, we don't get the drop-down allowing us to choose between different (unuseful) file types.  :)

>+++ b/mail/locales/en-US/chrome/messenger/preferences/preferences.properties
>@@ -54,16 +54,17 @@ typeDetailsWithTypeOrExt=(%1$S)
>+soundFilesDescription=Sound files

This should be "Sound Files", as per the other filters I've seen.  (The "Save As…" Formats, for example.)

r=me, and ui-r=me with those fixed!

Thanks,
Blake.
Attachment #658031 - Flags: ui-review+
Attachment #658031 - Flags: review?(bwinton)
Attachment #658031 - Flags: review+
Amended string for sound file types. Reviewed in previous comment.
Attachment #382783 - Attachment is obsolete: true
Attachment #658031 - Attachment is obsolete: true
Attachment #660860 - Flags: ui-review+
Attachment #660860 - Flags: review+
https://hg.mozilla.org/comm-central/rev/fc2a8fd24ed1
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.