#2866 HIGH 8.2.0 (: Network Manager GUI doesn't report success or failure

[Zarro Boogs per Child]

#2866: Network Manager GUI doesn't report success or failure
----------------------+-----------------------------------------------------
   Reporter:  gnu     |       Owner:  mtd                              
       Type:  defect  |      Status:  new                              
   Priority:  high    |   Milestone:  8.2.0 (was Update.2)             
  Component:  sugar   |     Version:  Development build as of this date
 Resolution:          |    Keywords:  8.2.0:? 9.1.0:+ polish:8.2.0 r+  
Next_action:  code    |    Verified:  0                                
  Blockedby:          |    Blocking:  3993, 6995                       
----------------------+-----------------------------------------------------
Changes (by mtd):

  * keywords:  8.2.0:? 9.1.0:+ polish:8.2.0 r- => 8.2.0:? 9.1.0:+
               polish:8.2.0 r+


Comment:

 Replying to [comment:24 tomeu]:
 > 0001--2866-add-IP-address-to-wireless-ap-palettes.patch
 >
 > {{{
 > 'ip-changed':               (gobject.SIGNAL_RUN_FIRST,
 >                              gobject.TYPE_NONE, ([])),
 > }}}
 >
 > Any reason why this isn't a property like what you have done in
 model/devices/network/mesh.py?

 I think you're looking at nmclient.py.


 > {{{
 >         def _padded(child, xalign=0, yalign=0.5):
 > }}}
 >
 > I'm not sure if we have any benefit from having funcs local to methods,
 as we should strive to write focused classes with few methods (a dozen
 max?). Thus perhaps in sake of keeping things simple we should refrain
 from using local funcs and just use regular private methods? This is just
 my opinion, not sure if it's shared by anyone else here.

 Yeah I can't really justify adding that function on the class, but it
 seems worse to duplicate the code since it's more than a handful of lines
 than the little bit of decrease in simplicity.

 > {{{
 > ip_address_text = "%s: %s" % (_("IP address"), ip_address)
 > }}}
 >
 > Is this correct for all languages? Don't exist languages that would use
 a symbol different to ':' or have different conventions about spaces, word
 positions, etc? I would have done it like this:
 >
 > {{{
 > ip_address_text = _("IP address: %s") % ip_address
 > }}}
 >
 > so that translators have more choice about how to translate it.

 Thanks -- I've fixed that.

 > And I prefer to use ' instead of " because find it's more readable, but
 again it's my personal preference.


 I found plenty of pre-existing "s so I left them in but I've noted your
 feedback and will take it into account in the future.

 > [signal/event callbacks should use __]
 > Although the code you are modifying is from before we adopted this
 convention.

 Yeah, so as discussed on IRC I've left it consistent but undesirable for
 now for this 0.82.0 patch, but my larger (but more invasive) patch for
 master has this change (and changes all event/signal cb methods so they
 are consistently like that).

 > [self._chan_label]
 > I know you are following the local convention in that code, but I really
 dislike using abbreviations in identifiers.

 Again, left as-is on 0.82.0 for consistency.  Will update my bigger master
 patch to take this into account.

 >
 > {{{
 >             ip_address_text = "%s: %s" % (_("IP address"), ip_address)
 > }}}
 >
 > The same string appears two times and thus will be offered for
 translation two times. Perhaps we can add a constant to
 view/devices/network/__init__.py?

 Thanks -- I hadn't thought of that.  I've fixed the issue so we only offer
 the string for translation once, but as discussed on IRC, to avoid
 changing more files than already touched, for 0.82.0, I've put the
 translatable string in wireless.py.  I will update my larger, master patch
 exatcly as you requested.

 >
 > 0003--2866-wireless-mesh-frame-icons-pulse-while-connecti.patch
 >
 > {{{
 > self._icon.props.base_color = pulse_color   # only temporarily
 > }}}
 >
 > What this comment mean?

 This is there because it may seem odd to initialize that variable to that
 value, but it will get set to an appropriate value in _update_state().
 Because the appropriate value is not known without applying the logic in
 _update_state(), because it's possible (though improbable; I'd consider it
 a type of race) for the base_color property to be tested/use between this
 code and _update_state(), and because until this property is set any
 testing/using of it will raise an Exception, I initialize the property to
 a known value so that Exception can't really happen.  If you suggest a
 clearer comment (I didn't want to make it so long that I would have to
 spend a whole line on it), I will definitely incorporate that into my
 larger, master patch.

 As discussed on IRC I think I've handled all your comments, so I'm setting
 r+ and pushing to sucrose-0.82 branch.  I will update my larger patch and
 ask for a review/push to master separately.

 Thanks for the review.

-- 
Ticket URL: <http://dev.laptop.org/ticket/2866#comment:25>
One Laptop Per Child <http://laptop.org/>
OLPC bug tracking system