#7248 NORM 8.2.0 (: Speaker device has inconsistent behavior

[Zarro Boogs per Child]

#7248: Speaker device has inconsistent behavior
-------------------------+--------------------------------------------------
   Reporter:  Eben       |       Owner:  mtd                              
       Type:  defect     |      Status:  assigned                         
   Priority:  normal     |   Milestone:  8.2.0 (was Update.2)             
  Component:  sugar      |     Version:  Development build as of this date
 Resolution:             |    Keywords:  8.2.0:+ r?                       
Next_action:  never set  |    Verified:  0                                
  Blockedby:             |    Blocking:  7420                             
-------------------------+--------------------------------------------------
Changes (by mtd):

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


Comment:

 Replying to [comment:16 tomeu]:
 > {{{
 > +        'muted-changed' : (gobject.SIGNAL_RUN_FIRST,
 > +                           gobject.TYPE_NONE,
 > +                           ([gobject.TYPE_BOOLEAN,
 gobject.TYPE_BOOLEAN])),
 > +        'volume-changed': (gobject.SIGNAL_RUN_FIRST,
 > +                           gobject.TYPE_NONE,
 > +                           ([gobject.TYPE_INT, gobject.TYPE_INT])),
 > }}}
 >
 > Would be nice to convert muted and volume into properties, so we
 just used notify::muted to detect changes, but is not worth to delay
 this patch, I think.

 Ok...I will take that approach in the future.

 > Also wonder if makes any sense to keep hardwaremanager.py. Maybe it
   should be merged into the different device model classes? We may
   enter tickets for 9.1, if agree on this.

 I would agree, and I heard from marcopg on IRC yesterday that this
 should go away. My issue with hardwaremanager.py is that it gets
 between the simple model-view relationships implied by the
 sugar/{model,view}/devices subtree.  I had a go at removing it but
 it's a bit of work for little practical benefit, I think.  The reason
 it exists, I think, is that the network "devices" really aren't - they
 are a way of exposing NetworkManager-provided information; in fact
 NetworkManager is really the "device".  Anyway, yes, I agree it
 doesn't make sense, if we were starting from scratch, to write
 hardwaremanager.py exactly as it is.

 > {{{
 > +        self._sigids = []
 > }}}
 >
 > This doesn't seem to be used?

 Whoops, removed.

 >
 > {{{
 > +        vol_step = 10
 > }}}
 >
 > Perhaps this should be a private constant? (in upper case and at the
 > class or module level)

 I've made it a public constant in hardwaremanager.py and changed
 view/keyhandler.py and view/devices/speaker.py to use it.  I thought
 about making it a constant, and didn't because I thought this other
 extreme was perhaps less palatable.  Well now you can choose.  I
 didn't see much point in having two private constants that were meant
 to achieve the same thing (the middle ground), but please reject &
 explicitly ask for this again if you prefer it that way, and I'll Just
 Do It.

 > {{{
 > +                                          upper=100 + vol_step,
 > }}}
 >
 > The upper limit is 110? I thought upper was a valid value in the
 > adjustment.

 If I set upper=100 the max adjustment.value I ever see is 90.  Perhaps
 I'm doing something else wrong?

 >
 > {{{
 > +                                          step_incr=vol_step,
 > }}}
 >
 > Wouldn't be nice to be able to move the slider smoothly percent by
 > percent?

 You can do this "smooth" movement via the mouse, even with step_incr.
 I didn't think it was that useful to do via the keyboard arrow keys,
 as the keyboard volume up/down keys move by this step as well.  I'm no
 expert on whether the smooth movement via the arrow keys is more/less
 desirable than the resulting inconsistency between the arrow keys and
 the volume up/down keys.  I have no strong feeling either way.  So of
 the: a) mouse drag; b) arrow key press; and c) volume key press user
 actions, which (if any) do you want to be done smoothly, or "clamped"
 to step_incr?


 > The rest of the patch looked very good to me, thanks!

 Thanks for the review.  In my re-submitted patch, I have removed a few
 logging lines from view/hardwaremanager.py and view/devices/speaker.py
 that had somehow snuck in before.

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