Opened 9 years ago
Last modified 3 years ago
#3424 reopened defect
Incorrect result with diffModelicaFileListings
Reported by: | Adeel Asghar | Owned by: | Martin Sjölund |
---|---|---|---|
Priority: | blocker | Milestone: | 1.19.0 |
Component: | Interactive Environment | Version: | trunk |
Keywords: | Cc: |
Description
diffModelicaFileListings
produces an incorrect result when addComponent
is used. Run the attached script.
Attachments (6)
Change History (54)
by , 9 years ago
Attachment: | AddComponent.mos added |
---|
comment:1 by , 9 years ago
comment:3 by , 9 years ago
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.
comment:5 by , 9 years ago
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.
comment:6 by , 9 years ago
Milestone: | Future → 1.9.3 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Fixed (for only additions) in 7b9a6c9.
by , 9 years ago
Attachment: | AddDeleteComponent.mos added |
---|
comment:7 by , 9 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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
follow-up: 9 comment:8 by , 9 years ago
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 :)
comment:9 by , 9 years ago
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?
comment:11 by , 9 years ago
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.
by , 9 years ago
Attachment: | addClassAnnotation.mos added |
---|
comment:14 by , 9 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
comment:15 by , 9 years ago
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.
comment:16 by , 9 years ago
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.
by , 9 years ago
Attachment: | addClassAnnotation1.mos added |
---|
comment:17 by , 9 years ago
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.
comment:18 by , 9 years ago
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.
follow-up: 21 comment:19 by , 9 years ago
You do:
addClassAnnotation(... extent={{-100, -100}, {100, 100}}, preserveAspectRatio=false ...);
But it should be:
addClassAnnotation(... preserveAspectRatio=false, extent={{-100, -100}, {100, 100}} ...);
comment:20 by , 9 years ago
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.
comment:21 by , 9 years ago
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.
comment:22 by , 9 years ago
Skipping default values only helps when it is default values that you add. Once you add something else, you get the same issues again.
comment:23 by , 9 years ago
I will fix merging of annotations to not change the order, so you do not need to query as much.
comment:24 by , 9 years ago
... the submods are just inserted as they are. Will take more effort than I thought to fix it. But it should be possible.
by , 9 years ago
Attachment: | addannotationdiff.svg added |
---|
comment:28 by , 9 years ago
Doesn't have newlines in the places you want them, but a lot better than before.
by , 9 years ago
Attachment: | TestDiff.mos added |
---|
comment:30 by , 9 years ago
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.
comment:34 by , 8 years ago
Milestone: | 1.11.0 → 1.12.0 |
---|
Milestone moved to 1.12.0 due to 1.11.0 already being released.
comment:35 by , 7 years ago
Milestone: | 1.12.0 → Future |
---|
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.
comment:36 by , 6 years ago
Milestone: | Future → 1.14.0 |
---|---|
Priority: | high → blocker |
See also test case in ticket:5187#comment:8
comment:37 by , 6 years ago
@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?
comment:39 by , 6 years ago
@sjoelund.se, any chance you can have a look at this before we tag 1.14.0 beta?
Thanks!
comment:40 by , 5 years ago
Milestone: | 1.14.0 → 1.15.0 |
---|
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
comment:41 by , 5 years ago
Is this fixed with https://trac.openmodelica.org/OpenModelica/changeset/ed07d20/OpenModelica ? Will this be backported to 1.14?
Thanks!
comment:42 by , 5 years ago
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.
comment:43 by , 5 years ago
We may backport the remaining issue fixes to 1.14.0, but we won't wait for them to be fixed for the release.
comment:44 by , 5 years ago
Milestone: | 1.15.0 → 1.16.0 |
---|
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
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.