Community
Participate
Working Groups
The current implementation of code folding for SSE is turned off by default because it is inefficient. This is because the current code folding implementation uses node adapters which hang up the models they are attached to. The proposed fix is to use the Reconciler framework to implement the code folding so that it will be calculated in the background and not rely on listeners on the model.
Created attachment 140009 [details] Fix Patch This patch implements the suggested fix to re-implement code folding using the reconciler framework rather then the node adapter framework. Folding strategy implementations have been written for XML, CSS and DTD, and the XML implementation is plugged in for not only XML but HTML and JSP as well, effectively implementing behind the scenes code folding for XML, HTML, JSP, CSS and DTD. The new folding strategies are plugin in in a similar way to the old ones, through an editor configuration provisional configuration extension with a type of "foldingstrategy" the old type being "structruedtextfoldingprovider" the type was changed for differentiation from the old way to the new way. This patch not only implements the new way but also removes all file from the old way, deleting all now unused classes and in some classes packages. All removed classes and packages are/were internal. One of the advantages of the new implementation is that there is only one class needed to implement a new folding strategy for a content type as apposed to the old way which needed 4 or 5 different classes with much duplicated code among the different folding providers. The new implementation eliminates code duplication leaving only two simple methods to be implemented to create a new folding strategy. Tied in with this fix is a related fix to the preference for validating as you type. In the preferences this option is under General->Editors->Structured Text Editor and is labeled "Report problems as you type". Previously this option if unchecked would disable not only the as you type validation but also the highlighting and spell checking which have their own preference check boxes. Not only this the preference if changed would only be recognized as changed if any open editors were first closed and reopened. Because the new code folding is tied in in a similar manor to the highlighting and spell checking this had to be fixed to be sure code folding would not be disabled by this preference. Thus this patch also includes a fix so that the "report problems as you type" preference only disables the as you type validation and nothing else, and also will take effect, through the use of the existing property listeners, even if an editor is already open. Also do do with preferences this patch now enables code folding by default in the SSE seeing as it was disabled by default because of its inefficiencies. Testing: I have tested as many different scenarios as I can on XML, HTML, JSP, CSS, and DTD files to see how the code folding reacts. this includes but is certainly not limited to, adding and removing blocks of code or individual nodes, removing parts of nodes to make them invalid (folding should disappear for that node), copy and past different parts of code, moving end tags and start tags so they wrap more or less content, etc. It is certain that I could not think of every possible action someone could make in the editors to see the code folding reaction but I can say that for at least normal use code folding now works as expected. Other: Code folding for DTDs is rather week and does not do much for anything other then comments and really long ELEMENT tags (which are typically short). This is because syntax such as <[ ]> is not turned into an IndexedRegion by the DTD parser so in turn each opening and close tag comes in separately as Undefined DTDNodes. To get more useful code folding out of DTD the parser would need to be updated to recognize this syntax as IndexedRegions. I have documented all of my code but if there are any further questions, concerns, or comments let me know.
Created attachment 140118 [details] Updated Fix Patch This fixes a logged null pointer I ran into while I had this patch applied during other work.
*** Bug 269600 has been marked as a duplicate of this bug. ***
*** Bug 192230 has been marked as a duplicate of this bug. ***
I'm fairly certain this merits mentioning on http://wiki.eclipse.org/New_Help_for_Old_Friends_V .
(In reply to comment #5) > I'm fairly certain this merits mentioning on > http://wiki.eclipse.org/New_Help_for_Old_Friends_V . > I have added something there for this based on what I saw on previous "new help for old friends" pages.
Ian, Overall, this patch looks good. I'm glad to see some steps being taken forward in improving the design of our content folding support. So far, the things I can see that might need cleaned up a bit: 1. We might have talked about this before, but were the expand/collapse icons in the gutter put on the same line as the start tag closing bracket in order to not hide the attributes for multi-lined start tags: <tag foo="bar" baz="boo"> 2. I tried pasting the content for a stylesheet (https://bugs.eclipse.org/bugs/skins/standard/global.css) into a new CSS file. The default template has it so the only thing in the document is the encoding. If I do a Ctrl+A, and replace all of the text with the content from global.css, I end up having the entire stylesheet collapsible. 3. Since content types are hierarchical, the folding strategy should apply to base types. For example, XSL will not pick up the default XML folding strategy. So I think, in the absence of a folding strategy existing for the content type, the base type hierarchy should be explored for an applicable folding strategy. 4. What is the purpose of AbstractStructuredFoldingStrategy#internalSetDocument()? It seems like it's doing a bit of work that's handled by reconcile() anyway. 5. In DocumentRegionProcessor, you'll want to null out fFoldingStrategy in setDocument() so that getFoldingStrategy() will reinitialize the strategy. The strategy should also be told to remove itself from its viewer's projectListener before setting to null. As an aside, setViewer should probably check that if its fViewer != null, it should remove itself as a projectionListener from the old fViewer. 6. In DocumentRegionProcessor, you'll need to make sure you clean up the FoldingStrategy... in particular, when you set the viewer, you add the FoldingStrategy as a projection listener. You're going to need to make sure that you remove it when it is no longer needed, or you'll be leaky. 7. In jsp.ui's plugin.xml, you have a foldingstrategy for "org.eclipse.wst.css.core.csssource", which I assume is for CSS JSP which has a content type id of "org.eclipse.jst.jsp.core.cssjspsource"
Ideally also there should be unit tests to go with various parts of the test suites to make sure that folding is being applied to the correct nodes when appropriate. It'll also help with verification of fixes later on. A bit more work upfront, but helps in the long term.
Created attachment 142029 [details] Patch with Nick's Suggestions (In reply to comment #7) > So far, the things I can see that might need cleaned up a bit: > 1. We might have talked about this before, but were the expand/collapse icons > in the gutter put on the same line as the start tag closing bracket in order to > not hide the attributes for multi-lined start tags: > <tag foo="bar" > baz="boo"> The folding area was already set to be at the end of the start tag so that attributes would not be folded, but the exception to this was start tags with no end tags or tags that are the start and end tag such as <foo /> in this case I was allowing the tag to be folded, but this patch removes that functionality. For CSS documents the strategy was updated so that the start of the folding would start after the selector text so that rules with lots of selectors would not fold away the selectors. > 2. I tried pasting the content for a stylesheet > (https://bugs.eclipse.org/bugs/skins/standard/global.css) into a new CSS file. > The default template has it so the only thing in the document is the encoding. > If I do a Ctrl+A, and replace all of the text with the content from global.css, > I end up having the entire stylesheet collapsible. This was fixed by the fix for number 4. > 3. Since content types are hierarchical, the folding strategy should apply to > base types. For example, XSL will not pick up the default XML folding strategy. > So I think, in the absence of a folding strategy existing for the content type, > the base type hierarchy should be explored for an applicable folding strategy. Have not yet figured out a way to search the content type hierarchy but when/if I do will update the patch with that enhancement. For now I did add the XSL content type to those associated with the XML folding strategy. > 4. What is the purpose of > AbstractStructuredFoldingStrategy#internalSetDocument()? It seems like it's > doing a bit of work that's handled by reconcile() anyway. When I first wrote the code I was thinking that it made sence to parse the whole document when it was first set and then to just updated as changes were made. But it turns out the "update" code I wrote in reconcile gets notified when the document is frist set and that it can parse the new document perfictly well on its own. Not on that but it fixed the issue with CSS documents. The solution being to just remove the setInternalDocument method. > 5. In DocumentRegionProcessor, you'll want to null out fFoldingStrategy in > setDocument() so that getFoldingStrategy() will reinitialize the strategy. The > strategy should also be told to remove itself from its viewer's projectListener > before setting to null. As an aside, setViewer should probably check that if > its fViewer != null, it should remove itself as a projectionListener from the > old fViewer. Done and done. > 6. In DocumentRegionProcessor, you'll need to make sure you clean up the > FoldingStrategy... in particular, when you set the viewer, you add the > FoldingStrategy as a projection listener. You're going to need to make sure > that you remove it when it is no longer needed, or you'll be leaky. added an uninstall method to AbstractStructuredFoldingStrategy that should take care of avoiding any leaks. > 7. In jsp.ui's plugin.xml, you have a foldingstrategy for > "org.eclipse.wst.css.core.csssource", which I assume is for CSS JSP which has a > content type id of "org.eclipse.jst.jsp.core.cssjspsource" Fixed.
(In reply to comment #8) > Ideally also there should be unit tests to go with various parts of the test > suites to make sure that folding is being applied to the correct nodes when > appropriate. > > It'll also help with verification of fixes later on. A bit more work upfront, > but helps in the long term. > Agreed. I will write up some JUnits and attach here.
(In reply to comment #8) > Ideally also there should be unit tests to go with various parts of the test > suites to make sure that folding is being applied to the correct nodes when > appropriate. > > It'll also help with verification of fixes later on. A bit more work upfront, > but helps in the long term. > I have run into a bit of a snag with the JUnit tests. The problem is that with the way the folding is writen now it is calculated as a ReconcilingStrategy which runs in the background. This means that I/the test have no way of knowing when this background reconciling has finished so that I can then test to be sure all of the correct annotations have been added in. The only idea have come up with so far is to put some X time limit on how long the test will wait for the annotations to happen and fail after X time if all of the expected annotations have not shown up. But I don't like this because the whole point of putting the folding on the reconciler was for it to run in the background whenever it got a chance, so putting a time limit on it on the test could cause "random" JUnit failures. Some other suggestion as to how to deal with this problem would be appreciated. Does anyone know of how the other reconcilers are tested, if at all?
Created attachment 142070 [details] Updated Patch with Nick's Suggestions > 3. Since content types are hierarchical, the folding strategy should apply to > base types. For example, XSL will not pick up the default XML folding strategy. > So I think, in the absence of a folding strategy existing for the content type, > the base type hierarchy should be explored for an applicable folding strategy. This patch now incorporates the hierarchical nature of the content types. If a folding strategy for the doc type can not be found then its parent doc types will be searched until it finds one. This patch removes the explicit xsl extension to use the XML strategy tat was added in in the last version of this patch and instead relies on this new hierarchy search to successfully use the XML folding strategy.
Created attachment 142072 [details] Updated Patch with Nick's Suggestions (marked as patch this time) Sorry forgot to mark the last attachment as a patch, this is the same as the previous patch, just now marked as such.
(In reply to comment #11) > I have run into a bit of a snag with the JUnit tests. The problem is that with > the way the folding is writen now it is calculated as a ReconcilingStrategy > which runs in the background. This means that I/the test have no way of > knowing when this background reconciling has finished so that I can then test > to be sure all of the correct annotations have been added in. > > The only idea have come up with so far is to put some X time limit on how long > the test will wait for the annotations to happen and fail after X time if all > of the expected annotations have not shown up. But I don't like this because > the whole point of putting the folding on the reconciler was for it to run in > the background whenever it got a chance, so putting a time limit on it on the > test could cause "random" JUnit failures. > > Some other suggestion as to how to deal with this problem would be appreciated. > Does anyone know of how the other reconcilers are tested, if at all? I ran into similar issues with testing Launch Configurations. You may need to create up some Mock classes or interfaces to test this. In the launch configurations for XSL I kept doing a periodic pool on the Job that was running, to check to see when it was completed. If the Job returned that it was done, then I proceeded witht he rest of the unit test to check the results. You may beable to do something similar. Another option might be to have a Listener that is checked and notifies the unit test when the job has completed. You can take a look at the XSL Launch Configuration tests to see how I implemented those tests, might give you some ideas. You could also look at EasyMock which allows you a way to setup and test code using Mock objects. That is available in Orbit now. http://en.wikipedia.org/wiki/Mock_object
Created attachment 142163 [details] Folding Enhancment Patch I found a couple bugs with the patch when writing the JUnits. JUnits to follow.
Created attachment 142164 [details] Folding JUnits Patch This patch adds in JUnit tests for the XML, CSS, and DTD folding strategies.
Created attachment 143324 [details] Folding Enhancment Patch 2 Some testing code snuck into the last patch and caused issues which Nick found. This patch removes that bad testing code.
Created attachment 143325 [details] Folding JUnits Patch 2 There was an issue with the XML folding test for testing XSL files, this fixes it.
Created attachment 145666 [details] Enhacment and JUnits Fix Patch This patch combines the JUnits and the enhancement patch into one. It also updates the XML folding so that multi-line XMLattributes get folded as well. The JUnits have also been updated to reflect this change.
With the latest patch, I have 5 failing unit tests for xml.ui.tests--are you seeing the same? TestReconcilerXML: testIllFormedNoCloseBracket testIllFormedNoAttrValue TestSourceValidationFramework testSourceValidationEnablementWithInheritedValidators testSourceValidationEnablementWithUniqueValidators testSourceValidationEnablementWithUnrelatedContentTypes
(In reply to comment #20) > With the latest patch, I have 5 failing unit tests for xml.ui.tests--are you > seeing the same? > > TestReconcilerXML: > testIllFormedNoCloseBracket > testIllFormedNoAttrValue > > TestSourceValidationFramework > testSourceValidationEnablementWithInheritedValidators > testSourceValidationEnablementWithUniqueValidators > testSourceValidationEnablementWithUnrelatedContentTypes > Hrm, yes. Don't ever remember seeing thous before. I'll look into it.
Created attachment 145702 [details] Enhacment and JUnits Fix Patch Update 1 There was an issue with the default value for whether to do as you type validation or not. This patch has the needed update. I have re-run all SSE JUnits and everything is now passing.
Changes released. Thanks for the contribution!
A big +1 for this fix. I just tested it with some of my extremely large XML Schema files (several thousand global elements) and the sucker opened in less than a second with code folding enabled. Before it would take up to 2 minutes to open it.
One thing I have noticed with this is that formatting may need to turn off code folding and then turn it back on for performance reasons. There seems to be some slow down when formatting a large xml file. Of course this was always there, but something to consider as we tune in the coming milestones.
(In reply to comment #24) > A big +1 for this fix. I just tested it with some of my extremely large XML > Schema files (several thousand global elements) and the sucker opened in less > than a second with code folding enabled. Before it would take up to 2 minutes > to open it. > I agree ++1. It's nice to have this enabled by default now
glad to hear its working well.
*** Bug 259640 has been marked as a duplicate of this bug. ***
Verified on 3.2M2-S-20090921_063829
*** Bug 158877 has been marked as a duplicate of this bug. ***
*** Bug 298370 has been marked as a duplicate of this bug. ***
