NEW FEATURE: enhanced color selector

[walter]

This is an enhancement to the color selector used in the About Me control panel and the initialization screen (same behavior but a different code base). It enables the user to cycle through color choices as well as jump randomly, which is the current default behavior.

Details can be found here:

 http://wiki.sugarlabs.org/go/Features/Enhanced_color_selector

Three components are impacted:

sugar
sugar-toolkit
sugar-artwork

The associated patches are included.

[tomeu]

+ [self.strole, self.fill] = self.get_random_color()

Typo

+ # save an index to our color in the list

Superfluous comment, see  http://wiki.sugarlabs.org/go/Development_Team/Code_guidelines about comments.

+ self.n = self.find_index()

Better use descriptive identifiers, here I would use self._current_color or self._current_color_index, note the leading underscore because it's a private member.

+ logging.debug('Color string is not valid: %s, '
+ 'fallback to default', color_string)

Are we really falling back to a default? Better logging.error instead of logging.debug or even better, raise a ValueError exception.

+ [self.stroke,self.fill] = _parse_string(color_string)

Watch out the space after ',', see PEP-08 about whitespace.

+ def get_random_color(self):

Why is this an instance method? Shouldn't be a function in the module scope as it has nothing to do with any particular XoColor instance?

+ my_n = int(random.random() * (len(colors) - 1))

Every time you create a new identifier, you have a chance to make your code more readable by choosing descriptive names. I would use color_index, random_color, etc.

+ [my_stroke, my_fill] = colors[my_n]

stroke, fill = colors[color_index] should be enough

+ my_n = self.n
+ my_n += 1

Suggest doing instead next_index = self._current_color + 1

+ def get_prev_color(self):

Try to avoid abbreviations.

+ def find_index(self):

If this method is not used outside its class, it should be private (see PEP-08 about scopes).

+ for c in range(0,len(colors)):

Maybe s/c/color?

+ # if the color is not found, then return 0

Not necessary

[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.

[walter]

Updated patches for xocolor.py and view.py based on Tomeu's feedback. I haven't done anything to the hippocanvas code yet, as I am not quite sure how to set things up so that I can reuse the view.py code there.

[walter]

I am not convinced that making the change in the initialization screen is worthwhile. In fact I would argue we should just randomly assign a color dyad upon initialization and let the user change it using the Control Panel. In any case, it would be nice to get some feedback on the Control Panel patch independently of any other changes.

[walter]

As per the discussion at the design meeting ( http://wiki.sugarlabs.org/go/Design_Team/0.88_Meeting#Enhanced_Color_Selector), I've updated xocolor to added next/prev fill/stroke color selection (patch attached) and the control panel about me color selector (patch attached). Once these patches are reviewed, I will update the initialization screens as well. (Note that I have not yet rearranged the buttons in a + pattern. They are still in a row: prev stroke; prev fill; XO; next fill; next stroke. I did eliminate "random". Clicking on the central XO does nothing.

[walter]

new as of 24-01-10

[walter]

new as of 24-01-10

[walter]

had fill and stroke swapped

[alsroot]

I just tried to remove circular dependencies

[erikos]

Thanks Walter and Aleksey for your work on this one!

Aleksey, can you have a look and find out how to position the icons in a plus? I agree that it is confusing the current way. Your idea of only making the changing color "visible" sounds interesting, too. Maybe that can be tried as well and we should make then a screenshot of both options and mail to the design team as well.

[alsroot]

last two patches implement  http://wiki.sugarlabs.org/go/Design_Team/0.88_Meeting#Enhanced_Color_Selector

and support two axis changing i.e. while changing stroke color, fill order range will no be changed

[erikos]

Selector where only the color that changes is displayed

[erikos]

Selector where fill and stroke color is displayed

[erikos]

Thanks Aleksey, this looks already great. I have made a screenshot with the two possible versions.

I showed both versions to my wife. She was confused by the version where only one color was displayed for the "XO-buttons". She thought one color would be white then. And it is hard to say which of the icons shows the fill and which one the stroke color.

The second patch was already better. But still not fully satisfying in the sense that the navigation was too complex.

She offered a grid showing all the XOS with the available colors.

[tomeu]

I'm a bit confused, do we agree that the user experience in the latest patch is better than what we have now and should go into 0.88? Or are discussions still going?

[alsroot]

I guess discussions still going since "arrows" scheme is really confusing

 http://wiki.sugarlabs.org/go/Design_Team/0.88_Meeting#Enhanced_Color_Selector

[walter]

A new patch based on Alsroot's just-another-patch ( http://dev.sugarlabs.org/attachment/ticket/1592/just-another-view.patch). This one uses Christian's design from Feb 6, with the exception that the gray box surrounding the central icon is missing.

I made these changes to Aleksey's patch:

1) all icons are the same size, centered on the page.
2) the click to change colors text is in the line above the icons.
3) the name field is now centered on the page below the color icons.
4) the name label has been removed.
5) disabled the changed alert for clicks on the center icon.

[walter]

Feb 8 patch acording to Christian's Feb 6 design

[walter]

Christian's Feb 6 design

[walter]

Well, I still don't know how to do a box, but I do know how to do separators, included in the new patch: new-view-separators.patch. I am quite pleased with the way it looks.

[alsroot]

Replying to walter:

I can't apply new-view-separators.patch, could you update your local repo and post new patch.

[walter]

diff against latest version in git

[walter]

I cloned a fresh copy and generated a fresh patch...

[alsroot]

I'm ok w/ sugar code from coding POV
but imho for me one row scheme is looking like a puzzle game where the final goal is guessing the rule how colors are being changed :)

[walter]

