Incorrect result with diffModelicaFileListings

[Adeel Asghar]

diffModelicaFileListings produces an incorrect result when addComponent is used. Run the attached script.

[Martin Sjölund]

I think I could make some heuristics for "only additions" and "only deletions", to avoid running the full diff algorithm on this. As long as OMEdit would update the buffer on each operation, this would work well I think.

[Adeel Asghar]

Yeah, OMEdit will update the buffer on each operation.

[Adeel Asghar]

No wait. There will be some operations on which OMEdit don't update the buffer.
For example, deleting 10 components together. If OMEdit update the buffer for each 10 components then it's way too bad. In such cases OMEdit will only update the buffer once.

[Martin Sjölund]

That might still be fine since it is only deletions.

[Martin Sjölund]

I now get:

model M1
  Real m1 annotation(Placement(visible = true, transformation(origin = {-24, -54}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  annotation(Icon(coordinateSystem(preserveAspectRatio=false,
      extent={{-100,-100},{100,100}}), graphics={Line(points={{-60,50},{60,50}}, color={0,0,255}),
      Line(points={{-40,30},{40,30}}, color={0,0,255}),
      Line(points={{-20,10},{20,10}}, color={0,0,255}),
      Line(points={{0,90},{0,50}}, color={0,0,255})}));
end M1;

Which is better, but not quite there. I think the reason that newline+whitespace is removed is because there is none in the pretty-printing. But that could possibly be fixed.

[Martin Sjölund]

Fixed (for only additions) in 7b9a6c9.

[Adeel Asghar]

Yes, it is really only fixed for additions. However, a very normal case is to add and then delete a component (not too complicated just very simple and easy case). This is not working as expected. See the attached script AddDeleteComponent.mos

[Martin Sjölund]

This one does delete+add. You should merge after each operation, re-parsing the contents in order to make the merging go smoother. Although I suspect it will still fail :)

[Adeel Asghar]

Replying to sjoelund.se:

This one does delete+add. You should merge after each operation, re-parsing the contents in order to make the merging go smoother. Although I suspect it will still fail :)

What do you mean by merge here?

[Martin Sjölund]

diffModelicaFileListings does a textual merge of two texts, right? :)

[Adeel Asghar]

Yes.

• So I have actual text in s1
• I did AddComponent and got s2 via listFile
• I did diffModelicaFileListings on s1 and s2 to get contents
• Then I deleted the component and got s2 via listFile
• And then finally did diffModelicaFileListings on contents and s2 which should result back in s1.

[Martin Sjölund]

I must be tired. Yes, it should work.

[Martin Sjölund]

Should be fixed now.

[Adeel Asghar]

I am not really sure if we should address this problem or not. I don't know how to handle coordinateSystem but at least for each new graphical object we should create a new line to make it better readable.

[Martin Sjölund]

Well, you need to make sure that you pass all the options in the same order as they were declared in before. As for the missing newlines, I don't know how we could enforce that since the merging is just a lexer and not a parser.

[Martin Sjölund]

