Comment on attachment 144441 [details] [review]
Wicd support patch

Functionally, this patch looks pretty sane after a quick review, however, it needs to following the HACKING style guidelines. This file is available at the top of the project.

Three things that stand out at a quick glance: not using spaces (4) and instead using hard tabs; always use { } around conditionals; and finally, interface definitions should be pulled out into their own files. EventArgs classes and delegates can be grouped into the main class or interface file.

Thanks!

Created attachment 144443 [details] [review]
Wicd support patch v2

Here you go with the problems you mentioned hopefully fixed. Thanks for the quick reply!

Created attachment 144446 [details] [review]
Wicd support patch v2.1

Fix for namespace and Makefile.am

Getting better. Now that it's easier to read, a few more things stand out. Hopefully I'm not coming off as too annoying or nit-picky, but it's always good to call these things out sooner than later :)

a) please explicitly mark private members as such (e.g. private IWicd manager;)

b) there are some lines that have unnecessary changes (e.g. whitespace only changes) and particularly one change where two Catalog.GetString lines are indented causing a pointless diff section

c) the ManagerPresent method should be a property (IsManagerPresent) instead

d) In GetTranslatedCode on the Wcid class, you should indent the case labels:

switch (wicdCode) {
    case NOT_CONNECTED:
        return State.Disconnected;
    ...
}

e) I'd prefer an enum for the Wcid codes instead of consts, but since it's private, I'd let it slide - just a bit cleaner. You can pass this to DBus as well, so overall the APIs are cleaner.

enum WcidCodes : uint {
    NotConnected = 0,
    Connecting,
    Wireless,
    Wired,
    Suspended
}

f) In new code, please put a space before parenthesis (on method calls and conditinals, etc.), but don't change any old code that doesn't need changing :)

Thanks!

Created attachment 144467 [details] [review]
Wicd support patch v2.2

Hopefully last and good patch ;-) .

Created attachment 144468 [details] [review]
Wicd support patch v2.2b

Grr, some tabs slipped through. This is the good patch.

Ping ? :-)

Created attachment 145869 [details] [review]
Patch committed to master

Hi Jérémie, I've committed this to master, with a few small variations:

 a) StatusStruct, IWicd, and StateChangeInternalHandler are now private inner types of the Wicd class instead of internal types global to the Banshee.Networking namespace

 b) Similarly I did the same for the INetworkManager interface

 c) I moved State and StateChangedHandler from NetworkManager.cs to State.cs

 d) Fixed a few outstanding syntax/style/whitespace issues

Thanks!