pradosoft / prado

Prado - Component Framework for PHP
Other
186 stars 71 forks source link

TColorPicker #632

Open Airakath opened 7 years ago

Airakath commented 7 years ago

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

ctrlaltca commented 7 years ago

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 commented 7 years ago

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

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

Airakath commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

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

Airakath commented 7 years ago

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

LCSKJ commented 7 years ago

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 commented 7 years ago

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