Opened 10 years ago

Closed 8 years ago

Last modified 7 years ago

#2905 closed defect (fixed)

Provide comment-preserving parsing and unparsing

Reported by: casella Owned by: somebody
Priority: critical Milestone:
Component: Frontend Version: trunk
Keywords: Cc: adeas31, sjoelund.se, adrpo

Description

The OMEdit GUI is based on OMC for all editing activities. When OMC passes the text of a class to the GUI, all original formatting information is lost, so that, as soon as the modified class is saved, the original formatting is lost on the file as well.

This makes it very hard to use OMEdit on modelling projects that use revision control (SVN, GIT, etc.) - all serious ones do. This also makes it impossible to use OMEdit to work on the Modelica Standard Library without screwing up the code. Last, but not least, this makes it impossible to use OMEdit on the same project in conjunction with other Modelica tools, which is a very likely user scenario in the near future.

The solution to this problem has already been studied and partially implemented 6 years ago (see: https://www.modelica.org/events/modelica2008/Proceedings/sessions/session6a3.pdf).

It is now of the utmost importance that this solution is implemented in OMC, so that OMEdit becomes usable in the above-mentioned scenarios.

Attachments (1)

omc-diffalgorithm.svg (331.8 KB) - added by sjoelund.se 9 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 10 years ago by ceraolo

A special case is reported here.
Consider the following model

model aa
  Real x;
algorithm
  if time > 0.5 then
  //this is a comment
    x := 2;
  else
    x := 1;
  end if;
end aa;

when saved it becomes:

model aa
  Real x;
algorithm
  if time > 0.5 then
    x := 2;
  else
    x := 1;
  end if;
  //this is a comment
end aa;

This makes it particularly difficult to make clear documentation of code.

Another example is constituted by the block comments: /*...*/.
The text of the rows following the first one is moved right, often by several characters.
I've not checked the details, but after some work it often happens that the commented lines are moved by several tens of characters right, making them hardly visible when scrolling the code.

comment:2 Changed 10 years ago by Christoph <buchner@…>

I have also observed this previously, and it was pretty annoying. And yes, I also use revision control for projects (kind of insane not to, to be honest)

comment:3 Changed 10 years ago by casella

  • Cc adeas31 sjoelund.se adrpo added

I had some discussion with Martin Sjoelund about how to get this done. One possibility, which is referred to in this ticket, is to parse comments and whitespace in the AST, so that it is possible to re-generate the same code when saving the modified model. However, this might be unnecessarily complicated and time-consuming to implement and debug.

Another option to solve this issue is that OMEdit works directly on the source text files, instead of working on their internal representation in OMC after parsing them. The easiest way to implement this is that OMEdit just edits the text file, and then has OMC re-load and re-parse it when the model is saved, checked or simulated. If it is possible to do this without having to re-load the entire root-level package the class being edited belongs to, this might turn out to be fast enough, after the recent improvemens in DLL communication between OMEdit and OMC. Alternatively, it would be possible to keep track of the correspondence between the tokens in the text file and the tokens in the parsed AST, so that it is possible to update the latter after changes to the text file without reloading everything from scratch.

Last, but not least, if OMEdit starts working on the text files directly, it would probably be much easier to eventually build direct support of revision control systems in OMEdit, as all these systems use files as basic storage entities. RCS support is essential for serious industrial use of OMEdit.

I would suggest the developers to discuss these options out of the box - maybe a good solution is easier (and less bug-prone) to implement than we thought.

Last edited 10 years ago by casella (previous) (diff)

comment:4 Changed 10 years ago by sjoelund.se

Well. My idea would be to re-parse after every automatic change by OMEdit (opening the parameters dialog and setting a modifier for example). And comparing the old file to the tokens in the new one and simply performing the changes in the old file to get a minimal diff.

When manually editing a model in OMEdit, the positions in the source file should be noted and exactly those changes be passed to the updated version of the file. No other changes should be incorporated.

Probably, each opened file needs to have one representation in OMEdit (the actual text) and another in omc (the AST).

It is also essential that OMEdit does not change the directory structure when saving files (this happens quite frequently). Because doing this makes it impossible to support any RCS.

comment:5 Changed 10 years ago by sjoelund.se

  • Milestone changed from 1.9.2 to 1.9.3

Milestone changed to 1.9.3 since 1.9.2 was released.

comment:6 Changed 9 years ago by anonymous

There is a special case of reformatting that maybe can be solved with a limited programming effort (while waiting for a more complete solution to this ticket's issue).
Consider the following model:

model CommentBug " Model"
  /* This is a several-line comment
     line two  
     line three
     line four
  */
end CommentBug;

Every time this model is saved in OMEdit lines two, three, four, and the comment ending, are displaced to right by two spaces.
If this comment is part of a large model that is worked with for several weeks, in the end it might get very far from the beginning of line.
Or the user must continually manually reposition all the multi-line comments or, better, totally avoid them.

Tested with r24813.

comment:7 Changed 9 years ago by sjoelund.se

I added a listFile API as a first step: 6293f8f/OMCompiler

Now I just need to decide between using an OMCCp lexer and compare files in OpenModelica, or to use flex.

Changed 9 years ago by sjoelund.se

comment:8 Changed 9 years ago by sjoelund.se

I now locally have a version of Myer's diff algorithm (the greedy one) implemented in MetaModelica. It can produce diffs for sequences of integers or strings (see omc-diffalgorithm.svg).

With an OMCC-based lexer we should also be able to lex two Modelica files to create some sequences of tokens to compare. And then we process the diff (edits) to insert the changes in a way that gives as few changes as possible to the original text-file (despite the OMC unparsing messing up the formatting).

comment:9 Changed 9 years ago by sjoelund.se

The ModelicaDiff lexer and testcase has been merged into the compiler. Missing is an API function for OMEdit to call, which does the work. And a whole bunch of testcases to make sure that everything works correctly. The examples in this ticket have already been added to the test suite and produce an empty diff as expected.

The way the diff works right now is:

  • Lex before and after
  • Compare before and after
  • If a comment has been moved (deleted+added), move it back to its original place
  • Lex before and the partial merge
  • Compare before and partial merge
  • Print the diffs (or final output, depending on what you wanted)

comment:10 Changed 9 years ago by sjoelund.se

c5516d9/OMCompiler includes an API that OMEdit can use to reduce the number of changes done to the source code.

comment:11 Changed 9 years ago by sjoelund.se

  • Milestone changed from 1.9.3 to 1.9.4

Moved to new milestone 1.9.4

comment:12 Changed 9 years ago by adrpo

The following tickets need to be fixed for new OMEdit: #3417, #3460, #3552, #3574

comment:13 Changed 9 years ago by casella

See also #3642 and b1e94ad

comment:14 Changed 9 years ago by adeas31

Seems like this ticket is done. I think we should close it now.

comment:15 Changed 9 years ago by casella

Unfortunately, not yet. We need to fix #3698 first, I discovered it today. I hope there are no other such problems around. It might be wise to do some more testing...

comment:16 Changed 9 years ago by casella

Still one more issue left.

Create a model M, add some stuff in it with some extra spaces and save it. Then duplicate the model. The duplicated model does not preserve the spaces and formatting of the original one, and is rather formatted to OMEdit's liking.

comment:17 Changed 9 years ago by adeas31

Yes, because duplicate uses the copyClass API and omc pretty-printing.

comment:18 Changed 9 years ago by casella

I guess it shouldn't, then :)

The basic principle should be that any automatic pretty-printing should be optional, not automatic

comment:19 Changed 9 years ago by adeas31

But then how are you going to duplicate the class? I mean how can you duplicate a package saved in directories? How can you duplicate a class and put it into another package and then update the components relative paths and so on.

comment:20 Changed 9 years ago by casella

If the model is duplicated in the same scope, there is actually nothing to change in the code of the duplicated model, except the name of the model in the first and last lines. It seems weird that the formatting is completely changed in this case. Maybe we could at least try to preserve the formatting in this specific circumstance. If you duplicate in a different scope and there are component paths to change, let OMC pretty-printing kick in.

Duplicating a model and then modifying it is a very commmon development pattern, so losing the formatting each time and having to reapply it manually seems really a big waste of time.

comment:21 Changed 9 years ago by sjoelund.se

Adeel: You can use the diff API using the original file(s) for the class and compare them to the new ones. This should be able to preserve some things better.

comment:22 Changed 9 years ago by sjoelund.se

  • Milestone changed from 1.9.4 to 1.9.5

Milestone pushed to 1.9.5

comment:23 Changed 9 years ago by sjoelund.se

  • Milestone changed from 1.9.5 to 1.10.0

Milestone renamed

comment:24 Changed 8 years ago by casella

  • Resolution set to fixed
  • Status changed from new to closed

I guess this historical ticket can be closed now, as almost all the pending issues have been solved. We are using OMEdit routinely for the development of a prototype power system library and we haven't encountered any big problems for a long time now.

The only issue left is moved to #3912

comment:25 Changed 7 years ago by sjoelund.se

  • Milestone 1.10.0 deleted

Milestone deleted

Note: See TracTickets for help on using tickets.