sugarlabs / physics

a box2d playpen
GNU General Public License v3.0
7 stars 26 forks source link

Add Color-Picking Palette #23

Closed Kachachan closed 7 years ago

Kachachan commented 7 years ago

1) Added a Color-Picking Palette 2) Added Random Color Button next to it

Curious things to note: 1) On one of my laptop, the two buttons above have same size icons., On another laptop, they somehow aren't. Don't know how it looks like to everyone else. 2) I'll get around to updating the random button art after I get my stylus back -- I understand that it is slightly lacking at the moment, but I don't have any means to make any changes for the next few day :P

quozl commented 7 years ago

Thanks, very interesting.

Tested on Ubuntu 16.04. It works as you describe. Yes, the icons are not the same size; they will need to be resized according to Sugar guidelines.

It is not shown when the colour choice is random. Perhaps you might use a ToggleToolButton for the second button, and turn off the toggle when the first button is used.

White is a very difficult colour to use! The object cannot be seen against the white background. Perhaps white should not be used, or the background should be changeable as well.

There are several indentation and blank line changes in d1d12ee, but I won't go into detail on those until we've consensus on behaviour and appearance. Review the commit in detail to see what I mean. I'm able to strip those out myself before merging.

Kachachan commented 7 years ago

Very weird. In the SVG files their dimensions are both specified as 55 * 55. Seems like the icon is made smaller because it's part of ColorToolButton, not the normal buttons like everything else. Any idea on how I might fix this?

Kachachan commented 7 years ago

Re: white, I think I'll change it a different color - very light gray perhaps?

Re: ToggleToolButton, you mean to highlight the random button when that's the latest choice and to unhighlight it when the user picks a color? Hope I'm understanding you correctly :P

quozl commented 7 years ago

Re: icon dimensions, no, sorry, I don't know. I guess if you compare them with other icons you may find the reason for the behaviour.

Re: white. Something other than white would fix it.

Re: ToggleToolButton, yes, make the random button active when it is clicked, and have it pop back to inactive when the other button is used to pick a colour.

walterbender commented 7 years ago

Actually, regarding white, I think you should keep it. But change the default background color to a light gray (say F0F0F0). This will make white objects jump out a bit.

On Wed, Jan 4, 2017 at 11:35 PM, James Cameron notifications@github.com wrote:

Re: icon dimensions, no, sorry, I don't know. I guess if you compare them with other icons you may find the reason for the behaviour.

Re: white. Something other than white would fix it.

Re: ToggleToolButton, yes, make the random button active when it is clicked, and have it pop back to inactive when the other button is used to pick a colour.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sugarlabs/physics/pull/23#issuecomment-270562166, or mute the thread https://github.com/notifications/unsubscribe-auth/ADz74fOq7h6qXBYDgBNXdVEb7jjPNFlTks5rPHMUgaJpZM4La_qF .

-- Walter Bender Sugar Labs http://www.sugarlabs.org http://www.sugarlabs.org

Kachachan commented 7 years ago

What I'm trying right now is, under the __init__ of PhysicsActivity inside activity.py:

self.game_canvas = sugargame.canvas.PygameCanvas(self) #this line was always here self.game_canvas.override_background_color(Gtk.StateType.NORMAL, Gdk.RGBA(240, 240, 240, 0.8))

And nothing is happening. Pointers please?

Kachachan commented 7 years ago

Okay, I found it -- I was editing the wrong places.

Please give my latest commits a look. I tried making the bg #f0f0f0 (as @walterbender suggested earlier), which indeed makes the white jump out a bit; but I do feel that it might perhaps be a bit too gray. What do you think?

I also replaced the icon (image courtesy of @walterbender), which does make the actual palette look much more equal to each other, which I think looks fine now.

Lastly, I also changed to a ToggleButton -- the toggle will be activated every time the user clicks on a color in the palette. I've tried bug-testing this as extensively as I can, and I found nothing, but obviously I may have missed something so please help me out here too.

