Bugzilla – Full Text Bug Listing
| Summary: | Indent auto-wrapped lines | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] TMF | Reporter: | Mary Komor <mkomor> | ||||||
| Component: | Xtext | Assignee: | Project Inbox <tmf.xtext-inbox> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | chanskw, moritz.eysholdt, sebastian.zarnekow, sven.efftinge | ||||||
| Version: | unspecified | Flags: | sven.efftinge:
indigo+
sebastian.zarnekow: review+ |
||||||
| Target Milestone: | RC3 | ||||||||
| Hardware: | All | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Mary Komor
If I provide the patch, can the fix be made available in v2.0? That depends on the patch. It may not break existing APIs and we have to make sure that it does not change any behavior unintentionally. In order to be able to review it in time, a patch is welcome as soon as possible. Disclaimer: This is not a promise that we'll have the time to review a patch before 2.0 Created attachment 195459 [details]
Patch to support indentation of wrapped lines in the formatter
The attached patch contains the following:
1. FormattingConfig -- added a new field (wrappedLineIndentation) to hold the value for the number of tabs (or equivalent) that wrapped lines should be indented
2. FormattingConfigBasedStream -- when a line is wrapped, increment the indent of the LineEntry that is at the lastBreakableEntryIndex. Query the FormattingConfig for the number of indent level to add.
Hi Mary, thanks for the patch. Would it be possible for you to provide some unit tests, too? Are there any existing unit tests for the formatter that I can add to? You may want to have a look at org.eclipse.xtext.parsetree.formatter.FormatterTest (In reply to comment #6) > You may want to have a look at > org.eclipse.xtext.parsetree.formatter.FormatterTest Are the test packages kept up to date? I checked them out from the GIT repository, but they fail to build. I have updated and tested using org.eclipse.xtext.parsetree.formatter. Should I provide a patch for the updated test cases? (In reply to comment #8) > I have updated and tested using org.eclipse.xtext.parsetree.formatter. Should I > provide a patch for the updated test cases? That would be great. Created attachment 196174 [details]
Patch to update test suite to use indentation of wrapped lines in the formatter
The attached patch has the following updates
1. org.eclipse.xtext.parsetree.formatter.FormatterTestConfig -- set indentation for wrapped lines to 1 tab (or equivalent) in the FormattingConfig
2. org.eclipse.xtext.parsetree.formatter.FormatterTest -- updated the following to include the indentation (tab) in the expected value:
o testIndentationAndLineWrap
o testLinewrapDatatypeRuleComments
o testSuppressedLinewrap
Hi Mary,
thx for your contribution. However, I don't quite understand how it improves the indentation-mechanism over what we already have:
Let's say there is a rule like:
---
Rule: 'foo' val+=ID 'bar';
---
and you configure
---
cfg.SetAutoLineWrap(20);
cfg.setIndentationStart().after(grammar.getFooKeyword);
cfg.setIndentationEnd().after(grammar.getBarKeyword);
---
then this indentation will only be applied if auto-linewrap kicks in.
e.g.
"foo a b c d bar" will become "foo a b c d bar"
and
"foo a b c d e f g h i j k l m n o p bar" will become
---
foo a b c d e f g h
i j k l m n o p
bar
---
Please refer to FormatterTest.testIndentationAndLineWrap() as an example.
doesn't this solve your use-case?
regards,
Moritz
(In reply to comment #11) Hi Moritz, I'm looking specifically for a way to indent lines that are wrapped because it has exceeded max line width (auto-linewrap) setting. From what I understand of FormattingConfig.setIndentation(AbstractElement, AbstractElement), it indents all new/wrapped lines between the beginElement and endElement, regardless of whether the line was wrapped because of a setLineBreak on a keyword or because the line exceeded the max line width (auto-linewrap). Is my understanding correct? If so, then I'm looking for something that has more granular control. I want to specify additional indentation for lines that get wrapped because the max line width has been exceeded. So, using a variation of your example in comment #11, --- Rule: 'foo' val+=ID 'c' val2+=ID 'bar'; --- and I configure --- cfg.SetAutoLineWrap(20); cfg.setLineWrap.after(grammar.getCKeyword); cfg.setIndentation(grammar.getFooKeyword, grammar.getBarKeyword); --- then indentation will be applied when auto-linewrap kicks in as well as when the "c" keyword is encountered e.g. "foo a b c d bar" will become --- foo a b c d bar --- and "foo a b c d e f g h i j k l m n o p bar" will become --- foo a b c d e f g h i j k <-- line wrapped & indented because of setIndentation l m n o p bar <-- line wrapped & indented because of auto-linewrap, but the wrapped line is flushed with previous (parent)line --- What I want is --- foo a b c d e f g h i j k <-- line wrapped & indented because of setIndentation l m n o p bar <-- line wrapped & indented further because this line was wrapped when auto-linewrap(max line width) was exceeded --- Here's a Java example: --- cfg.SetAutoLineWrap(40); cfg.setLineWrap.after(grammar.getSemi); cfg.setLineWrap.after(grammar.getLeftCurly); cfg.setIndentation(grammar.getLeftCurly, grammar.getRightCurly); --- e.g. --- public void myMethod(String param1, String param2) { System.out.println("Prints line number one"); System.out.println("Prints line number two"); } --- will become --- public void myMethod(String param1, String param2) { System.out.println("Prints line number one"); System.out.println("Prints line number two"); } --- but I want --- public void myMethod(String param1, String param2) { System.out.println("Prints line number one"); System.out.println("Prints line number two"); } --- I hope I made some sense... Regards, Mary Just wondering where we are with this. Did my previous comment clarify things. I hope I did not end up confusing you even more. Mary Hi Mary, thanks for the clarification. We'll review your patch carefully but I cannot promise that it'll make it into the 2.0 release since we are already late in the end-game for Indigo. Regards, Sebastian Preliminary scheduled for RC3 to make sure it'll be reviewed. The patches look good to me. I'd apply them for RC3. What do others think? (In reply to comment #12) > What I want is > > --- > foo a b c > d e f g h i j k <-- line wrapped & indented because of setIndentation > l m n o p bar <-- line wrapped & indented further because this line > was wrapped when auto-linewrap(max line width) > was exceeded > --- hi Mary, you can get this formatting with this configuration: --- cfg.SetAutoLineWrap(20); cfg.setLineWrap.after(grammar.getCKeyword); cfg.setIndentation(grammar.getCKeyword, grammar.getBarKeyword); cfg.setIndentation(grammar.getFooKeyword, grammar.getBarKeyword); --- why isn't that sufficient? The same is true for your java example: --- cfg.SetAutoLineWrap(40); cfg.setLineWrap.after(grammar.getSemi); cfg.setLineWrap.after(grammar.getLeftCurly); cfg.setIndentation(grammar.getLeftCurly, grammar.getRightCurly); cfg.setIndentation(grammar.getLeftParenthesis, grammar.getRightParenthesis); --- all you need to do is define indentation between the parenthesis that surround the println's parameters. You are saying that one needs to distinguish between indentation applied to explicitly-wrapped lines and indentation applied to auto-wrapped lines. The use cases you've provided don't show why this should be necessary. (In reply to comment #17) > --- > cfg.SetAutoLineWrap(20); > cfg.setLineWrap.after(grammar.getCKeyword); > cfg.setIndentation(grammar.getCKeyword, grammar.getBarKeyword); > cfg.setIndentation(grammar.getFooKeyword, grammar.getBarKeyword); > --- my bad, this is supposed to be --- cfg.SetAutoLineWrap(20); cfg.setLineWrap.after(grammar.getCKeyword); cfg.setIndentation(grammar.getDKeyword, grammar.getBarKeyword); cfg.setIndentation(grammar.getFooKeyword, grammar.getBarKeyword); --- it needs to be 'DKeyword', otherwise 'c' gets indented twice. Hi Moritz, Thanks for your consideration. Yes, I can probably explicitly set indentation in all the appropriate places, but I would end up painstakingly setting the indentation for just about every rule in my grammar! I am asking for a more generic way to indent lines that are wrapped because the line has exceeded the max line width (i.e. "auto-wrapped"). For the simple examples that we have been using in this defect, it is easy to do and looks trivial. Once you take a real-world (i.e. complex) grammar file, it starts to get really tedious (assuming that it *is* possible) if you have to explicitly set the indentation for everything. Once the rule definition starts to get complex, it gets harder to easily set the indentation between two keywords. Mary (In reply to comment #19) > (...) I am asking for a more > generic way to indent lines that are wrapped because the line has exceeded the > max line width (i.e. "auto-wrapped"). (...) I agree that such scenarios do exist and that a generic way to specify indentation does simplify them. I've applied your patch, thx for it. This is in time for RC3. pushed to master. (In reply to comment #21) > pushed to master. Awesome! Thanks! Closing all bugs that were set to RESOLVED before Neon.0 Closing all bugs that were set to RESOLVED before Neon.0 |