IAutoWrapFormatter with indent ignores newline under certain conditions

[schwarfl]

Hello,

I'm trying to implement a formatter which indents lines which are autowrapped. Similiar to slide 128 from de.slideshare.net/meysholdt/xtexts-new-formatter-api. So e.g. a model like if: "CONDITION" = "VALUE1", "VALUE2", "VALUE3", "VALUE4", "VALUE5", "VALUE6", "VALUE7" ("RESULT") should be displayed as

if: "CONDITION" = "VALUE1", "VALUE2", 
    "VALUE3", "VALUE4", "VALUE5", 
    "VALUE6", "VALUE7"
("RESULT")

Since I don't have any terminating keyword, I use regionForEObject.nextHiddenRegion to get the end hidden region for the indent. Everything works fine as long as the last part of the multi-wrapped line contains more than one element. So e.g. the example above works fine. But if the last part contains only one element, the indentation seems to cause a conflict with the newline for the result element. E.g.

    if: "CONDITION" = "VALUE1", "VALUE2", "VALUE3", "VALUE4", "VALUE5", "VALUE6"  ("RESULT1")
    if: "CONDITION" = "VALUE7", "VALUE8", "VALUE9", "VALUE10", "VALUE11", "VALUE12"  ("RESULT2") 

will be displayed as

if: "CONDITION" = "VALUE1", "VALUE2",
      "VALUE3", "VALUE4", "VALUE5",
      "VALUE6" ("RESULT1")

if: "CONDITION" = "VALUE7", "VALUE8",
            "VALUE9", "VALUE10", "VALUE11",
            "VALUE12" ("RESULT2")

The instruction for a newline seems to be ignored and the second entry is indented twice. My DSL is very simple:

Model:
  rules+=Rules*;

Rules:
  condition = Condition
  result = Result
;

Result:
  "(" values += STRING ("," values += STRING)* ")"
;

Condition:
  "if:" condition = ConditionAtom
;

ConditionAtom: 
  name = STRING "=" 
  values += STRING ("," values += STRING)*
;

The formatter is also quite simple:

class MyDslFormatter extends AbstractFormatter2 {

    @Inject extension MyDslGrammarAccess

    def dispatch void format(Model model, extension IFormattableDocument document) {
        for (Rules rules : model.getRules()) {
            rules.format;
        }
    }

    def dispatch void format(Rules rules, extension IFormattableDocument document) {
        rules.getCondition.format;
        rules.getResult.format;
    }

    def dispatch void format(Result result, extension IFormattableDocument document){
        result.regionFor.keyword("(").prepend[newLine]
    }

    def dispatch void format(Condition condition, extension IFormattableDocument document) {
        format(condition.condition, document);
    }

    private static class BooleanWrapper {
        boolean first = true;

        def isFirst(){
            return first;
        }

        def setIsFirst(boolean value){
            first = value;
        }
    }

    def dispatch void format(ConditionAtom conditionatom, extension IFormattableDocument document) {
        val firstIndent = new BooleanWrapper() // needed to prevent double indent if line is autowrapped twice
        val access = conditionAtomAccess
        val IAutowrapFormatter autoWrapIndent  = [region, wrapped, doc |
            if (firstIndent.isFirst){
                firstIndent.isFirst = false
                val first = (region as IWhitespace).hiddenRegion
                val last = conditionatom.regionForEObject.nextHiddenRegion
                println("indent white for " + first.offset + " to " + last.offset + " (" + region.textRegionAccess.regionForDocument.text.substring(first.offset, last.offset) + ") ")
                doc.set(first, last, [indent])
            }
        ]
        conditionatom.allRegionsFor.keywords(",").forEach[append[autowrap; onAutowrap = autoWrapIndent]]
    }
}

I have attached my workspace with the dsl and a simple test to show the problem. The test testLongLastLinePart runs fine but the test testShortLastLinePart has the problem described above. workspaceXtext.zip

Best regards Florian Schwarz

[cdietrich]

see https://www.eclipse.org/forums/index.php/t/1084863/ as well

[cdietrich]

@franzbecker and i did some anaysis on the problem.

the Problem occurs if the newline for line and a explicit newLine fall together. this results in a conflict hat is "handled" in org.eclipse.xtext.formatting2.internal.HiddenRegionFormattingMerger.merge(List<? extends IHiddenRegionFormatting>)

this handling does lead to creation and replacement of new replacers in org.eclipse.xtext.formatting2.internal.TextReplacerMerger.mergeHiddenRegionReplacers(List<? extends ITextReplacer>) which basically changes the ArrayListTextSegmentSet