Well, it is simple, it has the necessary functionality, and if discovering the rules is part of the process, that may not be a bad thing. The goal is to give the kids something systemic to explore the space while keeping things to a minimum... IMHO, we should move on... (I should get around to writing the Python version of XOMan and from there, really let the kids start exploring color.

[tomeu]

Moving to 0.90, if there's serious interest from deployments to have this in 0.88 and no big concerns about destabilization, please move it to the milestone 0.88.x

[tomeu]

Can someone please state which are the patches up for review?

[walter]

The patches are all included as attachments to this ticket. If it would help, I could regenerate them relative to the latest version in git, since they are all quite old at this point.

[tomeu]

Replying to walter:

The patches are all included as attachments to this ticket. If it would help, I could regenerate them relative to the latest version in git, since they are all quite old at this point.

That would certainly help, but also need to know which are the patches that need review because there are now 10 patches but I don't think all of them are being proposed.

[walter]

latest version of cpsection/aboutme/view.py

[walter]

and the latest version of xocolor.py for review

[tomeu]

As commented in #sugar, please make sure the code matches the present logic and remove the signal disconnects as all those widgets get released at the same time. Also would be good to have the whitespace fixes in a separate patch.

[erikos]

Like discussed on irc, we need to do the following for the toolkit part:

- The logic about the next_color* calls should be put inside the module. As they are based on the color index of the module, I think the module level makes more sense. The problem is, that you can generate a color that is not in the index. We have to handle the cases where someone calls the method with a color that is not in the index:

<erikos> >>> a = xocolor.XoColor('#000000,#FFFFFF')
<erikos> >>> a.get_next_color()
<erikos> '#FF2B34,#B20008'
<erikos> >>> a.get_prev_color()
<erikos> '#BCCDFF,#AC32FF'
<erikos> >>> a.get_next_fill_color()
<erikos> walterbender: that call never returns

So, I think they should be on the module level, like get_random_color, if we want to expose them.
And if we add API we should add comments, it would be great if you could add a comment to the public methods what they do (I will cleanup the rest of the module after we landed this feature).

[walter]

cleaned up version as per feedback

[walter]

cleaned up version as per feedback

[erikos]

Walter, I prefer the new approach of the patch, looks much better now. However, something is still left to do...

>>> from sugar.graphics import xocolor
>>> c = xocolor.get_radom_color()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'get_radom_color'
>>> c = xocolor.get_random_color()
>>> c
'#FF8F00,#AC32FF'
>>> c.get_next_stroke_color()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'str' object has no attribute 'get_next_stroke_color'
>>> get_next_stroke_color(c)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'get_next_stroke_color' is not defined
>>> xocolor.get_next_stroke_color(c)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/erikos/sugar-jhbuild-master/install/lib/python2.6/site-packages/sugar/graphics/xocolor.py", line 242, in get_next_stroke_color
    current_index = _get_index(color)
  File "/home/erikos/sugar-jhbuild-master/install/lib/python2.6/site-packages/sugar/graphics/xocolor.py", line 311, in _get_index
    if colors[index] == [color.stroke, color.fill]:
AttributeError: 'str' object has no attribute 'stroke'

[walter]

Not sure what the bug is here. get_random_color returns a string, not an xocolor object, so you cannot use it with methods that want an xocolor as an argument. You can convert a string to an xocolor using the factory method as per below:

import xocolor
c = xocolor.get_random_color()
print c

#FFC169,#F8E800

type(c)

<type 'str'>

c2 = xocolor.XoColor(c)
c3 = xocolor.get_next_fill_color(c2)
print c3

#FFC169,#00EA11

Are you suggesting that the get methods take strings as arguments instead of XoColors? Or I could test for the argument type and do the "right" thing depending upon the argument type. But what is the use case?

[walter]

Another approach would be to have all of the utility methods return xocolor objects instead of strings. This would require an API change for get_random_color that would need to be propagated to the initialization dialog and perhaps other places as well. Somewhat outside of the context of this ticket, but I am willing to make the change if people think it will make the code more robust/easier to maintain.

[walter]

As per a discussion with Erikos and Tomeu on irc, I moved the xocolor modifications to view.py. The patch is now restricted to the one file. Please refer to  http://bugs.sugarlabs.org/attachment/ticket/1592/0001-moved-_get_next-prev-methods-from-xocolor.patch

[walter]

cleaned up patch

[tomeu]

+def _get_prev_stroke_color(color):

Is it really necesary to abbreviate previous?

+ """ Return the prev color pair in the list that shares the same fill

Any reasons for using abbreviations in docstrings?

+ for index in range(0, len(colors)):
+ if colors[index] == [color.stroke, color.fill]:
+ return index

Use list.index()?

+ self._handlers = []

self._handlers is not used, please drop it.

+ for p in sorted(self._pickers.keys()):

s/p/picker_index?

[walter]

latest version of cpsection/aboutme/view.py

[walter]

 http://bugs.sugarlabs.org/attachment/ticket/1592/0001-enhanced-color-selector-with-code-clean-as-per-sugge.patch has all the changes suggested by Tomeu above except that I left in self._handlers as it is used.

[tomeu]

self._handlers = []

self._handlers is not used, please drop it.

Please push with this change and with an appropriate commit message.

[walter]

Enhanced color selector adds previous/next stoke and fill color selection; removes random

[walter]

 http://bugs.sugarlabs.org/attachment/ticket/1592/0001-added-enhanced-color-selector-cycle-through-previous.patch

Should be all set. (Erikos: Can you please commit this patch as I don't have commit privileges on mainline?)

[walter]

 http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/6eeb99a3c9cbde8611dc167ce95dc79bd5c767a0

Final commit.