eclipse / gemoc-studio-modeldebugging

gemoc-studio-modeldebugging
Eclipse Public License 1.0
6 stars 15 forks source link

[fix] manage many-values in the trace constructor + [fix] bug in trace constructor with dyn obj removal + [fix] allow ecore from plugin in .dsl file #209

Closed Faezeh-Kh closed 2 years ago

Faezeh-Kh commented 2 years ago

Description

The commits are related to two issues in the GenericTraceConstructor: 1) In Ecore, it is possible to define EAttributes with many values (i.e., defining an attribute with upperband = -1). In this case, the type of such EAttribute is a simple data type (e.g., EString) but its value is a list of simple values (e.g., EList). Such cases were not considered in the GenericTraceConstructor and it throws ClassCastException when trying to set or update the values of such EAttriutes. The first two commits are trying to solve this issue.

2) When a model is changed because of removing one of its objects, for the removed object, a new state must be created in the trace by removing the latest values of its dimensions from its previous state. In some cases, the value of a dimension might be null, but the GenericTraceConstructor does not check it. This results in OutOfBoundException because of trying to access an element of an empty list. The third commit checks such a situation and resolves the exception.

ebousse commented 2 years ago

Looks overall OK to me. However, I see that a lot of code is very similar.

Would there be a way to create smart private methods to factorize the code a bit more? If needed using lambdas?

Faezeh-Kh commented 2 years ago

According to the GenericTraceImpl.ecore, the "ManyAttribtueValue" classes do not have a specific common superclass with a common feature for their "value" features. So to create them and to set/get their values, we need to create/use the objects using their specific type. This is the reason why I couldn't find any better way, but you may have other suggestions. image

ebousse commented 2 years ago

I was imagining something like that:


private EObject createValue(eType) {
    switch (etype) {
        case EcorePackage.Literals.EBOOLEAN: return GenerictraceFactory.eINSTANCE.createIntegerAttributeValue();
        case EcorePackage.Literals.EINT: return GenerictraceFactory.eINSTANCE.createIntegerAttributeValue();
        …
    }
}

private EObject createManyValue(eType) {
    switch (etype) {
        case EcorePackage.Literals.EBOOLEAN: return GenerictraceFactory.eINSTANCE.createManyIntegerAttributeValue();
        case EcorePackage.Literals.EINT: return GenerictraceFactory.eINSTANCE.createManyBooleanAttributeValue();
        …
    }
}

private void genericVersion(eType) {
    if (mutableProperty.getUpperBound() == -1) {
        final EObject value = createManyValue(eType)
        if(dynamicProperty.isPresent()) {
            value.getAttributeValue().addAll(dynamicProperty.get().getValue());
        }
        result = value;
    }
    else {
        final IntegerAttributeValue value = createValue(eType)
        if(dynamicProperty.isPresent()) {
            value.setAttributeValue((Integer)dynamicProperty.get().getValue());
        }
        result = value;
    }
}

But getAttributeValue remains annoying here. It would have been better to have in the metamodel a generic ManyAttributeValue EClass as a super type…

ebousse commented 2 years ago

But actually here I'm suggesting a complete refactoring of the whole class. Maybe we can leave that for another PR.

Faezeh-Kh commented 2 years ago

As soon as I had free time, I will try to do the refactoring. Thanks for your hints :)

dvojtise commented 2 years ago

So, let's accept this PR (that fixes some issues) and open a refactoring feature request ;-)

Faezeh-Kh commented 2 years ago

I made a new commit to resolve the error shown in the following figure by considering that an ecore file can also be a platform plugin image

ebousse commented 2 years ago

Ah, good, however it would have been better to make this second fix in a separate branch+PR. The usual policy is "one PR == one fix OR one feature".

@dvojtise Would you prefer to split this PR in two, or is it OK to merge this as is just for this time (which would require renaming the PR btw)?

dvojtise commented 2 years ago

this is ok for this time no need to add more work just change the title of the PR so the changelog will be able to identify both contrib

dvojtise commented 2 years ago

CI job for test verification: https://ci.eclipse.org/gemoc/job/gemoc-studio-integration/job/Faezeh-Kh-PR-209/