Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TColorPicker #632

Open
Airakath opened this issue Jan 26, 2017 · 13 comments
Open

TColorPicker #632

Airakath opened this issue Jan 26, 2017 · 13 comments

Comments

@Airakath
Copy link

Hello,
I just noticed a problem with TColorPicker. In full mode, the color gradient range does not match the selected color.

@ctrlaltca
Copy link
Member

Hi! can you please elaborate on what you mean? I've tried the third example of http://pradoframework.net/demos/quickstart/?page=Controls.Samples.TColorPicker.Home , but i can't understand the problem.

@LCSKJ
Copy link
Member

LCSKJ commented Jan 27, 2017

Looks like a bug in the JS of TColorPicker, to reproduce:

  • open third example mentioned by @ctrlaltca
  • use the hue slider and change to a value not multiple of 60
  • use the gradient color picker to change color
  • watch the hue value decrease to a multiple of 60 while mousemoving
  • while the hue changes the gradient stays the same, hence the selected color does not match the gradient color anymore

Having a fast look at the source code, it seems to be a rounding problem with using parseInt().

@Airakath
Copy link
Author

Yes, that's it ! LCSKJ, you explained the problem very well!

I also have a sugestion to do. Also it is possible in a future, that the color can also be selected by writing in hexadecimal textbox and / or rgb

@Airakath
Copy link
Author

I conduct some research. I found a version dating from 2005 of ricoColor.js.

I do not know if this will be of great help, but it is always a beginning

Http://nerdyjs.com/script/134755

@ctrlaltca
Copy link
Member

I've just tried replacing our current implementation with the one linked by @Airakath and guess what? Same problem.
The same problem is also present in prado-3.2, before the jQuery migration.

@Airakath
Copy link
Author

I deepened my research, here is the most recent version that I could find

Version 1.1.2

Https://java.net/projects/faban/sources/git/content/harness/web/scripts/rico.js?rev=9b4487b3abbd297a882044b121dd29cc5030363f

@Airakath
Copy link
Author

I also found a version dating back 3 years ago.

But the version is not indicated

Http://trac.zentraal.com/browser/oakdiocese.openlayers/OpenLayers-2.13/lib/Rico/Color.js?rev=2288

@Airakath
Copy link
Author

Tomorrow I will ask a colleague to work with me more closely. I'll keep you informed tomorrow night if we have found a solution.

@LCSKJ
Copy link
Member

LCSKJ commented Jan 30, 2017

i think the problem is not the 3rd party library, but within Prado.WebUI.TColorPicker itself ... i suspect the poblem comes from the juggling between rgb and hsb in conjunction with putting the rounded values back into the inputs. i wasn't able to take a closer look in the meantime - maybe i find some time tomorrow.

@ctrlaltca
Copy link
Member

I also found a version dating back 3 years ago.

But the version is not indicated

Http://trac.zentraal.com/browser/oakdiocese.openlayers/OpenLayers-2.13/lib/Rico/Color.js?rev=2288

Tested this one, too; it's exactly the same script with 2 small changes:

  • support for picking color from transparent items (if the item is transparent then try to pick color from its parent)
  • support for html color short codes (eg. #fff instead of #ffffff)

I tend to agree with @LCSKJ on the cause of the problem.

@Airakath
Copy link
Author

I will try to orient my search, towards Prado.WebUI.TColorPicker to try to help you.

@LCSKJ
Copy link
Member

LCSKJ commented Jan 31, 2017

OK, after having a closer look I can confirm that the problem comes from switching rgb to hsb and vice versa while putting the value back into the input and read from it again for next calculation. Looks like somewhere between those operations the values are loosing precision since the Rico.Color object needs fractions between 0 and 1 but the inputs are showing degrees (0-360°) or percent (0-100%) for hsb values.

I tried to make a clean fix, but it is not as easy as it seems. I think the whole Prado.WebUI.TColorPicker class needs a rewrite holding the values for calculation internally and only convert them for representation in the inputs. Maybe someone else can have a closer look and finds an easier way.

However, since the problem is the changing hue value even if only saturation and brightness are about to change, I can propose a quick and dirty fix by keeping the hue value unchanged during changeSV() calls introducing a special parameter in the setColor() method:

--- a/framework/Web/Javascripts/source/prado/colorpicker/colorpicker.js
+++ b/framework/Web/Javascripts/source/prado/colorpicker/colorpicker.js
@@ -686,7 +686,7 @@ Prado.WebUI.TColorPicker = jQuery.klass(Prado.WebUI.Control, {

                this.inputs.currentColor.style.backgroundColor = color.asHex();

-               return this.setColor(color);
+               return this.setColor(color, false, true);
        },

        changeH : function(ev)
@@ -754,11 +754,11 @@ Prado.WebUI.TColorPicker = jQuery.klass(Prado.WebUI.Control, {
                }
        },

-       setColor : function(color, update)
+       setColor : function(color, update, nohue)
        {
                var hsb = color.asHSB();

-               this.inputs.H.value = parseInt(hsb.h*360);
+               if (!nohue) this.inputs.H.value = parseInt(hsb.h*360);
                this.inputs.S.value = parseInt(hsb.s*100);
                this.inputs.V.value = parseInt(hsb.b*100);
                this.inputs.R.value = color.rgb.r;

This seems to work quite well, but as said, it's not a very clean solution. The diff is from master either, so if appropriate, the changes need to be applied to current 3.3 branch.

@ctrlaltca Maybe we can even use another 3rd party library for upcoming Prado4 since it seems the Rico.Color library is abandoned ... a composer dependency using a newer and more active bower-asset maybe.

@Airakath
Copy link
Author

Airakath commented Jan 31, 2017

Thanks a lot, that's nice of you. It should help me out right now.
good work !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants