Updated composite panel

[barnettwilliam]

Rebase of PR #134 to incorporate the recent platform changes.

[barnettwilliam]

The core composite panel code has been tested and is functioning in 39375bb. Using conversion function function-flexmi2plantuml wasn't possible so it was invoked it as an action function via an ActionButtion for testing by modifying the epsilon tool; issue #152 raised to address this.

[szschaler]

Quite a large change. Not everything seems related to the core change proposed?

I note further opportunities for refactoring. I am open to deferring some of that refactoring to a later issue, but would like to understand whether it's feasible to do this efficiently now...

[szschaler]

Thanks, @arturboronat and @barnettwilliam for discussing these comments so extensively. What's the most efficient way of splitting the work? (I guess, really I'm asking @arturboronat whether you have any capacity of lending a hand with some of these changes?)

[arturboronat]

It's probably easier if I refactor findPanel and resolveRef. Anything else other than that?

[szschaler]

Thanks, @arturboronat, that would be extremely helpful!

[arturboronat]

Thank you both for working on this pull request.

I merged the changes to my codebase by there is not much I can do :-)

The proposed refactoring won't work because these functions (findPanel and resolveRef) are used to perform data bindings (converting JSON objects into class instances). These are scripting functions processing JSON/YAML objects and we can't benefit from OOP features like inheritance until we have class instances.

On the other hand, the activity configuration used in the YAMTL playground fails to pass the validator. See the errors:

The OCL activity under platformtools does not have composite panels (it's three months old) and I don't know where the example above can be found.

@barnettwilliam Could you test the validator with the YAMTL activity https://yamtl.github.io/playground-activities/yamtl-demo-activity.yml ?

@szschaler Do you see any other way of performing some meaningful refactoring?

[szschaler]

@arturboronat ah yes. Looking at this again, I realise my misunderstanding. But that really only means we need to refactor more consistently: it should be the initialisation function of the panel that gets passed in the JSON sub-structure providing the details of this panel and then using that to setup the panel, possibly recursively so.

Once we have this, all the rest can happen over Panel objects and, thus, can be managed through methods within Panel that are overwritten in its sub-classes if required.

[barnettwilliam]

@arturboronat yes I'll test with your example, it's likely the validation rules need to be updated. I'm surprised the example I tested didn't pick that up. I'll also update the ocl example in mdenet/educationplatform-examples.

[arturboronat]

@szschaler The initialize() function is already defined/overriden in each Panel class and the idea of passing a JSON document as parameter would help refactor the switch statement mentioned above but it is not specific to CompositePanel. The search for the relevant JSON document when looking for panel details seems to belong to ActivityManager though.

[barnettwilliam]

@arturboronat do you have a version of your activity files available for me to test with? they don't appear to be available in the YAMTL repositories. A change I made was to rename childrenPanels to childPanels so the activity needs to be updated to reflect this.

Update on this - I tested a local copy of yamtl-demo-activity with test file stubs and childPanel naming and the activity validates.

[arturboronat]

@barnettwilliam The activity is https://yamtl.github.io/playground-activities/yamtl-demo-activity.yml

In that file the file: value for model transformation, metamodels and models can be obtained by prepending the prefix: https://yamtl.github.io/playground-activities/. For example, when the value is CD2DB.groovy it should be updated to https://yamtl.github.io/playground-activities/CD2DB.groovy

[barnettwilliam]

@barnettwilliam The activity is https://yamtl.github.io/playground-activities/yamtl-demo-activity.yml

In that file the file: value for model transformation, metamodels and models can be obtained by prepending the prefix: https://yamtl.github.io/playground-activities/. For example, when the value is CD2DB.groovy it should be updated to https://yamtl.github.io/playground-activities/CD2DB.groovy

Thanks Artur, when using the activity prefix suggested and childPanels naming the activity validates functions correctly.

[arturboronat]

Fab, I'll try this later today.

[barnettwilliam]

@arturboronat ah yes. Looking at this again, I realise my misunderstanding. But that really only means we need to refactor more consistently: it should be the initialisation function of the panel that gets passed in the JSON sub-structure providing the details of this panel and then using that to setup the panel, possibly recursively so.

Once we have this, all the rest can happen over Panel objects and, thus, can be managed through methods within Panel that are overwritten in its sub-classes if required.

@szschaler does this mean that the ActivityManager will then take on the responsibility of creating Panel instances? the approach I have been taking is to keep ActivityManager and ToolsManager handling config file objects that are free of any UI code. Then, have the Playground.js construct the platform level objects like Panel. And if this is the case, do we then favour instantiating objects from configs earlier and mind Panel objects with UI information being included in the ActivityManager etc.?

[barnettwilliam]

Updated documentation can be found mdenet/educationplatform.wiki@9bb39b88d4c230a6db1653f3e1c1af9837d7ab39 branch feature/composite-panel

[szschaler]

Happy except for the implementation of save()

[szschaler]

Thanks for these comments about the proposed refactoring. It looks like this is significantly more complex than I thought and, therefore, really doesn't belong into this PR. I have resolved the corresponding conversations and am happy for this to be merged, subject to a small fix to the implementation of save{}.