Refactoring XYZSeriesStyle class

[anarak]

Any suggestions what to do with this class for refactoring which contains only swt and nebula libraries?

[jayjaybillings]

We can't refactor XYZSeriesStyle, but let me look at the class that uses it (assuming ISeries) real quick.

[SmithRWORNL]

For my two cents, I think the styles are poorly categorized. Other than AbstractSeriesStyle, all the individual styles are completely tied to the xygraph implementation for the CSV service.

My vote would be to put ISeriesStyle and AbstractSeriesStyle in o.e.e.viz.service.styles and for all other styles to be moved to the new CSV service bundle, possibly with new names that better reflect their use (ie CSVAxisStyle or XYGraphAxisStyle instead of the ambiguous XYZAxisStyle.)

[jayjaybillings]

Okay, we don't need to rewrite it, but it should be moved. Whatever bundle you are moving the RCP stuff to should get the XYZ*Style.java files. Basically, what @SmithRWORNL said, but put it in an RCP-specific bundle or bundle fragment.

I'm thinking about the best way to update CSVSeries. Gimme a minute.

[jayjaybillings]

So, it looks to me like line 93 in the main constructor is not necessary:

    protected CSVSeries(EventList<Double> source, Color color) {
        super(source);
        style = new XYZSeriesStyle(color); // Why call this when we have ISeries.setStyle(IStyle)?
        time = 0;
        parent = null;
        label = "unlabeled series";
    }

As I note in my highlight, why create a concrete style when creating CSVSeries when ISeries has a setStyle() operation that CSVSeries implements? I think we should delete that line and update EAVP, if necessary, to always call setStyle() instead of relying on a hard-coded, concretely-typed, encapsulation-violating default.

[anarak]

@SmithRWORNL did you mean to create a new bundle o.e.e.viz.service.styles and move ISeriesStyle and AbstractSeriesStyle there? How about instead leaving them where they are now, but move all other CSV specific realizations to the style package within already agreed to be created bundle o.e.e.viz.service.scv? I think it is better to leave them where they are now because the *.service bundle is meant to contain abstract code.

Also, it seems that only CSV service uses SeriesStyle realizations, because of that I'd just omit XYZ from the names of the classes to make AxisStyle, etc. Or, CSVAxisStyle if needs to be.

[jayjaybillings]

We need the XYZSeriesStyle classes to maintain the XYZ part of the name. This is a normal class naming convention to expose the implementation in the name for concrete classes.

@anarak is correct that the interfaces and abstract classes should stay in the current bundle. Here's the structure I want:

• org.eclipse.eavp.viz.service - Holds abstract base classes, interfaces, and the most basic common implementations. This should also hold the handler interfaces we discussed.
• org.eclipse.eavp.viz.service.csv - Holds implementations of the services interfaces specifically for CSV files. These are classes that will not depend on the exact UI toolkit implementation with a little refactoring. For example, CSVPlot.
• org.eclipse.eavp.viz.service.csv.rcp - Holds RCP specific implementation classes like XYZSeriesStyle or SWTCSVPlotHandler.

[SmithRWORNL]

@anarak o.e.e.viz.service.styles already exists as a package inside of o.e.e.viz.service, where it should stay, containing the interface and abstract class.

I believe everything but ISeriesStyle and AbstractSeriesStyle are not just CSV specific, but also RCP specific thanks to being implemented specifically for XYGraph, and so would be best placed in o.e.e.viz.service.csv.rcp.

Names like CSVAxisStyle would be best, as we may reuse the ISeriesStyles in the future, such as for the SWTChart service.

[anarak]

@jayjaybillings removing line 93 from constructor might help in CSVSeries, but we will need to set the style later anyway, in CSVPlot for example, which we are trying to dissect too.

[anarak]

@SmithRWORNL I was confused by your wording, you said to

"put ISeriesStyle and AbstractSeriesStyle in o.e.e.viz.service.styles"

they are already there. Now I understand what you meant.

[jayjaybillings]

@anarak Yes, that is correct. We will probably have to inject this on the handler. So, for example, our IPlotHandler interface would have an operation like getStyle() or, if we wanted to create multiple styles dynamically, getStyleFactory() and our SWTCSVPlotHandler would return XYZSeriesStyle instances or an appropriate factory.

[SmithRWORNL]

The old CSVVizService will be replaced in 0.3