Opened 12 years ago

Closed 12 years ago

#9 closed defect (fixed)

fix network frame device

Reported by: erikos Owned by: erikos
Priority: major Milestone:
Component: Sugar Version:
Severity: Keywords: r+
Cc: Distribution/OS:
Bug Status:

Description


Attachments (1)

frame_wireless_device.patch (14.4 KB) - added by erikos 12 years ago.
implements the frame device for wireless connections

Download all attachments as: .zip

Change History (4)

Changed 12 years ago by erikos

implements the frame device for wireless connections

comment:1 Changed 12 years ago by erikos

  • Keywords r? added

comment:2 follow-up: Changed 12 years ago by tomeu

  • Keywords r+ added; r? removed
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.

comment:3 in reply to: ↑ 2 Changed 12 years ago by erikos

  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.