[Bugs] #1592 UNSP: NEW FEATURE: enhanced color selector

[Sugar Labs Bugs]

#1592: NEW FEATURE: enhanced color selector
------------------------------------------+---------------------------------
    Reporter:  walter                     |          Owner:  tomeu      
        Type:  enhancement                |         Status:  new        
    Priority:  Unspecified by Maintainer  |      Milestone:  0.88       
   Component:  sugar                      |        Version:  Unspecified
    Severity:  Unspecified                |       Keywords:  r!         
Distribution:  Unspecified                |   Status_field:  Unconfirmed
------------------------------------------+---------------------------------

Comment(by tomeu):

 > +"""
 > +class StopButton(ToolButton):

 Better remove that code altogether.

 > +class ColorPrev(EventIcon):

 Watch out abbrs. Maybe ColorPreviousButton?

 > +        'color-changed': (gobject.SIGNAL_RUN_FIRST,

 Who listens for this signal (and the others with the same name)?

 > +class ColorNext(EventIcon):

 Quite a bit of duplicated code, what about having a single ColorButton
 class that accepts a "direction" arg in its constructor with the constants
 _DIRECTION_LEFT or _DIRECTION_RIGHT?

 > +        # self.icon.props.icon_name = 'view-refresh'
 > +        # self.icon.props.accelerator = '<Ctrl>z'

 Better to avoid pushing commented out code because tends to stay there
 forever and ends up being a mess. If you want to remember about an
 improvement, consider adding a TODO comment with a url to a ticket in
 trac.

 > +        xocolor = XoColor()

 Liked better xo_color like above.

 > +        xocolor = XoColor()
 > +        xocolor.set_color("#FFFFFF,#FFFFFF")
 > +        self.icon.props.xo_color = xocolor

 Why not just self.icon.props.xo_color = XoColor('#FFFFFF,#FFFFFF') ?

 > +class Prev(EventIcon):

 Maybe PreviousButton?

 > +        self._xo_next_color = XoColor(self._xo_color.get_next_color())
 > +        self._xo_prev_color = XoColor(self._xo_color.get_prev_color())

 Do we need those variables in AboutMe, given that we already have
 references to self._color_prev and self._color_next?

 > +    def set_prev_colors(self):
 > +        # update next color to the current color
 > +        self._xo_next_color.set_color(self._xo_color.to_string())
 > +        self._color_next.icon.props.xo_color = self._xo_next_color
 > +        self._color_next.emit('color-changed',
 self._xo_next_color.to_string())
 > +        # update color picker to the prev color
 > +        self._undo_colors = self._xo_color.to_string()
 > +        self._xo_color.set_color(self._xo_prev_color.to_string())
 > +        self._color_picker.icon.props.xo_color = self._xo_color
 > +        self._color_picker.emit('color-changed',
 self._xo_color.to_string())
 > +        # update prev color to its prev color
 > +
 self._xo_prev_color.set_color(self._xo_prev_color.get_prev_color())
 > +        self._color_prev.icon.props.xo_color = self._xo_prev_color
 > +        self._color_prev.emit('color-changed',
 self._xo_prev_color.to_string())

 What about this instead?

 {{{
     def set_widgets_to_previous_color(self):
         self._undo_colors = self._color_picker.get_color()

         current_color =
 self._color_picker.get_color().get_previous_color()
         previous_color = current_color.get_previous_color()
         next_color = self._color_picker.get_color()
         self._update_buttons_colors(previous_color, current_color,
 next_color)

     def _update_buttons_colors(self, previous_color, current_color,
 next_color):
         self._color_previous.set_color(previous_color)
         self._color_picker.set_color(current_color)
         self._color_next.set_color(next_color)
 }}}

 Otherwise we end up with a lot of very similar code that gets difficult to
 read.

 > +"""

 More dead code to remove.

 > +    def __init__(self, me, **kwargs):

 I'm starting to suspect that "me" is a XoColor, if so, what about calling
 it owner_color?

 > +        self._xo = CanvasIcon(size=style.XLARGE_ICON_SIZE,

 self._xo_icon?

 > +class ColorPrev(hippo.CanvasBox, hippo.CanvasItem):

 See comment before about duplicated code.

 > +        self._p = colorpicker.Prev(self)

 I know the original code was already trying to save letters, but now they
 aren't as expensive as before!

 If we are going to have the same UI in the control panel and in the intro
 window, then both places should reference a new module in jarabe/view
 which would contain the common code.

 > +        print "Language button pressed"

 Better not include unfinished features.

-- 
Ticket URL: <http://bugs.sugarlabs.org/ticket/1592#comment:2>
Sugar Labs <http://sugarlabs.org/>
Sugar Labs bug tracking system