SheetChartWrapper.CHART_CREATOR_IMPL should be public

[WoozyG]

Describe your motivation

Since it defines a system property that can be used to specify an overriding implementation of a class, the static field should be public so applications can set the property by reference.

Describe the solution you'd like

Change field access from private to public.

Describe alternatives you've considered

hard-coding the current value in my application.

Additional context

I'll definitely be implementing my own ChartCreator class, as I did in v8. We extend Spreadsheet Charts to replicate nearly all Excel chart properties for consistent look when displaying online and allowing users to download the workbook for offline use. This is how we start that process.

[WoozyG]

Also, there is no documentation around the public setChartCreator(ChartCreator newChartCreator).

It sets a class static, so it really should be synchronized, and documented that the class instance being set will be called from multiple threads, and should be coded accordingly.

For that matter, with the system property, the setter should probably be private - or pick one or the other, not both.

[sissbruecker]

@WoozyG Your're use-case isn't really clear. It seems there are two ways to specify your own chart creator implementation:

• configure the system property spreadsheet.chart.creator.implementation to the name of your custom chart creator class
• or use SheetChartWrapper.setChartCreator to configure an instance of your custom chart creator class

Making the name of the system property configurable doesn't make any sense without further context, why not just use the existing one?

[WoozyG]

I don't want the system property name configurable, I just want it public. Currently, it isn't documented, or public, so we have to dig to find it, then if we use it to specify a class, Vaadin may change the value and break apps because we had to use our own constant/config/static text to reference the property.

Safest is to be able to reference the Vaadin constant, to keep any future naming changes, such as adding "flow" to the name, or anything like that, from breaking an app.

I'd be fine using setChartCreator IF it were synchronized, and we could guarantee our call to it always happened before any call that used the static singleton value.

For that matter, using a static singleton is fraught with potential for thread conflicts, as multiple user sessions read Workbooks with charts, if the implementation accidentally maintains any instance state or uses any classes that do. Historically many Formatter implementations for example had internal state that could be a problem when accessed by parallel threads, and using those without knowing that internal implementation detail would break an app, but only under load.

The same is possible with the current implementation of SheetChartWrapper with a static instance of ChartCreator.

I've handled that in my custom implementation by having my ChartCreator delegate to a new instance of a new custom class for each chart call, and ensuring that class has lightweight constructor behavior.

Hopefully this sheds more light on the issue I was trying to raise, let me know if it doesn't.

[sissbruecker]

Ah sorry, I misread your intention and was assuming the property would be set through the environment or a property file. Your intention is to set it at runtime through Java code. Makes sense then.

[WoozyG]

Yes, setting it in code a runtime allows for compile-time type checking if we decide to rename/replace/move the implementation class. I can't see a case where we'd have multiple implementations and want to externally configure which one is used at runtime via settings, but I suppose someone may come up with a case some day for that, maybe.