767655 - New messages arrive sound file browser wont select ogg files

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Rubin110]

Debian Sid 64 bit, running XFCE with a little bit of Gnome here and there, Thunderbird Daily 16.0a1 (2012-06-19).

Steps to reproduce
1. Make the most amazing recording of a baby bird chipping through an egg shell and record it as an ogg file
2. Load up a recent daily build of Thunderbird
3. Go to preferences, general tab
4. Select Play Sound, Use the following sound file
5. Click Browse
6. Locate your amazing baby bird chipping ogg file
7. Observe

Expect result
Ogg file is selectable through the file browser

Actual result
Only *.wav files can be display in the file browser

Notes
Thunderbird seems fine with playing ogg files, tested by simply renaming the more amazing recording from .ogg to .wav. Thunderbird plays it like a champ.

Comment 1

[Jim Porter (:squib)]

Confirming. This should be easy to fix.

Comment 2

[Javi Rueda :javirid]

Almost finished with bug 466276. Then I will "work" on this one.

Comment 3

[Javi Rueda :javirid]

Unable to work on this, as my computer is down.

One way to try this is using https://developer.mozilla.org/en-US/docs/Code_snippets/Miscellaneous#System_info.

Another way could be https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests#Runtime_detection

:-)

Comment 4

[Javi Rueda :javirid]

Fixed my computer. Re-taking this bug.

Comment 5

[Ludovic Hirlimann [:Usul]]

Javi if you need help #maildev on irc.mozilla.org is the place to ask.

Comment 6

[Javi Rueda :javirid]

Attachment: Screenshot of the dialog

Screenshot showing how it looks the open sound file dialog on Ubuntu. Notice that OGG files are now selectable.

Comment 7

[Javi Rueda :javirid]

Attachment: patch

This patch uses the code snippet in MDN for detect the underlying OS where the application is running.

Some readibility improvements have been done in order to keep the line length shorter than the 80-chars limit.

No "else" clause because other systems haven't been tested.

I have tested this patch myself on Windows (Win7 x32) and Linux (Ubuntu x64). The Ubuntu test was run on Unity and Gnome.

Last reviewer of this particular code was Blake. Could you take a look into this, Blake? Thank you.

The screenshot was taken on Ubuntu with Gnome as a desktop.

Comment 8

[Javi Rueda :javirid]

(In reply to Ludovic Hirlimann [:Usul] from comment #5)
> Javi if you need help #maildev on irc.mozilla.org is the place to ask.

Thank you, Ludovic. I would have gone to the introduction or the Firefox channel. I am going to spend some time now on TB so there I would be from now on :-)

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 672417 [details] [diff] [review]
patch

>+++ b/mail/components/preferences/general.js
>@@ -101,22 +101,29 @@
>-    // On Mac, allow AIFF and CAF files too
>     var bundlePrefs = document.getElementById("bundlePreferences");
>     var soundFilesText = bundlePrefs.getString("soundFilesDescription");
>-    if (Application.platformIsMac)
>+    const Cc = Components.classes, Ci = Components.interfaces;
>+    var osString = Cc["@mozilla.org/xre/app-info;1"]
>+                      .getService(Ci.nsIXULRuntime).OS;
>+
>+    if (osString=="Darwin")

I really think we prefer to use Application.platformIsMac…  If that doesn't work for Linux, adding a platformIsLinux would seem to be a better solution, but…

>+      // On Mac, allow AIFF and CAF files too
>       fp.appendFilter(soundFilesText, "*.wav; *.aif; *.aiff; *.caf");
>-    else
>+    else if (osString=="Linux")
>+      // On Linux, allow for OGG files
>+      fp.appendFilter(soundFilesText, "*.wav; *.ogg");
>+    else if (osString=="WINNT")
>       fp.appendFilter(soundFilesText, "*.wav");

Are you sure Mac and Windows can't play .ogg files?  Opening a web tab to http://en.wikipedia.org/wiki/File:Rondo_Alla_Turka.ogg in Thunderbird plays just fine on my MacBook Pro.  If the .ogg files can be played everywhere, then let's just add them everywhere.

