fix network frame device

[erikos]

implements the frame device for wireless connections

[tomeu]

def freq_to_channel(freq): 

Why not having a global dict called frequency_to_channel instead? Not sure it's much better, just a suggestion.

        self._chan_label = gtk.Label() 

Any good reason for the abbreviation? Cannot we have _channel_label instead? If we could improve all the chan and freq variables, I think it could help quite a bit later to casual readers/bugfixers.

def connecting
...
def connected
...
def disconnected

What about set_connecting_state(), set_connected_state(), etc? or a single set_state() receiving a state constant? method names use to have a verb in imperative form, which should convey what will happen when it gets executed.

    def _disconnect_activate_cb(self, menuitem): 

should we have one more underscore there?

        self._palette = WirelessPalette(self._name, self) 

Could we avoid somehow this circular reference? Maybe by the palette firing a deactivate-clicked signal?

self._active_ap_op

Couldn't manage to discover what "op" means here.

No more nipicks, but if we could avoid having so many gratuitous abbreviations that don't bring any value, I think it would be great. When fixing a bug or reading a stack trace, it's better to not have to read the whole module in order to understand what's going on.

[erikos]

Replying to tomeu:

{{{
def freq_to_channel(freq):
}}}

Why not having a global dict called frequency_to_channel instead? Not sure it's much better, just a suggestion.

{{{
self._chan_label = gtk.Label()
}}}

Any good reason for the abbreviation? Cannot we have _channel_label instead? If we could improve all the chan and freq variables, I think it could help quite a bit later to casual readers/bugfixers.

done.

{{{
def connecting
...
def connected
...
def disconnected
}}}

What about set_connecting_state(), set_connected_state(), etc? or a single set_state() receiving a state constant? method names use to have a verb in imperative form, which should convey what will happen when it gets executed.

i made it setters, makes sense.

{{{
def _disconnect_activate_cb(self, menuitem):
}}}

should we have one more underscore there?

Yup.

{{{
self._palette = WirelessPalette(self._name, self)
}}}

Could we avoid somehow this circular reference? Maybe by the palette firing a deactivate-clicked signal?

Made it a signal - thanks very much for that tip.

{{{
self._active_ap_op
}}}

Couldn't manage to discover what "op" means here.

'op' stands for object_path. I thought this might be too long to make it self._active_ap_object_path. At least i was consistent on the patch with that usage.

No more nipicks, but if we could avoid having so many gratuitous abbreviations that don't bring any value, I think it would be great. When fixing a bug or reading a stack trace, it's better to not have to read the whole module in order to understand what's going on.

If we want to get rid of the abbreviations we should consider adjusting the limit of 80 chars as well. i guess 10 more chars could help there already. In general i will try to avoid abbreviations.

Thanks for the detailed review.