Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 315249 - [Folding] review FoldingAPI
Summary: [Folding] review FoldingAPI
Status: CLOSED FIXED
Alias: None
Product: TMF
Classification: Modeling
Component: Xtext (show other bugs)
Version: 1.0.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: M5   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-01 12:31 EDT by Michael Clay CLA
Modified: 2017-09-19 18:16 EDT (History)
2 users (show)

See Also:
sebastian.zarnekow: indigo+


Attachments
proposed patch, pls review (2.16 KB, patch)
2010-06-01 12:31 EDT, Michael Clay CLA
no flags Details | Diff
proposed patch, pls review (17.25 KB, patch)
2010-09-22 15:49 EDT, Michael Clay CLA
no flags Details | Diff
revised patch (15.97 KB, patch)
2010-09-30 15:56 EDT, Michael Clay CLA
no flags Details | Diff
patch with revised IFoldingRegionAcceptor API (18.43 KB, patch)
2010-10-04 16:05 EDT, Michael Clay CLA
no flags Details | Diff
update patch to new node model (19.40 KB, patch)
2010-12-16 15:48 EST, Michael Clay CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Clay CLA 2010-06-01 12:31:27 EDT
Created attachment 170657 [details]
proposed patch, pls review

should be deprecated and replaced with
DefaultFoldingRegionProvider#getPosition(IXtextDocument,AbstractNode)
Comment 1 Sven Efftinge CLA 2010-06-18 05:59:11 EDT
Could you write a sentence or two about why you want to deprecate it?
Maybe link it to another related and existing bugzilla.
Comment 2 Sebastian Zarnekow CLA 2010-06-25 11:26:42 EDT
I like the idea to be able to fold multi-line leaf nodes by means of the API in the default implementation.
Comment 3 Sven Efftinge CLA 2010-08-03 05:27:55 EDT
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.
Comment 4 Sebastian Zarnekow CLA 2010-08-25 13:20:21 EDT
Scheduled according to Sven's suggestion.
Comment 5 Sebastian Zarnekow CLA 2010-09-20 10:34:30 EDT
Michael, are you willing to contribute folding for ML_comments, unit tests and an updated patch that removes the deprecated #getPosition(.., CompositeNode)?
Comment 6 Michael Clay CLA 2010-09-20 15:08:36 EDT
(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
Comment 7 Michael Clay CLA 2010-09-22 15:49:22 EDT
Created attachment 179405 [details]
proposed patch, pls review
Comment 8 Sven Efftinge CLA 2010-09-27 06:29:41 EDT
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).
Comment 9 Michael Clay CLA 2010-09-30 15:56:31 EDT
Created attachment 179994 [details]
revised patch
Comment 10 Sebastian Zarnekow CLA 2010-10-03 12:36:51 EDT
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
Comment 11 Michael Clay CLA 2010-10-04 16:05:56 EDT
Created attachment 180205 [details]
patch with revised IFoldingRegionAcceptor API
Comment 12 Sebastian Zarnekow CLA 2010-12-06 03:48:09 EST
Michael, are you still working on this one?
Comment 13 Sebastian Zarnekow CLA 2010-12-15 11:42:29 EST
Removed milestone.
Comment 14 Michael Clay CLA 2010-12-16 15:48:35 EST
Created attachment 185369 [details]
update patch to new node model
Comment 15 Sebastian Zarnekow CLA 2011-01-04 04:45:18 EST
Patch applied.
Comment 16 Karsten Thoms CLA 2017-09-19 18:07:07 EDT
Closing all bugs that were set to RESOLVED before Neon.0
Comment 17 Karsten Thoms CLA 2017-09-19 18:16:43 EDT
Closing all bugs that were set to RESOLVED before Neon.0