Thanks,
Blake.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

(Oh, I did also want to say "Thank you for the patch!  And I look forward to reviewing the next iteration!"  :)

Comment 11

[Javi Rueda :javirid]

The option to enable OGG files on all platform was firstly tested by me.

On Windows OGG files aren't played on Windows Media Player. To enable them, codecs must be installed. Then it works for any player which uses DirectShow Filters. This doesn't affects to Firefox, as it seems to use its own codec. Maybe you played that Wikipedia file from the site itself?

On Linux, OGG files are playable out-of-the-box.

These are the reasons why I chose to not include OGG files on the Windows code.

What is true, however is that, looking at other TB code, they are always using platformIsMac, not the platform detection common code. It is true, also, that that code didn't need to separate all three platforms, just need to do things differently on Mac.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Okay, let's only add it for Linux, then, and add the platformIsLinux attribute to the Application object.

Thanks,
Blake.

Comment 13

[Jim Porter (:squib)]

Why wouldn't we support Ogg on Windows? Thunderbird should play Ogg perfectly well regardless of platform. Just because Ogg is less common on Windows doesn't seem like a good reason to me to disallow users from using Ogg files.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

It seemed, from my reading of the code, and from Javi's comment, that we call out to PlaySystemSound, and don't play the files from within Thunderbird, so we can't actually play Ogg regardless of platform.

If you can play Ogg files on Windows (and/or Mac), using the play button in the preferences dialog, without installing third-party codecs, please leave a comment, because that would change my decision.

Thanks,
Blake.

Comment 15

[Javi Rueda :javirid]

Doing a search at the web, there are people who aren't able to play some OGG files -where TB enables them to choose- while they have no problems with some others.

I downloaded the files that are used on Debian, but they didn't play neither.

Comment 16

[Rubin110]

As I stated in the bug description, I can play ogg files just fine through Debian Sid running some bastard combination of XFCE4 and Gnome3.

Might sound a bit naive but I figured there might simply be a way to poke whatever host OS you're running off of and ask what mime types of audio files it can play, then simply only list those out in the file browser?

Comment 17

[Javi Rueda :javirid]

Attachment: Enabling OGG file selections on Mac and Linux

Need to shift between Linux and Windows, so, for the moment, just leaving it unflagged as I am going to test on my other OS.

Comment 18

[Javi Rueda :javirid]

I have added a platformIsLinux attribute to the steelApplication Interface and implemented the code.

I am mostly following the same procedure that Mark Banner did on bug 500123 that made this changes http://hg.mozilla.org/comm-central/rev/f9f606a5f2bd

Right now I am trying to create the xpcshell-tests.

Last patch I sent was just because I needed to store it in some place. Code isn't good enough as the comment is inserted above the platformIsMac definition.

I also tested the patch that wanted to enable all platforms to play OGG files on Windows and without the codecs installed and it didn't work.

Comment 19

[Ludovic Hirlimann [:Usul]]

Javi , you should request reviews if you want to go forward with this.

Comment 20

[Javi Rueda :javirid]

(In reply to Ludovic Hirlimann [:Usul] from comment #19)
> Javi , you should request reviews if you want to go forward with this.

This wasn't a patch intended to be reviewed but a temporary store for it while I switched between Linux and Windows in order to test it.

Working on a new one, including tests, as it is going to change the steelApplication interface.

Comment 21

[Javi Rueda :javirid]

Attachment: patch 2

Modifies interface steelIApplication adding a new platformIsLinux attribute.

Patch will enable to select OGG files both on Mac OSX and Linux platforms. Tests on Windows operating system shows that it is not possible to play these sound files out-of-the-box.

I tested this patch on Linux and on Windows. Maybe you could test in on Mac, Blake?

Comment 22

[Javi Rueda :javirid]

Attachment: Tests for patch 2

Here are the tests for the patch I have just submited. Tests have been splitted in order to make it easy to review. If only one is needed, just ask for it and I will send a new one.

The previous code only took into account Mac and "other" systems. Now it is needed to differentiate between Mac and Linux, so the tree needed some changes.

Directory /notmac is not needed, as platform could be not Mac, but we are still in need of see if it is Linux. Then, 2 new directories are created /linux and /notmaclinux.

Those new directories are basically the same, except they will expect different results from the tests.

Each attribute -platformIsMac and platformIsLinux- is tested in its own file. Original test_platform.js tests for platformIsMac attribute.

Could you take a look at this patch, Mark? Thank you :-)

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 678551 [details] [diff] [review]
patch 2

