Allow pointStyle to be a function drawing custom shape

[uzlov]

Custom point style as function.
Example https://jsfiddle.net/e8n4xd4z/19461/

[simonbrunel]

The style/pointStyle option is going to be scriptable for all charts (currently, only the bubble chart implement scriptable options). That means style can't be a function that draws the point.

[benmccann]

@uzlov I'm going to close this PR for now, but please feel free to reopen it if you're able to update the code to make it scriptable like the bubble chart

[uzlov]

If I'm understanding correct, my code is not opposite with scriptable options. Even more, it's allow to have more custom functionality. We can return in scriptable option "pointStyle" pointer to function for drawing the point. My code just allow to use this custom function.
Mb I wrong, please point me.

[benmccann]

There was a PR submitted to make the pointStyle option scriptable (#5973) which will allow you to accomplish everything this PR does

[uzlov]

Scriptable option "pointStyle" should return string or image (#5973). But, my commit is allow to return function instead of string (or image). And, after, we can use this function for draw point in custom style.
We can use my MR after #5973. And code is not opposite to scriptable options, but extending it.
Please, check example. We have hardcoded list of point styles in

Chart.js/src/helpers/helpers.canvas.js

Line 66 in 740e087

drawPoint: function(ctx, style, radius, x, y, rotation) {

[benmccann]

Scriptable options also accept a function: https://www.chartjs.org/docs/latest/general/options.html#scriptable-options

Are you saying you want the option to be a function that returns a function that then returns a string or image? What would the point of that be?

[uzlov]

No no.
For now - we have a scriptable option (function), which is returning string or image, which we use in drawPoint. My code is allow to use custom function for drawing (not 'circle', 'rect' etc)
Function return a function, which is drawing on canvas. We have all what we need in drawPoint

1f62268#diff-5a99d0ca9a04b73627a387ff34c96da2R52

Instead of execution of lines 82-172 (in function drawPoint), we execute our function, that's all. Our function can to different for each point and related from data etc (thx to scriptable options from #5973)

[benmccann]

Ok. @etimberg @simonbrunel what do you think? You know the scriptable stuff best

[benmccann]

@uzlov you would need to update the documentation as well

[uzlov]

Ok, thx. I'll update docs.

[simonbrunel]

@uzlov scriptable options doesn't support "function" as regular value. A function returning a function for every point elements sounds really wrong to me but more importantly, it produces a poor and confusing API since users will have to wrap the "drawing" function inside the "scriptable" function:

style: function(context) {
    // This function is called 1000 times if there are 1000 points
    // `context` is the option context (not the canvas context)
    return function(ctx) {
        ctx.draw();
    };
}

// highly confusing with:
// NOT SUPPORTED BY SCRIPTABLE OPTIONS
style: function(ctx) {
   ctx.draw();
}

Actually, I'm not even sure we should support that "feature" as option. Why not overriding the drawPoint method instead?

[etimberg]

I agree that the API will be confusing. If we do decide to support function properties like this I think only the first option makes sense from an API design perspective. I would prefer to explore other options first.

Perhaps there is an easy way to register new point styles and drawing functions?

[simonbrunel]

If we do decide to support function properties like this I think only the first option makes sense from an API design perspective.

Mixing scriptable option with regular function (my first snippet) shouldn't be a solution to consider (it's not about the function signature, could be a single parameter, it would be the same issue).

Perhaps there is an easy way to register new point styles and drawing functions?

There is nothing well designed for that, though I'm not sure this is a common use case so would not try to add an extra API since it can easily be done by overriding the drawPoint helper:

var overridden = helpers.canvas.drawPoint;
helpers.canvas.drawPoint = function(ctx, style, radius, x, y, rotation) {
    if (style === 'foobar') {
        ctx.moveTo();
        // ...
    } else {
        overridden.apply(this, arguments);
    }
}

[uzlov]

ok, I see
thx for your answers @simonbrunel @etimberg

[etimberg]

@simonbrunel that seems like a fine to solution to me for this.

[benmccann]

I will change the state of this PR back to closed for now given that overriding drawPoint is probably the better way to go