GNOME Bugzilla – Bug 611991
N900 does support ogg and flac with extra package - banshee not
Last modified: 2010-03-28 02:50:40 UTC
* Install "Extra Decoders Support" on N900 (http://maemo.org/downloads/product/Maemo5/decoders-support/) * Connect the device to your PC * Open Banshee and wait for detection of the phone in Banshee * Right click on "Nokia N900 Phone" -> device preferences (? - "Geräteeigenschaften" in german) * WAV and mp3 are selectable codecs only (if the right gstreamer-plugins are installed of course) EXPECTED: WAV, mp3, ogg, flac, (aac)... are selectable codecs. The "Extra Decoders Support" is a well known standard extension to the N900. With the new automatic detection of the phone in Banshee 1.5.4 these codecs cannot be used anymore (was possible with .is_audio_device which is ignored now). For free linux distributions (like Debian) ogg/flac are the only "legal" methods to use the phone but cannot be selected anymore. I do not know what would be better: hard code all possible codecs (including audio/ogg and audio/flac) though failing without the "Extra Decoders" on the device or any possible detection of the codecs on the device and then make them selectable (first possibility is simple - just adding the codecs in MaemoDevice.cs which works for me, second possibility may not work because there are no standard indications on the mounted directories for ogg/flac support)
Created attachment 156110 [details] [review] Patch: Read the .is_audio_player-file if it exists to configure paths and codecs By default the Nokia N900 does not support Ogg Vorbis, but it is possible to add this by installing the correct package on the phone. As Banshee does not know if the user has installed this package, it defaults to only support the MP3 codec. By reading the .is_audio_player-file the user can add the ogg vorbis codec information, to let Banshee support Vorbis on the N900. It also allows the user to configure the paths for audio and video
Review of attachment 156110 [details] [review]: Thanks for the patch! ::: src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MaemoDevice.cs @@ +104,3 @@ public override bool LoadDeviceConfiguration () { + if (File.Exists (IsAudioPlayerPath)) { I'd rather we not re-write the .is_audio_player parser here; I'd rather we reuse the parser in the base.LoadDeviceConfiguration, and make whatever changes are needed to properties etc to make that work.
Ok, i will rewrite the patch. I guess the cleaner way to do this is to extract a new class: IsAudioPlayerParser (i don't think it needs an interface since it will be pretty unique), so that it can be used where necessary. One question: I read in the coding guidelines that Banshee.IO should be used instead of System.IO when possible. But in MassStorageDevice.cs it is used to read the file. Is this correct or should it also be replaced by a call to something in Banshee.IO?
Created attachment 156568 [details] [review] Rewrote the patch. Also added this support for the other devices (Android and WebOS) This patch is a new implementation of the optional .is_audio_player file. It changes a lot more, but keeps the logic of the .is_audio_player file in the MassStorageDevice class. Every MassStorageDevice now has two properties for every setting, the DefaultValue and the final Value. The final Value is chosen depending on the availability of the .is_audio_player file and the correct key in this file. I have updated the code for MaemoDevice, AndroidDevice and WebOSDevice. By default they should work exactly as before, but they now all allow an optional .is_audio_player file. I tested this with a regular usb-stick and my N900 (i don't have an Android or WebOS device). To support the optional .is_audio_player file a Device-class needs to override all the Default*-properties, and call the LoadConfig() method in the LoadDeviceConfiguration() method.
Please re-attach the patch as a text file.
Created attachment 156587 [details] [review] Part 1 of the patch (modified MassStorageDevice and MaemoDevice)
Created attachment 156588 [details] [review] Part 2 of the patch (modified AndroidDevice and WebOSDevice)
Hey Jeroen, thanks for the patches. That part 1 patch doesn't look like it would apply against master - it's removing the pieces from your earlier patch I guess. Can you attach a patch (or two) that doesn't have that cumulative dependency - just applies against master?
@Gabriel: Ah yes that is true... I will need to figure out how to do this as I'm really new to git and Linux development (i work as a .net programmer on that other os). The problem is that i have commited my previous patch in my local git tree, so i would need to be able to remove that commit from my tree somehow i guess... I have downloaded the Git Community Book, but have not yet had the time to really start studying it. I'll try to create a good patch tomorrow. Also please comment if i did not yet follow all the preferred coding guidelines. I tried to follow them, but they are in some places very different from .net (not to say, 'weird' if you are used to the .net coding guidelines :)) so i might have made some mistakes. thx!
Hey Jeroen, the coding style looks fine. You can squash local commits with git rebase --interactive HEAD~N Where N how many commits you want to possibly rebase, so probably 2 or 3 or something. Then you can squash commits together, and modify the commit message.
Created attachment 156639 [details] [review] [MassStorage] Rewrote my patch to add support for an optional .is_audio_player file on the N900 and also updated Android and WebOS device classes I think this should be correct... i hope :)
This patch works as expected and fine for me (Debian banshee 1.5.6-1 inserted patch into source).
Committed, thanks Jeroen! commit 0628224f2b813ce12552f58faedea69a382c181b Author: Jeroen Budts <jeroen@lightyear.be> Date: Sun Mar 28 13:29:47 2010 +1100 [Dap.MassStorage] Read .is_audio_player (bgo#611991) If the file exists, use custom settings for Android, Maemo and WebOS devices. Signed-off-by: Alexander Kojevnikov <alexander@kojevnikov.com>