>+++ b/mail/components/preferences/general.js
>@@ -105,17 +105,19 @@
>     if (Application.platformIsMac)
>-      fp.appendFilter(soundFilesText, "*.wav; *.aif; *.aiff; *.caf");
>+      fp.appendFilter(soundFilesText, "*.wav; *.aif; *.aiff; *.caf; *.ogg");

I finally got a chance to test this on Mac, and it looks like we can't play ogg files, so you'll need to revert this change.

Other than that, it seems fine.  r=me with that fixed, and thank you for fixing this bug!

Later,
Blake.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 678558 [details] [diff] [review]
Tests for patch 2

Review of attachment 678558 [details] [diff] [review]:
-----------------------------------------------------------------

When we wrote the ifdefs in the Makefile for the mac/ sub-directory etc, we didn't have the platform support in xpcshell.ini. I think it would be better to drop the sub-directories now, and just name the files differently and have them all in one directory, with the one xpcshell.ini doing the platform selection.

Comment 25

[Javi Rueda :javirid]

Naming files test_mac_platformIsMac, test_linux_platformIsMac and test_other_platformIsMac would be OK, Mark? The same with platformIsLinux.

Comment 26

[Mark Banner (:standard8)]

Yes, that looks fine.

Comment 27

[Javi Rueda :javirid]

Attachment: patch 3

This is a new patch including both older -and now obsoloted- patches.

I have made the change so Mac users will not see the OGG sound filter on the dialog box, Blake. Also, thank you for your review and for checking it worked on a Mac system.

In Makefile.in, references for tests directories have been replaced by the new /test directory. Directories /mac and /notmac have been removed.

New test files have been created with names test_(mac/linux/other)_platformIs(Mac/Linux). Those new files are referenced on xpcshell.ini at the same directory and executed depending on the OS platform at the testing system.

I wasn't able to execute a block of tests depending on the platform, so there are conditional statements for doing it for every single test at xpcshell.ini.

At global mail/test/xpcshell.ini references for the old tests have been replaced by a new reference for mail/steel/test/xpcshell.ini.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 681921 [details] [diff] [review]
patch 3

Review of attachment 681921 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of minor comments, as these are minor, I'll address them and get this landed for you.

Thanks for the patch and for updating the tests.

::: mail/steel/steelIApplication.idl
@@ +15,5 @@
>     * doesn't support print preview on Mac because Mac provides a preview option
>     * in the print dialog.
>     */
>    readonly attribute boolean platformIsMac;
> +  /**

nit: insert a blank line before the comment please

::: mail/steel/test/test_linux_platformIsLinux.js
@@ +23,5 @@
> +  inSafeMode: false,
> +  logConsoleErrors: true,
> +  OS: "XPCShell",
> +  XPCOMABI: "noarch-spidermonkey",
> +  

nit: unnecessary whitespace on empty line - it occurs in the same place in all the test files, so probably not your fault, but could you remove it please.

Comment 29

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/comm-central/rev/5e40b999b002

Thanks Javi.

Comment 30

[Javi Rueda :javirid]

Thank you for the review and for fixing those problems yourself, Mark. Because of the change to the steelIApplication interface, maybe it is needed a "dev-doc-needed" flag, but I am not sure of this and don't know if we should wait to this code gets to the release stage.

Thank you for your review and for checking it on a Mac system, Blake.

Comment 31

[Ludovic Hirlimann [:Usul]]

We didn't change interface, so no need to create documentation for devs. removing the keyword