GNOME Bugzilla – Bug 682087
[PATCH] add usb mass storage hardware support on OS X
Last modified: 2012-08-19 20:30:04 UTC
Created attachment 221588 [details] [review] add usb mass storage hardware support on OS X See the patches commit message for description. This is a rather large patch but only happens in the Osx.Backend, shouldn't affect the linux build.
Review of attachment 221588 [details] [review]: Wow, that is an impressive patch! As I can't compile this, just a few nitpicks and questions, so that I don't feel completely useless ;) ::: src/Backends/Banshee.Osx/Banshee.Hardware.Osx/LowLevel/CoreFoundation.cs @@ +1,2 @@ +// +// OsxDiskArbiter.cs That's not the right filename ::: src/Backends/Banshee.Osx/Banshee.Osx.addin.xml @@ +20,2 @@ <Extension path="/Banshee/Platform/HardwareManager"> + <HardwareManager class="Banshee.OsxBackend.HardwareManager" id="Banshee.OsxBackend.HardwareManager" /> Just for my information, why is the id attribute needed ? ::: src/Backends/Banshee.Osx/Banshee.OsxBackend/HardwareManager.cs @@ +6,2 @@ // +// Copyright (C) 2012 Timo Dörr Please leave the previous copyright info there when adding yourself, even if there's not much left of the original code. @@ +36,3 @@ +using System.Linq; + +using Banshee.Dap.MassStorage; Is that really needed? Not depending on Banshee.Dap.MassStorage would also allow dropping some changes to Makefile.am and the .csproj @@ +66,2 @@ } + private void deviceAppeared (object o, DeviceArguments args) Please name the methods with an upper case starting letter.
Created attachment 221743 [details] [review] fixed version of the previous patch I fixed those issues: * removed unneccesarry Banshee.Dap.MassStorage referene from file & Makefile.am * fixed the filenames in the copyright note in several more files * uppercased the methodnames The "id" field in the Banshee.Osx.addin.xml manifest is required. I just tested, if I remove it the HardwareManager won't be loaded. I am not perfectly shure why this is, I oriented myself on Banshee.Gio.addin.xml which uses the id field for the HardwareManager on GIO, too. For the Copyright issue in HardwareManager.cs, I did not remove the copyright statement. I've started from scratch with a new file, the header got generated by MonoDevelop, thats why there is only one copyright message. Since HardwareManager implements IHardwareManager, git "detects" changes which are just the stub function definitions of the implemented IHardwareManager methods. I really don't mind re-adding another copyright note, but since I've started from scratch on that file, there shouldn't be any legal issues if its missing. Perhaps you could do a "git rm HardwareManager.cs", and afterwards apply my patch and commit? That way the file delete should be in the git history, which I can't do when sending a patch. I've setup a branch osx-hardware on github against latest master, with the included "patches for the patch", for your better review here: https://github.com/Dynalon/banshee-osx/pull/2 (add .diff for the patch) If also attached the fixed patch, that I recreated with "git merge --squash osx-master" against git master.
Comment on attachment 221743 [details] [review] fixed version of the previous patch Thanks for the changes, committed with some cosmetic changes (code formatting, etc.) For the id attribute, you're right, it's used by Banshee.Hardware.HardwareManager to identify the backend. Don't worry about the copyright thing, it's fine like that, I just wanted to make sure it wasn't a mistake.