I have a commit that handles delete(token)+whitespace+add(same token). So graphics={ will keep the { on the same line now. Still does not handle how to deal with adding newline in the middle of an array/modification if necessary.

[Martin Sjölund]

Maybe I could try to balance paranthesis and look for added commas on the same level to add a newline for new element if there previously existed newlines after every comma. It does require some matchcontinue statements though. And might be a bit slow.

[Adeel Asghar]

It's not possible to get the order of coordinatesystem attributes. But I still believe that listing graphical objects on newline should be possible and we should do it.

However, if you try the script addClassAnnotation1.mos. You will see that it changes the stuff which we didn't change. I mean we added diagram annotation and the lines of icon annotation are messed up.

[Martin Sjölund]

You do:

addClassAnnotation(... extent={{-100, -100}, {100, 100}}, preserveAspectRatio=false ...);

But it should be:

addClassAnnotation(... preserveAspectRatio=false, extent={{-100, -100}, {100, 100}} ...);

[Martin Sjölund]

Of course it is possible to get the order from OMC. You might need to query it though. Or to modify addClassAnnotation to not change the existing order.

[Adeel Asghar]

Replying to sjoelund.se:

You do:

addClassAnnotation(... extent={{-100, -100}, {100, 100}}, preserveAspectRatio=false ...);

But it should be:

addClassAnnotation(... preserveAspectRatio=false, extent={{-100, -100}, {100, 100}} ...);

According to Modelica specs,

record CoordinateSystem
  Extent extent;
  Boolean preserveAspectRatio=true;
  Real initialScale = 0.1;
  DrawingUnit grid[2];
end CoordinateSystem;

So i always do addClassAnnotation in that order. If I change as you say then it will work for the above case but not when the class has extent defined before preserveAspectRatio.

What we should do is skip default values and that is what Dymola is doing. I can handle this in OMEdit.

And yes we can query the order from OMC but that also depends on CoordinateSystem record.

[Martin Sjölund]

Skipping default values only helps when it is default values that you add. Once you add something else, you get the same issues again.

[Martin Sjölund]

I will fix merging of annotations to not change the order, so you do not need to query as much.

[Martin Sjölund]

... the submods are just inserted as they are. Will take more effort than I thought to fix it. But it should be possible.

[Martin Sjölund]

​https://github.com/OpenModelica/OMCompiler/pull/196

[Martin Sjölund]

Is OK?

[Adeel Asghar]

Replying to sjoelund.se:

Is OK?

Yeah that looks really nice.

[Martin Sjölund]

Doesn't have newlines in the places you want them, but a lot better than before.

[Dietmar Winkler]

Looks like it was forgotten to reschedule.

[Adeel Asghar]

Seems like "\n" is not preserved. See TestDiff.mos.
I loaded BouncingBall.mo from examples added experiment annotation to it and diff produced bad results.

[Martin Sjölund]

Milestone pushed to 1.9.5

[Martin Sjölund]

Milestone renamed

[Martin Sjölund]

Ticket retargeted after milestone closed

[Martin Sjölund]

Milestone moved to 1.12.0 due to 1.11.0 already being released.

[Francesco Casella]

The milestone of this ticket has been reassigned to "Future".

If you think the issue is still valid and relevant for you, please select milestone 1.13.0 for back-end, code generation and run-time issues, or 2.0.0 for front-end issues.

If you are aware that the problem is no longer present, please select the milestone corresponding to the version of OMC you used to check that, and set the status to "worksforme".

In both cases, a short informative comment would be welcome.

[Francesco Casella]

See also test case in #5187:comment:8

[Francesco Casella]

@sjoelund.se, can you please check the test case in ticket:5187#comment:8 and either fix it or leave a comment if you cannot do that in the next few days?

[Martin Sjölund]

I don't think I'll be able to have a look this week or next.

[Francesco Casella]

@sjoelund.se, any chance you can have a look at this before we tag 1.14.0 beta?

Thanks!

[Francesco Casella]

Releasing 1.14.0 which is stable and has many improvements w.r.t. 1.13.2.

This issue, previously marked as blocker for 1.14.0, is rescheduled to 1.15.0

[anonymous]

Is this fixed with https://trac.openmodelica.org/OpenModelica/changeset/ed07d20/OpenModelica ? Will this be backported to 1.14?

Thanks!

[Adeel Asghar]

Almost all the cases reported in this ticket are fixed except that we don't generate graphical objects in newlines instead all of them are in one line. It is more an issue of dumping rather than diff. One can also consider this as an enhancement. I am not sure if doing that will break anything or not.

[Francesco Casella]

We may backport the remaining issue fixes to 1.14.0, but we won't wait for them to be fixed for the release.

[Francesco Casella]

Release 1.15.0 was scrapped, because replaceable support eventually turned out to be more easily implemented in 1.16.0. Hence, all 1.15.0 tickets are rescheduled to 1.16.0

[Francesco Casella]

Retargeted to 1.17.0 after 1.16.0 release

[Francesco Casella]

Rescheduled to 1.18.0

[Francesco Casella]

Ticket retargeted after milestone closed

[Francesco Casella]

1.18.0 blocker tickets moved to 1.19.0