ssi-schaefer / lcdsl

Eclipse Launch Configuration DSL (Xtext based)
Eclipse Public License 1.0
23 stars 12 forks source link

Formatting of launch configuration files not possible #13

Closed wnickl closed 4 years ago

wnickl commented 7 years ago

Using CTRL+SHIFT+F in eclipse results in "complete content of file is moved to first line of file".

So instead of

java configuration XXX : BaseJavaLog4jLaunch { memory max = 1280M; main-class com.test.something.Main; project com.test.something; argument "42"; vm-argument "-Dtarget.home=${target_home}" ; }

I get

java configuration XXX : BaseJavaLog4jLaunch { memory max = 1280M; main-class om.test.something.Main; project com.test.something; argument "42"; vm-argument -Dtarget.home=${target_home}" ; }

miklossy commented 4 years ago

Such a result usually comes if the Xtext default formatter is used, but in LcDsl, a custom formatter implementation is already bound in the AbstractLcDslRuntimeModule, so I think this issue has been solved in the meantime and can be closed.

mduft commented 4 years ago

Yes, a formatter is bound and in place. However it does not fully work, and I haven't had time to look into the issue. It seems that indentation is only correctly applied for the first line of a block, but not for the others...

miklossy commented 4 years ago

Thanks for the info. You are right, I experienced the same behaviour. I will take a look at it and try to provide a pull request to improve the formatter.

mduft commented 4 years ago

This would be really appreciated, thanks!

miklossy commented 4 years ago

Hi Markus,

I analysed the problem and found out that in order to make the indentation properly work, the formatter should append a newline to each line. A quick solution could be to override the void format(EObject it, extension IFormattableDocument document) method in the LcDslFormatter class:

class LcDslFormatter extends AbstractFormatter2 {
    ...
    override dispatch void format(EObject it, extension IFormattableDocument document) {
        regionForEObject.allSemanticRegions.last.append[newLine]
    }
}

However, it could result in some unwanted side-effects when applying this customization to all EObjects. Do you have the possibility to test it on some *.lc files?

Thanks a lot for you feedback! Tamás

mduft commented 4 years ago

Hey,

Thanks for the analysis! I can certainly test this, yes :) It will just take me a little time to do so...

Cheers, Markus

mduft commented 4 years ago

I tried this out quickly, and it seems that once I remove indentation on any line within a launch configuration, format will not bring it back :| It seems this kind of override is also never called, since the other dispatch methods in the file get precedence when they match - at least a Syserr in the method was never called.

miklossy commented 4 years ago

Hmm.. interesting. Actually it seems to work on my computer: screencast

miklossy commented 4 years ago

@mduft Could it be possible that your code base is different from that what is checked in in the master branch? https://github.com/mduft/lcdsl/blob/master/com.wamas.ide.launching/src/com/wamas/ide/launching/formatting2/LcDslFormatter.xtend

mduft commented 4 years ago

Nope, I just freshly cloned, maybe something else went wrong... You just added that method as it is shown at the end of the file, right? Because that's what I tried :D

mduft commented 4 years ago

Ah, I think I found it - will re-test :) There was a problem with my setup.

mduft commented 4 years ago

OK, now it works. The only problem I observed when formatting our lc files is that blank lines disappear... We have a lot of blank lines separating blocks of arguments belonging together, blank lines before comments, etc.

miklossy commented 4 years ago

Preserving the already present new lines can be controlled by the setNewLines(int minNewLines, int defaultNewLines, int maxNewLines) parameters.

    /**
     * Configures the given new lines for this hidden region. Keeps the current configuration if it is in the valid
     * boundaries of {@code minNewLines} and {@code maxNewLines}. Applies {@code defaultNewLines} otherwise.
     */
    void setNewLines(int minNewLines, int defaultNewLines, int maxNewLines);

Instead of calling regionForEObject.allSemanticRegions.last.append[newLine], you can call e.g. regionForEObject.allSemanticRegions.last.append[setNewLines(1,1,2)] screencast Hint: In order to avoid org.eclipse.xtext.formatting2.ConflictingFormattingException: Conflicting values, you may have to adapt the [newline] parameters on other places as well.

mduft commented 4 years ago

Nice, thanks :) Can you provide a pull request? I would otherwise try to squeeze the changes in somewhere in the next days...

miklossy commented 4 years ago

Yes, I will provide a PR soon.

mduft commented 4 years ago

This is great news, thanks :)

miklossy commented 4 years ago

I created the PR

mduft commented 4 years ago

Thanks a lot. Will release ASAP.

mduft commented 4 years ago

New build is available at https://mduft.github.io/lcdsl-latest/ - you can verify (and hopefully enjoy) your changes there :)