fix: removed annotation should not appear in sniper mode

[algomaster99]

Duplicate of #4282

Fixes #4218

Opening this PR again in the hope that the tests will pass. Some weird issue came in the linked PR.

[algomaster99]

@slarse Any idea what certificate are the workflows talking about?

[slarse]

@algomaster99 Looks like someone forgot to renew a domain name, restarting the workflow.

[algomaster99]

You mean this domain - spoon.gforge.inria.fr ?

[algomaster99]

@monperrus ping for reviews.

[slarse]

This doesn't seem right to me. The list is cleared on the next line, but the listener is not called now.

[algomaster99]

I tested this. If you delete any number of annotations, it registers what annotations need to be added (the ones which are left after deletion) in the for-loop which follows.

So while pretty-printing it, only those are printed which are registered in the model. And they are exactly the ones we need.

[slarse]

I tested this. If you delete any number of annotations, it registers what annotations need to be added (the ones which are left after deletion) in the for-loop which follows.

Well, the problem I see is that if you set a non-empty list of annotations, then the change listener is only informed of additions. I'm not certain of precisely how this plays with the sniper printer, but this is strictly incorrect. Don't think of this in terms of the sniper printer alone (the change listener interface knows nothing of the sniper printer), but in a general sense: If the list is cleared, onListDeleteAll must be called, because all elements were deleted.

[algomaster99]

Don't think of this in terms of the sniper printer alone (the change listener interface knows nothing of the sniper printer),

Okay, I get what you are trying to say, and this makes sense. The listener could be used for something else in future, and "If the list is cleared, onListDeleteAll must be called" might be necessary there. I think this was just a fortuitous event in the case of sniper printer.

I will revert the deletion.

[algomaster99]

@monperrus @slarse I think this is ready for merge.

Just as a note - changes like 1bdf92b would also be needed in methods like setTypeMembers.

[algomaster99]

@monperrus The order of registering an event listener and performing the action doesn't matter for ChangeCollector because it just ignores the oldValue. It is just concerned with what roles has been changed. Example,

getFactory().getEnvironment().getModelChangeListener().onListDeleteAll(this, CtRole.ANNOTATION, this.annotations, new ArrayList<>(this.annotations));
this.annotations.clear();

and

this.annotations.clear();
getFactory().getEnvironment().getModelChangeListener().onListDeleteAll(this, CtRole.ANNOTATION, this.annotations, new ArrayList<>(this.annotations));

are the same thing for registering a change in a ChangeCollector.

Therefore, it is hard to assert the difference in change collector before and after the transformation because both of the above cases will have the same effect.

Maybe, we could try writing a test case which invokes ActionBasedChangeListener.onListDeleteAll instead of ChangeCollector.onListDeleteAll. I will try to look at coverage reports and figure out where ActionBasedChangeListener is used.

[algomaster99]

@monperrus FineModelChangeListener is implemented by only three classes:

1. ActionBasedChangeListenerImpl.java: Only this class uses the field and oldValue parameters, but it's only being used in a test.

2. ChangeCollector.java: Ignores the value of field and oldValue.

3. EmptyModelChangeListener.java: Does not do anything.

I say, that we skip writing test for now because it will only improve the robustness of ActionBasedChangeListenerImpl.java which is just used once in spoon and that too in tests. Not sure if it is used in other repositories, but for their sake, we could open a separate issue to write tests for ActionBasedChangeListener.

For this PR, let's move forward with merging it. I have followed the contract of the listener and made changes in accordance with that.

[monperrus]

Thanks a lot @algomaster99 for the fix and @slarse for spotting the unspecified behavior.

Per our discussion today, @algomaster99 will open an issue and a PR later on properly specifying and testing onDelete methods