Bugzilla – Full Text Bug Listing
| Summary: | [Folding] review FoldingAPI | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] TMF | Reporter: | Michael Clay <clay> | ||||||||||||
| Component: | Xtext | Assignee: | Project Inbox <tmf.xtext-inbox> | ||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | sebastian.zarnekow, sven.efftinge | ||||||||||||
| Version: | 1.0.0 | Flags: | sebastian.zarnekow:
indigo+
|
||||||||||||
| Target Milestone: | M5 | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Could you write a sentence or two about why you want to deprecate it? Maybe link it to another related and existing bugzilla. I like the idea to be able to fold multi-line leaf nodes by means of the API in the default implementation. Yes, that makes sense. But it looks like the #getPosition is never called for anything else than CompositeNode by the default implementation. Maybe we should add folding for multi line comments in order to show why #getPosition is typed to AbstractNode now. I suggest to do this in Indigo and then completely replace the old method. Scheduled according to Sven's suggestion. Michael, are you willing to contribute folding for ML_comments, unit tests and an updated patch that removes the deprecated #getPosition(.., CompositeNode)? (In reply to comment #5) > Michael, are you willing to contribute folding for ML_comments, unit tests and > an updated patch that removes the deprecated #getPosition(.., CompositeNode)? yes Created attachment 179405 [details]
proposed patch, pls review
We should rather leverage the IDocument partitioning in order to compute the regions for multiline comments. In case users want to add folding for other multiline tokens, they can do, but it's rare and not worth the amount of code. I like the change to use an Acceptor instead of a list. But please reduce the interface to the single accept method and we need only one implementation (the List-based one) which should be null safe. Logger declarations should be static (i.e. keep the old one). Created attachment 179994 [details]
revised patch
Hi Michael,
I've the impression that the API exposes to much implementation detail to clients. I'ld assume clients want to provide offset+lenght+optional text for a folded region. The do not want to hassle with details such as IFoldingRegion or the fact that regions that do not span multiple lines should not be folded.
What do you think about the following acceptor interface.
interface IFoldingRegionAcceptor {
void accept(int offset, int length, StyledString text);
void accept(int offset, int length);
}
The one and only acceptor implementation should deal with all this details.
A minor remark: I'd prefer private loggers since they encourage the common usage pattern that every class has its own logger. I don't think that we need a NullCompositeNode.
Regards,
Sebastian
Created attachment 180205 [details]
patch with revised IFoldingRegionAcceptor API
Michael, are you still working on this one? Removed milestone. Created attachment 185369 [details]
update patch to new node model
Patch applied. Closing all bugs that were set to RESOLVED before Neon.0 Closing all bugs that were set to RESOLVED before Neon.0 |
Created attachment 170657 [details] proposed patch, pls review should be deprecated and replaced with DefaultFoldingRegionProvider#getPosition(IXtextDocument,AbstractNode)