Refactor constructors

[joannacirillo]

Context

When creating a new object, no setters are currently called. However, if they have a side effect (which is the case if they have been created through the use of macro.set(...) for instance), we have to call them so we can see those side effects. Therefore, setters need to be called when creating a new instance if they exist.
In particular, with A and B two classes, B being a child class of A and the function setProp1 existing in both classes, only the setter from child class B must be called when creating a new object B .

Results

The setters are called if they exist when instanciating a new object. Changes might be needed to be done in classes that customly handle properties in constructor (extend function) like it is the case of the DataArray class.
For classes having internal properties set in the extend function that don't correspond to properties that can be given in the initialValues, they are now set at the beginning of the defaultValues (in order to be set first).

Changes

In the function newInstance, publicAPI.set(initialValues) is called. The correct setters are thus called when creating a new object, i.e. the one of the child class (defined in parent class or overwritten/created in child class) when one class extend another. Classes need to be created this way:

Parent class constructor example:

function defaultValues(initialValues) {
  return {
    // Internal Objects
    internalProp : [],
    internalVtkObj: macro.obj({}),

    prop1: 1,
    prop3: 70,
    prop5: 0,
    ...initialValues,
  };
}

// ----------------------------------------------------------------------------
// vtkParentClass methods
// ----------------------------------------------------------------------------

export function extend(publicAPI, model, initialValues = {}) {
  Object.assign(initialValues, defaultValues(initialValues));

  // If necessary, custom setting in the model can be done here
  // The values set in the model must be removed from the initialValues then

  // If something like model.prop = something was done here, it can be done in the defaultValues

  // Build VTK API
  macro.obj(publicAPI, model);
  macro.get(publicAPI, model, ['prop1']);

  vtkClassA(publicAPI, model);
}

Child class constructor example:

`function defaultValues(initialValues) {
  return {
    prop1: 111,
    prop2: 2,
    prop4: 9999,
    prop5: 2000,
    ...initialValues,
  };
}
// ----------------------------------------------------------------------------
// vtkChildClass methods
// ----------------------------------------------------------------------------

export function extend(publicAPI, model, initialValues = {}) {
  Object.assign(initialValues, defaultValues(initialValues);
 // values passed to parent are default of the child and initialValues
  vtkClassA.extend(publicAPI, model, initialValues);
  vtkClassB(publicAPI, model);
}`

---

• Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

• semantic-release commit messages
• Run npm run reformat to have correctly formatted code

Testing

• This change adds or fixes unit tests
• Tested environment:
  • vtk.js:
  • OS:
  • Browser:

[joannacirillo]

@jourdain @finetjul @martinken @luciemac This pull request is a proposal to solve the problem mentioned two months ago about the refactoring of constructors. I propose a way to handle that issue on the three highlighted cases as a PoC.

[finetjul]

How about: if (!initialValues.empty && !initialValues.values)

It would support undefined and null case for initialValues.empty

[jourdain]

empty flag was intended at construction time so you can allocate the array later on rather than ahead of time. Basically to ensure the user was aware that it created an empty dataArray.

[finetjul]

looking at the code in master, it seems that an array is ALWAYS provided or created, no ?

[jourdain]

not providing it require an empty flag. But yes, I think we always get something. Meaning that with the empty flag, we still allocate the TypedArray based on size/type...

[finetjul]

fair enough, so let's see if we all agree on the following:

if initialValues = {empty: true, size: undefined|null|0} then model.values = null
if initialValues = {empty: true, size: 10} then model.values = new TypedArray(10)
if initialValues = {empty: undefined|false} then throw error
if initialValues = {empty: undefined|false, data: aTypedArray} then model.values = aTypedArray
if initialValues = {empty: undefined|false, data: aJSArray} then model.values = TypedArray.from(aJSArray)
if setData(null) then model.values = null
if setData(aTypedArray) then model.values = aTypedArray
if setData(aJSArray) then model.values = TypedArray.from(aJSArray)

[joannacirillo]

Currently, if initialValues = {empty: true, size: undefined|null|0} then model.values = new TypedArray(0).

[finetjul]

@jourdain Are you ok with #2409 (comment) ?

[jourdain]

yes, sorry about that

[jourdain]

For if initialValues = {empty: true, size: undefined|null|0}

I will be fine with model.values = null || TypedArray(0)

[joannacirillo]

if initialValues = {empty: true, size: undefined|null|0} then model.values = null
if initialValues = {empty: true, size: 10} then model.values = new TypedArray(10)
if initialValues = {empty: undefined|false} then throw error
if initialValues = {empty: undefined|false, data: aTypedArray} then model.values = aTypedArray
if initialValues = {empty: undefined|false, data: aJSArray} then model.values = TypedArray.from(aJSArray)
if setData(null) then model.values = null
if setData(aTypedArray) then model.values = aTypedArray
if setData(aJSArray) then model.values = TypedArray.from(aJSArray)

It should be implemented this way in last commit.

[finetjul]

why not allow setData(null) ?

[joannacirillo]

The previous implementation would have raised an error in that case. It was not checking the typed array but was doing model.size = typedArray.length, which would have raised an error. It put the values to null but not update the other parameters. Such implementation avoiding an error can also be implemented.

[finetjul]

let's support setData(null)

[joannacirillo]

Then is it that we want that model.values is null or an "empty" array (of size model.size filled with zero) like it is the case in the constructor ?

[joannacirillo]

After some troubleshooting while applying the changes on the DataArray to classes using it, it appeared that it was necessary to handle the field values in extend. While copying the object through the call of vtk(dataArrayToCopy) the object we copy will always have a property called values. In order to ensure that the array can be given by only one mean in newInstance, I chose to call it values and to transpose it to data in the function extend so that setData can be called in the constructor.

[finetjul]

@joannacirillo which bufferShift to keep ?

[MilanPlFn2]

Hello, no news for a while, but where do we stand?

[finetjul]

Hello, no news for a while, but where do we stand?

@MilanPlFn2, it hasn't moved much due to lack of time. Is it something you would like to move forward ?