Opened 9 years ago

Closed 9 years ago

Last modified 6 years ago

#1592 closed enhancement (fixed)

NEW FEATURE: enhanced color selector

Reported by: walter Owned by: tomeu
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Unspecified
Severity: Unspecified Keywords: r+
Cc: erikos, alsroot Distribution/OS: Unspecified
Bug Status: Assigned

Description

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.

Attachments (21)

0001-color-selector-feature-changes.patch (19.1 KB) - added by walter 9 years ago.
0001-color-selector-feature.patch (2.6 KB) - added by walter 9 years ago.
0001-added-ENTITY-support.patch (2.0 KB) - added by walter 9 years ago.
xocolor.patch (4.9 KB) - added by walter 9 years ago.
new as of 24-01-10
view.patch (16.6 KB) - added by walter 9 years ago.
new as of 24-01-10
xocolor.py (9.5 KB) - added by walter 9 years ago.
had fill and stroke swapped
just-another-view.patch (6.3 KB) - added by alsroot 9 years ago.
two-axis-view.patch (9.2 KB) - added by alsroot 9 years ago.
two-axis-xocolor.patch (1.8 KB) - added by alsroot 9 years ago.
color_selector.png (12.0 KB) - added by erikos 9 years ago.
Selector where only the color that changes is displayed
color_selector_full.png (12.5 KB) - added by erikos 9 years ago.
Selector where fill and stroke color is displayed
new-view.patch (16.9 KB) - added by walter 9 years ago.
Feb 8 patch acording to Christian's Feb 6 design
100206_colorselector_opt1.png (29.2 KB) - added by walter 9 years ago.
Christian's Feb 6 design
new-view-separators.patch (9.0 KB) - added by walter 9 years ago.
diff against latest version in git
0001-new-color-selector-for-cpsection.patch (11.7 KB) - added by walter 9 years ago.
latest version of cpsection/aboutme/view.py
0001-extended-xocolor-for-new-color-selector.patch (4.5 KB) - added by walter 9 years ago.
and the latest version of xocolor.py for review
0001-added-prev-next-fill-and-stroke-color-to-selector.patch (12.0 KB) - added by walter 9 years ago.
cleaned up version as per feedback
0001-added-get_next-and-get_prev-fill-and-stroke-colors.patch (5.7 KB) - added by walter 9 years ago.
cleaned up version as per feedback
0001-moved-_get_next-prev-methods-from-xocolor.patch (14.7 KB) - added by walter 9 years ago.
cleaned up patch
0001-enhanced-color-selector-with-code-clean-as-per-sugge.patch (12.3 KB) - added by walter 9 years ago.
latest version of cpsection/aboutme/view.py
0001-added-enhanced-color-selector-cycle-through-previous.patch (1.5 KB) - added by walter 9 years ago.
Enhanced color selector adds previous/next stoke and fill color selection; removes random

Download all attachments as: .zip

Change History (57)

Changed 9 years ago by walter

Changed 9 years ago by walter

Changed 9 years ago by walter

comment:1 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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

comment:2 Changed 9 years ago 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.

comment:3 Changed 9 years ago by walter

  • Keywords r? added; r! removed

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.

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by 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.

Changed 9 years ago by walter

new as of 24-01-10

Changed 9 years ago by walter

new as of 24-01-10

Changed 9 years ago by walter

had fill and stroke swapped

Changed 9 years ago by alsroot

comment:6 Changed 9 years ago by alsroot

I just tried to remove circular dependencies

comment:7 Changed 9 years ago by erikos

  • Cc erikos alsroot added

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.

Changed 9 years ago by alsroot

Changed 9 years ago by alsroot

comment:8 Changed 9 years ago by 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

Changed 9 years ago by erikos

Selector where only the color that changes is displayed

Changed 9 years ago by erikos

Selector where fill and stroke color is displayed

comment:9 Changed 9 years ago by 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.

comment:10 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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?

comment:11 Changed 9 years ago by alsroot

  • Keywords r! removed

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

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

comment:12 Changed 9 years ago by 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.

Changed 9 years ago by walter

Feb 8 patch acording to Christian's Feb 6 design

Changed 9 years ago by walter

Christian's Feb 6 design

comment:13 Changed 9 years ago by 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.

comment:14 follow-up: Changed 9 years ago by walter

  • Keywords r? added

comment:15 in reply to: ↑ 14 Changed 9 years ago by alsroot

Replying to walter:

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

Changed 9 years ago by walter

diff against latest version in git

comment:16 Changed 9 years ago by walter

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

comment:17 Changed 9 years ago by 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 :)

comment:18 Changed 9 years ago by 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.

comment:19 Changed 9 years ago by tomeu

  • Milestone changed from 0.88 to 0.90

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

comment:20 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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

comment:21 follow-up: Changed 9 years ago by 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.

comment:22 in reply to: ↑ 21 Changed 9 years ago by 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.

Changed 9 years ago by walter

latest version of cpsection/aboutme/view.py

Changed 9 years ago by walter

and the latest version of xocolor.py for review

comment:23 Changed 9 years ago by walter

  • Keywords r? added; r! removed

comment:24 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

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.

comment:25 Changed 9 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned

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

Changed 9 years ago by walter

cleaned up version as per feedback

Changed 9 years ago by walter

cleaned up version as per feedback

comment:26 Changed 9 years ago by erikos

  • Keywords r? added; r! removed

comment:27 Changed 9 years ago by erikos

  • Keywords r! added; r? removed

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'

comment:28 Changed 9 years ago by 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?

comment:29 Changed 9 years ago by 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.

comment:30 Changed 9 years ago by walter

  • Keywords r? added; r! removed

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

Changed 9 years ago by walter

cleaned up patch

comment:31 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

+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?

Changed 9 years ago by walter

latest version of cpsection/aboutme/view.py

comment:32 Changed 9 years ago by walter

  • Keywords r? added; r! removed

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.

comment:33 Changed 9 years ago by tomeu

  • Keywords r+ added; r? removed

self._handlers = []

self._handlers is not used, please drop it.

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

Changed 9 years ago by walter

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

comment:34 Changed 9 years ago by 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?)

comment:35 Changed 9 years ago by walter

  • Resolution set to fixed
  • Status changed from new to closed

comment:36 Changed 6 years ago by dnarvaez

  • Milestone 0.90 deleted

Milestone 0.90 deleted

Note: See TracTickets for help on using tickets.