Opened 14 years ago
Closed 14 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)
Change History (4)
Changed 14 years ago by erikos
comment:1 Changed 14 years ago by erikos
- Keywords r? added
comment:2 follow-up: ↓ 3 Changed 14 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 14 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 disconnectedWhat 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_opCouldn'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.
implements the frame device for wireless connections