re:blank spaces everywhere in the doc, when I do my work, I try adding things everywhere, so when I take them out (because 95% of them don't work), I may have accidentally left in blank lines :P I've tried to clean that up as extensively as I can, but if I missed some of them then by all means please help me strip them out.

Thank you very much! Em Chotitamnavee.

quozl commented 7 years ago

Thanks. Tested f6c3ba3. Everything fine except a couple of things about the colour button;

screenshot of _physics activity_ screenshot of _physics activity__1

Kachachan commented 7 years ago

Hmm, can you elaborate on the incorrect background of icon part? I don't quite understand what you mean, sorry.

quozl commented 7 years ago

In the screenshot above, in the first row of pixels, almost all the pixels are hex colour #282828, except in the colour button where the pixels are hex colour #808080.

Kachachan commented 7 years ago

Doesn't that only happen when the user opens the color palette? Do you want me to make it so that the color button is never highlighted at all?

quozl commented 7 years ago

It is also happening when the palette is closed, or never used, and also while another palette is open.

Kachachan commented 7 years ago

I can't quite reproduce that... I'll give it a look again tomorrow. Quite late here right now.

Kachachan commented 7 years ago

But being highlighted while the palette is open is fine by you right? Or is that ground for changes as well.

quozl commented 7 years ago

I'm looking for it to be consistent with the other buttons in the toolbar; if there's no reason for the button to be different, it should not be different. All the other buttons on the same toolbar only brighten when the left mouse button is held down; the brightening is removed when the button is released.

The colour button you've added also brightens in the same way; but much brighter, and when the mouse button is released the background of the icon remains brighter. So it stands out, before it is used, and after it is used.

I'm reproducing with Ubuntu 16.04.

Kachachan commented 7 years ago

Also on Ubuntu 16.04. I experience the problem you are referring to now. Looking at sugar paint, it also has the same problem, so it's probably something inherent to sugar3.graphics.colortoolbutton, and not something that has happened because of my own code.

I tried playing around with the code in there, but I see no part of it that is directly related to highlight color. I'm not sure what I need to do here, so can I have some help please?

quozl commented 7 years ago

Thanks. I agree, Paint is also affected. There are comments in _ColorButton class in colorbutton.py that suggest the effect is intentional; see self._preview of _ColorButton.

Please review and comment on the pull request https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/359 where @Hrishi1999 is updating the colorbutton.py documentation. If you're having trouble with how it works, then the documentation should be fixed, and now would be a good time.

@i5o, do you think the background of a ColorButton should match rest of toolbar buttons? I do. See screenshots above in this pull-request, and give it a try yourself.

i5o commented 7 years ago

http://people.sugarlabs.org/ignacio/pi/lhnhOiqocp.log paint is not working; just saying. Will test the patch. But juding by the screenshot it looks like a CSS bug obviously

i5o commented 7 years ago

It works!.

do you think the background of a ColorButton should match rest of toolbar buttons?

I think it should. But how does this affects this patch it self?

I honestly loved the icon changing the fill color :)

quozl commented 7 years ago

do you think the background of a ColorButton should match rest of toolbar buttons? I think it should.

@i5o, thanks. I'd raise a bug for this but http://bugs.sugarlabs.org/ returns 502 Bad Gateway, so I've created issue #24.

how does this affects this patch it self?

@i5o, it means the issue above about the background is no longer part of this patch review.

@kachachan, it tests out fine here, this patch is ready to squash and merge. I'd also like the colour button to change to the random colour that will be used next, so I'll make an issue for that. #25.

Kachachan commented 7 years ago

@quozl, I played around with the self._preview a bit and it seemed to me it mostly dealt with the fg (i.e. what color the outline will be), and nothing I was changing changed the background color. I'll have to ask for you guys to pick this up from me, as I am returning a blank here :P

Per your last comment, I am assuming I need to squash all the commits I have hitherto added into one? Give me a minute for that, then.

EDIT: I THINK I did this correctly, but can you please give it one last look through to make sure it's working fine? I messed up halfway through and had to revert my changes, so I kinda feel uneasy here.

quozl commented 7 years ago

Yes, you did squash correctly. Method of proof: previous final commit was cdce849, new final commit is a62a361, and a diff of those two commits gives empty output.

Reviewed.

quozl commented 7 years ago

I've made further changes;