so that the loop in org.eclipse.xtext.formatting2.internal.FormattableDocument.createReplacements(ITextReplacerContext) will loop over the replacer r cause getReplacers().get(r) == r and no addFirst is called.

possible workaround (without breaking api):

• new Marker result:

  static class HiddenRegionFormattingConflictResolution extends HiddenRegionFormatting {
  
      public HiddenRegionFormattingConflictResolution(AbstractFormatter2 formatter/*, IHiddenRegionFormatting delegate*/) {
          super(formatter);
      }
  
  }

• change code of org.eclipse.xtext.formatting2.internal.HiddenRegionFormattingMerger.merge(List<? extends IHiddenRegionFormatting>) to

Iterator<? extends IHiddenRegionFormatting> iterator = Lists.reverse(conflicting).iterator();
        IHiddenRegionFormatting first = iterator.next();
        while(iterator.hasNext()) {
            IHiddenRegionFormatting next = iterator.next();
            first.mergeValuesFrom(next);
        }
        return new HiddenRegionFormattingConflictResolution(formatter);

and the code of org.eclipse.xtext.formatting2.internal.TextReplacerMerger.mergeHiddenRegionReplacers(List<? extends ITextReplacer>) to

protected ITextReplacer mergeHiddenRegionReplacers(List<? extends ITextReplacer> conflicting) {
        List<IHiddenRegionFormatting> formattings = Lists.newArrayList();
        Map<IHiddenRegionFormatting, ITextReplacer> map = Maps.newHashMap();
        ITextReplacer lastReplacer = null;
        IHiddenRegion region = null;
        for (ITextReplacer replacer : conflicting) {
            if (replacer instanceof HiddenRegionReplacer) {
                HiddenRegionReplacer hiddenRegionReplacer = (HiddenRegionReplacer) replacer;
                formattings.add(hiddenRegionReplacer.getFormatting());
                map.put(hiddenRegionReplacer.getFormatting(), hiddenRegionReplacer);
                lastReplacer = hiddenRegionReplacer;
                if (region == null)
                    region = hiddenRegionReplacer.getRegion();
                else if (region != hiddenRegionReplacer.getRegion())
                    return null;
            } else
                return null;
        }
        IHiddenRegionFormatting mergedFormatting = merger.merge(formattings);
        if (mergedFormatting instanceof HiddenRegionFormattingConflictResolution) {
            return lastReplacer ;
        }
        if (mergedFormatting != null)
            return formatter.createHiddenRegionReplacer(region, mergedFormatting);
        return null;
    }

alternatively we could simply always no care about the result and do a

Iterator<? extends IHiddenRegionFormatting> iterator = Lists.reverse(conflicting).iterator();
        IHiddenRegionFormatting first = iterator.next();
        while(iterator.hasNext()) {
            IHiddenRegionFormatting next = iterator.next();
            first.mergeValuesFrom(next);
        }
        return null;

    protected ITextReplacer mergeHiddenRegionReplacers(List<? extends ITextReplacer> conflicting) {
        List<IHiddenRegionFormatting> formattings = Lists.newArrayList();
        ITextReplacer lastReplacer = null;
        IHiddenRegion region = null;
        for (ITextReplacer replacer : conflicting) {
            if (replacer instanceof HiddenRegionReplacer) {
                HiddenRegionReplacer hiddenRegionReplacer = (HiddenRegionReplacer) replacer;
                formattings.add(hiddenRegionReplacer.getFormatting());
                lastReplacer = hiddenRegionReplacer;
                if (region == null)
                    region = hiddenRegionReplacer.getRegion();
                else if (region != hiddenRegionReplacer.getRegion())
                    return null;
            } else
                return null;
        }
        merger.merge(formattings);
        return lastReplacer;
    }

i guess we need some insights from @meysholdt or @svenefftinge next week to know whats the intended behaviours:

• should there be a conflict at all or should the autowrap area end one char before
• should the while loop at FormattableDocument.createReplacements behave differently
• should the complete chain from FormattableDocument.createReplacements to TextReplacerMerger.mergeHiddenRegionReplacers have the ability to return/change replacers

[meysholdt]

hey @schwarfl, thank you for reporting this. Thank you @cdietrich and @franzbecker for the analysis.

The bug was in FormattableDocument.createReplacements: When an autowrap triggered a rollback, ITextReplacers that were modified by the IAutowrapFormatter were ignored.