Opened 8 years ago

Closed 8 years ago

#4065 closed defect (fixed)

Linebreaks, comments in component argument list are mangled after moving it

Reported by: Christoph <buchner@…> Owned by: Martin Sjölund
Priority: high Milestone: 1.9.x
Component: Interactive Environment Version:
Keywords: Cc:

Description

I often align the arguments in a component instantiation with linebreaks (a must if you have many arguments, to avoid running endless lines). Sometimes I add in-line comments to some arguments, or even comment out arguments when testing/changing models.

This can look like this (simple minimum example, see component boundary, but imagine 5+ arguments):

model test_linebreaks
  Modelica.Fluid.Sources.FixedBoundary boundary(use_p = false, //some comment
                                                use_T = false) annotation(Placement(visible = true, transformation(origin = {-54, 42}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  Modelica.Fluid.Sources.Boundary_pT boundary1 annotation(Placement(visible = true, transformation(origin = {-30, -30}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(boundary.ports[1], boundary1.ports[1]) annotation(Line(points = {{-44, 42}, {-44, 9}, {-20, 9}, {-20, -30}}, color = {0, 127, 255}));
  annotation(uses(Modelica(version = "3.2.2")), Icon(graphics = {Polygon(origin = {-13.1502, 17.2813}, points = {{-20.8498, 26.7187}, {75.1502, 6.71867}, {15.1502, 54.7187}, {-74.8498, 22.7187}, {-42.8498, -55.2813}, {-20.8498, 26.7187}})}));
end test_linebreaks;

In current OMEdit, as soon as I move the component in the diagram, (or, it seems, change the position of a connection line attached to it), OMEdit somehow reorders the code, and removes all the linebreaks, and pulls comments out of the argument list, like this:

model test_linebreaks
  Modelica.Fluid.Sources.FixedBoundary boundary(use_p = false, use_T = false) annotation(Placement(visible = true, transformation(origin = {-54, 42}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  //some comment
  Modelica.Fluid.Sources.Boundary_pT boundary1 annotation(Placement(visible = true, transformation(origin = {-30, -30}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(boundary.ports[1], boundary1.ports[1]) annotation(Line(points = {{-44, 42}, {-44, 19}, {-20, 19}, {-20, -30}}, color = {0, 127, 255}));
  annotation(uses(Modelica(version = "3.2.2")), Icon(graphics = {Polygon(origin = {-13.1502, 17.2813}, points = {{-20.8498, 26.7187}, {75.1502, 6.71867}, {15.1502, 54.7187}, {-74.8498, 22.7187}, {-42.8498, -55.2813}, {-20.8498, 26.7187}})}));
end test_linebreaks;

This is not helpful, and on the contrary very frustrating. One has to often either undo the changes with Git (as OMEdit Ctrl-Z/Undo does not even pick up the change), which crashes OMedit if you don't unload the package first, or redo the changes (not fun for 5+ arguments).

Can this behaviour be fixed or circumvented?

Change History (17)

comment:1 by Christoph <buchner@…>, 8 years ago

I just realized that also spaces in the argument list are changed/added for no apparent reason:

  Modelica.Fluid.Sources.FixedBoundary boundary(use_p=false, //some comment
                                                use_T=false)

becomes

  Modelica.Fluid.Sources.FixedBoundary boundary(use_p = false, use_T = false) 

comment:2 by Christoph <buchner@…>, 8 years ago

Is there anyting more you need to triage/diagnose this?

comment:3 by Rüdiger Franke, 8 years ago

Component: *unknown*Interactive Environment
Milestone: Future1.11.0

Release notes of version 1.9.4 list the improvements:

  • Undo/Redo support.
  • Preserving text formatting, including indentation and whitespace. This is especially important for diff/merge with several collaborating developers possibly using several different Modelica tools.

The issue should be fixed until 1.11.0.

comment:4 by Adeel Asghar, 8 years ago

Owner: changed from somebody to Martin Sjölund
Status: newassigned

comment:5 by Martin Sjölund, 8 years ago

Smaller model to test the diff API:

diffModelicaFileListings(
  "model M\n  FixedBoundary boundary(use_p = false, //some comment\n                                                use_T = false) annotation();equation\n  connect(a, bb);\nend M;",
  "model M\n  FixedBoundary boundary(use_p = false, use_T = false) annotation();\n  //some comment\nequation\n  connect(a,b);\nend M;",
  OpenModelica.Scripting.DiffFormat.plain);

Fixing it still would not fix the complaints regarding OMEdit though...

comment:6 by Martin Sjölund, 8 years ago

Fixed in db9157b/OMCompiler for the master branch. I'll see if I can port it to 1.9 as well.

comment:7 by Christoph <buchner@…>, 8 years ago

Thank you! Does it also fix the removal of the linebreaks between arguments (even when no comment is there)?

comment:8 by Christoph <buchner@…>, 8 years ago

..and the added whitespace?

comment:9 by Christoph <buchner@…>, 8 years ago

Test model:

model test_linebreaks2_orig
  Modelica.Fluid.Sources.FixedBoundary boundary(use_p=false,
                                                use_T=false,
                                                p=1e5) annotation(Placement(visible = true, transformation(origin = {-54, 42}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  Modelica.Fluid.Sources.Boundary_pT boundary1 annotation(Placement(visible = true, transformation(origin = {-58, -28}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(boundary.ports[1], boundary1.ports[1]) annotation(Line(points = {{-44, 42}, {-44, 7}, {-48, 7}, {-48, -28}}, color = {0, 127, 255}));
  annotation(uses(Modelica(version = "3.2.2")), Icon(graphics = {Polygon(origin = {-13.1502, 17.2813}, points = {{-20.8498, 26.7187}, {75.1502, 6.71867}, {15.1502, 54.7187}, {-74.8498, 22.7187}, {-42.8498, -55.2813}, {-20.8498, 26.7187}})}));
end test_linebreaks2_orig;

becomes

model test_linebreaks2_orig
  Modelica.Fluid.Sources.FixedBoundary boundary(use_p = false, use_T = false, p = 1e5) annotation(Placement(visible = true, transformation(origin = {0, 62}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  Modelica.Fluid.Sources.Boundary_pT boundary1 annotation(Placement(visible = true, transformation(origin = {-24, -32}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(boundary.ports[1], boundary1.ports[1]) annotation(Line(points = {{10, 62}, {10, 7}, {-14, 7}, {-14, -32}}, color = {0, 127, 255}));
  annotation(uses(Modelica(version = "3.2.2")), Icon(graphics = {Polygon(origin = {-13.1502, 17.2813}, points = {{-20.8498, 26.7187}, {75.1502, 6.71867}, {15.1502, 54.7187}, {-74.8498, 22.7187}, {-42.8498, -55.2813}, {-20.8498, 26.7187}})}));
end test_linebreaks2_orig;

comment:10 by Martin Sjölund, 8 years ago

No, it does not fix this yet. The first commit only fixes the problem with comments moving around. Fixing the moving of whitespace for a component whose annotation does change might be possible to fix as well.

Of course OMEdit could solve test_linebreaks2_orig by doing the diff after changing each annotation. Having the diff do 3 modifications at once is what makes everything tricky...

comment:11 by Martin Sjölund, 8 years ago

A smaller test for the second part:

echo(false);
a1 := "model test_linebreaks2_orig
  Boundary boundary(use_p=false,
                    use_T=false,
                    p=1e5) annotation(Placement(visible = true, transformation(origin = {-54, 42}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(a, b) annotation(Line(points = {{-44, 42}, {-44, 7}, {-48, 7}, {-48, -28}}, color = {0, 127, 255}));
end test_linebreaks2_orig;";
b := "model test_linebreaks2_orig
  Boundary boundary(use_p = false, use_T = false, p = 1e5) annotation(Placement(visible = true, transformation(origin = {0, 62}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation
  connect(a, b) annotation(Line(points = {{10, 62}, {10, 7}, {-14, 7}, {-14, -32}}, color = {0, 127, 255}));
end test_linebreaks2_orig;";


echo(true);
"a1";
diffModelicaFileListings(
  a1,
  b,
  OpenModelica.Scripting.DiffFormat.color);
getErrorString();

comment:12 by Martin Sjölund, 8 years ago

I have a small and simple fix for that small example, but it fails on your original test_linebreaks2_orig... even corrupting the model.

comment:13 by Martin Sjölund, 8 years ago

e0cca96/OMCompiler should handle the second example (also backported to maint/1.9).

comment:14 by Rüdiger Franke, 8 years ago

Please do also have a look at #4110 -- additional newline introduced if changing a model parameter.

Moreover: when creating new models, then everything seems to get arranged without newlines, i.e. long lines with modifiers and annotations. How could this be improved?

comment:15 by Christoph <buchner@…>, 8 years ago

Martin: Tack saa mycket!

rfranke: Personally, I find the endlessly long lines that seem to be prevalent in Modelica pretty annoying, too. To mitigate this, I try to break after every function argument and indent to get a vertical(ish) list with not too much width, and let the accompanying annotation run to the right after the closing parens, so it stays mostly out of the way (see comment:11)

One way to approach this problem would be to formulate/propose some style guide to handle breaking lines between multiple function arguments, see e.g. the way Python does this here - I find that readable and usable. Also, I keep to not using whitespace around keyword argument equal signs:

Yes:

def complex(real, imag=0.0):
    return magic(r=real, i=imag)

No:

def complex(real, imag = 0.0):
    return magic(r = real, i = imag)

Another way, to at least mitigate, would be to color annotations and their arguments in light gray or whatever, since (IMO) annotations are often pretty unimportant in understanding the code, and just add much visual noise, which would be thus reduced.

comment:16 by Rüdiger Franke, 8 years ago

Such guidelines are really missing. Interesting that Python uses no spaces for "modifiers". Common Modelica styling considers the length of a line. Having no spaces for modifiers simplifies automatic linebreaks:

  parameter Real a "my parameter" annotation(Evaluate=true);

vs.

  parameter MyPackage.ManyManyManySubPackages.MoreSubPackages.LongModelName a
    "my parameter with long doc string and a long elaborative description text"
    annotation(Evaluate=true);

comment:17 by Martin Sjölund, 8 years ago

Milestone: 1.11.01.9.x
Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.