Community
Participate
Working Groups
Adding proposal cycling to the StructuredContentAssistant would be useful for proposals like templates.
What is the difference between this bug and Bug 294097?
*** Bug 294097 has been marked as a duplicate of this bug. ***
Created attachment 152867 [details] Enhancment Patch in progress This is the cycling enhancement thus far. So far its all only implemented for XML documents and gives you a page for tags and a page for templates. It should not be to bad to finishing implimenting it for the rest of the content types. Work left to be done: * lots of commenting * implement other content types * depreciate old classes and implement them so they actually use the new framework because the actual implementation of the content assist is pretty much copy/past from the old stuff so to avoid having to copies around I am going to make the old stuff use the new stuff. This way there will only be one copy of the code and adopters using the now legacy code wont be broken. When I put up the final patch I will go into more detail about how this thing actually works. But in general its a copy of the JDT content assist but then totally overhauled to work for different content types and structured documents. There are two new extension points in the sse.ui plugin, one for adding new content assist computers and computer categories. And another for allowing user control of which categories get displayed on the default page and/or own their own page. Again, I'll go into much more detail when its done and I am sure nothing is changing.
Ian, while I know this doesn't do cycling, how is this going to affect the contentAssitanceProcessor extension point that was recently added in 3.2M3 to the wst.xsl component? Also, since WST.XSL needs it's own content assitance outside of the XML component (i.e. we don't necessarily want the XSL stuff showing up in the plain XML editor), we need to do additional testing.
(In reply to comment #4) > Ian, while I know this doesn't do cycling, how is this going to affect the > contentAssitanceProcessor extension point that was recently added in 3.2M3 to > the wst.xsl component? Are you referring to the editorConfiguration extension point with a provisionalConfiguration element of type "contentassistprocessor"? If so I have set it up so any implementations of this now "legacy" extension have their results added to the default page of the content assist. > > Also, since WST.XSL needs it's own content assitance outside of the XML > component (i.e. we don't necessarily want the XSL stuff showing up in the plain > XML editor), we need to do additional testing. Not exactly sure what you are referring to here. If the WST.XSL already has a contentassistprocessor extension set for a specific content type that content assist will still work specifically for that content type. I will be posting full descriptions of the new extension points and finalized code this afternoon.
(In reply to comment #5) > (In reply to comment #4) > > Ian, while I know this doesn't do cycling, how is this going to affect the > > contentAssitanceProcessor extension point that was recently added in 3.2M3 to > > the wst.xsl component? > > Are you referring to the editorConfiguration extension point with a > provisionalConfiguration element of type "contentassistprocessor"? > > If so I have set it up so any implementations of this now "legacy" extension > have their results added to the default page of the content assist No, I'm talking about the content assistance processor extension point created in bug 259721. . Here is what I'm looking for and will need in any type of cycling. Content Assistance from XML and particularly XML is going to have to take in more than just the content type that is working on. In XML technologies it needs to take into account the Namespaces that are active within the XML document. Currently with the extension point implemented in bug 259721, this is left up to the processor to determine whether it needs to provide the content assistance for a particular namespace or not. In XSL it also needs to not only take into account the namespaces defined, but also the version of the XSL that is being implemented. XSL has an extension mechnanism in the language that allows for other custom XSL extensions to be supported by a processor. One such common extension is the EXSLT library. http://www.exslt.org/ There is also FXSL as well. http://fxsl.sourceforge.net/ Some of these can be contributed through the current namespace support from the XML editor, other's though require business logic, and XPath content assistance, which has to be contributed through processor extensions via the SSE. Unfortunately the current design of the processor extension in SSE only takes in one processor, and there is no easy way for Adopters to contribute additional processors. The XSL Processor extension point while it doesn't do cycles it does allow an easy way for additional Processor content assistance to be added by adopters. The XSL SourceViewerConfiguration where the Content Assistance processors are setup reads the available processors from the extension points and will make them available during content assistance. It is then up to the processors to determine if they need to be active based on the namespace or xml element they are currently active in. In XSL I contribute both the existing XMLContentAssistProcessor and the XSLContentAssistProcessor through this new extension point. I also have an example of the EXSLT Commons being contributed through it as well. So I think you need to allow cycling if we are reworking this based on not only template, but namespaces as well.
Created attachment 153652 [details] Enhancment Patch in progress Update 1 This patch is almost ready to go. At this point I am commenting and cleaning up code. There are still a couple of minor issues the biggest one being finding a way to separate the HTML tag proposals and JSP tag proposals generated from a model query on JSP documents. Dave, I have not yet looked into your comments. I need to see what XSL is doing to determine how this will affect it. My guess and hope is none at all. But finding a way to integrate the XSL extension point with my new ones might be another trick/possibility. I have commented my extension point schemas so if you wanted to take a look at the extension point I have added and start pondering how we may integrate that might be helpful.
What would really help here, is having some Unit tests for the various functionality and also know the amount of code coverage from those tests (less important). A patch of this size and complexity really should have unit tests to go with it. I'm sure you have some tests that you did to make sure that what you wrote is working, but these if they are manual need to be automated. It might also allow you to do some refactoring and help remove any code duplication you may have. Plus it would help in testing out the XSL use case as well.
(In reply to comment #8) > What would really help here, is having some Unit tests for the various > functionality and also know the amount of code coverage from those tests (less > important). A patch of this size and complexity really should have unit tests > to go with it. I'm sure you have some tests that you did to make sure that > what you wrote is working, but these if they are manual need to be automated. > > It might also allow you to do some refactoring and help remove any code > duplication you may have. Plus it would help in testing out the XSL use case > as well. I have every intention of writing a JUnit suite to rival that of the patch itself. I have just been waiting to do it until the patch had become stable from during my adhoc smoke tests. I meant to mentioned that in my last comment but obviously forgot.
Created attachment 153751 [details] Enhancment Patch in progress Update 2 Same patch only now fully commented. Still some things to work out as well as writing some JUnits.
Created attachment 153771 [details] Enhancment Patch in progress Update 3 Updated the patch so that my quick smoke testing of XSL content assist works now. I looked at the XSL code briefly and everything should keep working as it is now but the XSL content assist would now be using classes I have depreciated. I am currently playing with how XSL could leverage this enhancement.
Created attachment 153774 [details] Patch for XSL to use enhancment This is the patch that would be needed to make it so that XSL at least partially took advantage of this enhancement. Mostly an update to the plugin xml of xsl.ui but also a needed update to to StructuredTextViewerConfigurationXSL. Dave if you could take a look and let me know what you are thinking or what else you might need. With this patch the XSL specific content assist is still added through your xsl.ui.contentAssistProcessor extension, but the XML content assist is now added through my extension.
Created attachment 153837 [details] Enhancment Patch You will notice this patch is no longer labeled as "in progress" that is because it is at a state where I think it is about ready. The only thing I have left to do is write the JUnits. Since the last patch I have also added functionality so that the categories displayed on the default content assist page can have their order changed by the preference page and this order can be different then that of the separate pages order. I have also added in my suggested defaults for the order of the categories on the default pages and the ordering for the pages after the default page (this is done for XML, HTML, and JSP). As of now there are no categories that by default are not displayed on their default page as well as all categories currently have their own page to cycle to after the default page. If anyone has any suggestions for making these defaults any different I am open to suggestions. I guess I can't put it off any longer now, time to write some JUnits...
Created attachment 153838 [details] JSP Java catigory icon This is the icon that I have set for the JSP java content assist category. It is borrowed from org.eclipse.jdt.ui. It needs to go in: org.eclipse.jst.jsp.ui/icons/full/elcl16
Chris, I am adding you as a CC because I thought you may want to check out quickly how this effects JSDT. From my quick testing your content assist extension (now legacy) still works in script sections on HTML and JSP documents. Once this gets in you may want to look at implementing the new extension points.
Created attachment 153917 [details] Attempt at doing JUnits (not working) On the JUnit front I have run into a bit of a snag. The SSEContentAssistProcessor relies on a CompletionListener registered on the ContentAssistant to set up things for iteration on content assist start and end notifications. Problem is these notifications seem to only be able to be sent out when the content assist is invoked through the viewer (can be programicly invoked through doOperation) but then the results are displayed to the GUI and I have no way of accessing them directly. I am attaching my attempted modifications to TestXMLContentAssist. If you look at the new code you will notice in getProposals I use the assistant to get the processor for the current offset and then attempt to compute the completion proposals, but that ends up throughing an exception (which is logged) because the SSEContentAssistProcessor hasn't been setup because tits CompletionListener never gets invoked. Suggestions? My only thought is adding in hooks in the StructuredContentAssistant to be able to manually invoke the content listeners, but I don't like adding code into the 'real' code simply to make testing work.
(In reply to comment #16) > Created an attachment (id=153917) [details] > Attempt at doing JUnits (not working) > > On the JUnit front I have run into a bit of a snag. The > SSEContentAssistProcessor relies on a CompletionListener registered on the > ContentAssistant to set up things for iteration on content assist start and end > notifications. Problem is these notifications seem to only be able to be sent > out when the content assist is invoked through the viewer (can be programicly > invoked through doOperation) but then the results are displayed to the GUI and > I have no way of accessing them directly. > Suggestions? My only thought is adding in hooks in the > StructuredContentAssistant to be able to manually invoke the content listeners, > but I don't like adding code into the 'real' code simply to make testing work. My thoughts are that you are going to need to do some mocking of this so that you can test it. http://en.wikipedia.org/wiki/Mock_object EasyMock is part of Orbit and may help you in this particular case. http://easymock.org/ In this case it looks like you need to setup a Mock inmplementation of the SSEContentAssistProcessor and a CompletionListener that you can control and kick off during your tests. Also, I would not modify the existing proposal tests if possible, I would setup separate tests to run through the existing code. Especially if we are not yet removing the old code, that still needs to be under test as well. Also, some how the Gui is able to display the information so we might need to Mock out that portion so that we can access the information during the tests.
> > My thoughts are that you are going to need to do some mocking of this so that > you can test it. > > http://en.wikipedia.org/wiki/Mock_object > > EasyMock is part of Orbit and may help you in this particular case. > > http://easymock.org/ > > In this case it looks like you need to setup a Mock inmplementation of the > SSEContentAssistProcessor and a CompletionListener that you can control and > kick off during your tests. thanks, I'll take a look into it. > > Also, I would not modify the existing proposal tests if possible, I would setup > separate tests to run through the existing code. Especially if we are not yet > removing the old code, that still needs to be under test as well. Far point, will do. > > Also, some how the Gui is able to display the information so we might need to > Mock out that portion so that we can access the information during the tests.
(In reply to comment #17) > > My thoughts are that you are going to need to do some mocking of this so that > you can test it. > > http://en.wikipedia.org/wiki/Mock_object > > EasyMock is part of Orbit and may help you in this particular case. > > http://easymock.org/ > > In this case it looks like you need to setup a Mock inmplementation of the > SSEContentAssistProcessor and a CompletionListener that you can control and > kick off during your tests. I was looking into this mock stuff but it doesn't seem to do what I need to accomplish, and we definitely wouldn't want to Mock the SSEContentAssistProcessor because that's the root of what we are trying to test. I tried taking a sneeky aproach by creating "fack" subclasses of the StructuredContentAssistatn and StructuredTextViewerConfigurationXML so that i could add in a method to manualy fire the content assist event from the assistant but the ContentAssistEvent class is final and its contructors are private so I can not create one of these events or make a child class of it that I can create so that I can send out the event manually. And because the ContentAssistant fireSessionBeginEvent is "default" protection I can manage to override it if I put my fake subclass in the same package name as ContentAssistant but I can't then call the super (which is the only place these ContentAssistEvents are created) because java says its the super method is not visible. So unless anyone has any suggestions my next approach is to use java reflection trickery to access the private methods.
(In reply to comment #19) > (In reply to comment #17) > > > > My thoughts are that you are going to need to do some mocking of this so that > > you can test it. > > > I tried taking a sneeky aproach by creating "fack" subclasses of the > StructuredContentAssistatn and StructuredTextViewerConfigurationXML so that i > could add in a method to manualy fire the content assist event from the > assistant but the ContentAssistEvent class is final and its contructors are Thus why we should avoid finals. They make things more difficult to test. > private so I can not create one of these events or make a child class of it > that I can create so that I can send out the event manually. And because the > ContentAssistant fireSessionBeginEvent is "default" protection I can manage to > override it if I put my fake subclass in the same package name as > ContentAssistant but I can't then call the super (which is the only place these > ContentAssistEvents are created) because java says its the super method is not > visible. None of the classes I believe you are talking about are Public API (there is very little in the way of public API in XML, that is another issue altogether). So to me you should be able to do some refactorings to help make the testing easier. If any of these are using Interfaces, that is where Mocking comes into play. If you are having a difficulty with testing it, imagine the difficulty adopters may have implementing or using it. > So unless anyone has any suggestions my next approach is to use java reflection > trickery to access the private methods. See above comments, I'm for refactoring the stuff to make it easier to test, but I'm only one vote.
(In reply to comment #20) > Thus why we should avoid finals. They make things more difficult to test. > The method causing the issue is in org.eclipse.jface.text.contentassist.ContentAssistant so I/we have no controll over the access restrictions. > > None of the classes I believe you are talking about are Public API (there is > very little in the way of public API in XML, that is another issue altogether). > So to me you should be able to do some refactorings to help make the testing > easier. If any of these are using Interfaces, that is where Mocking comes > into play. If you are having a difficulty with testing it, imagine the > difficulty adopters may have implementing or using it. > The only problem here is that the SSEContentAssistProcessor uses a CompletionListener to know when a content assist request is starting and ending and its events are only sent out by the above mentioned ContentAssistant. Using java reflection I am easily able to call fireSessionBeginEvent and fireSessionEndEvent methods in ContentAssistant and simulate a content assist starting and ending and then everything else works without any modification. With these events now being simulated through the reflection it is simple to get the processor from the assistant (that is public api) and request the proposals from it (even multiple requests work to get each subsequent page). The difficulty in testing here I do not believe is from the design of the SSEContentAssistProcessor but that this code is all designed to be activated through UI means.
(In reply to comment #21) > The difficulty in testing here I do not believe is from the design of the > SSEContentAssistProcessor but that this code is all designed to be activated > through UI means. The code in question where you are having the problems is not designed to be testable. You can test code for the UI if it is designed with testing in mind. The problem is that there isn't a good separation between the UI portion and the business logic portion, they are intermixed. At least you found a way to get around the issue. Hate having to use reflection though when one shouldn't have to.
(In reply to comment #22) > The code in question where you are having the problems is not designed to be > testable. You can test code for the UI if it is designed with testing in mind. > The problem is that there isn't a good separation between the UI portion and > the business logic portion, they are intermixed. It would be possible to move the implimentation of the ICompletionListener out of the SSEContentAssistProcessor and have one have a reference to the other. This in turn would give you access to the assistSessionStarted method which is what we need to be called. But its parameter is a ContentAssistEvent which by jfaces design is a final class with private constructors. So my tests would not be able to create instances of them to in turn be able to call the assistSessionStarted manually. Which brings us right back to reflection. > At least you found a way to get around the issue. Hate having to use > reflection though when one shouldn't have to. If it was production code obviously reflection would be unspeakable to use. But its testing code and all is fair in love, war, and testing :)
Created attachment 154163 [details] Enhancment Patch with JUnits This is the enhancement patch with the XML and HTML JUnits done. I am working on the JSP JUnits, there are a lot of scenarios to test for JSPs.
Created attachment 154331 [details] Enhancment Patch with JUnits - Update 1 This is the fully finished patch with JUnits ready for review. Also need to attach a testing zip to go into one of the testing plugins.
Created attachment 154332 [details] JUnit testing project ZIP This zip needs to go in: org.eclipse.jst.jsp.ui.tests/projecttestfiles/
*** Bug 246939 has been marked as a duplicate of this bug. ***
Created attachment 156153 [details] Enhancment Patch with JUnits - Update 2 There was an update to one of the patched classes that made the patch be in conflict, this is an updated patch that will apply cleanly.
Comment on attachment 156153 [details] Enhancment Patch with JUnits - Update 2 Realized that I forgot to implement CSS content assist....Finishing up JUnits for it now.
Created attachment 156269 [details] Enhancement Patch with JUnits - Update 3 As I mentioned in comment #29 I made the slight oversight of not implementing this wonderful new framework for CSS documents. As a testament to the robustness of the new framework I was able to get content assist running in CSS documents in under an hour with all the associated preference pages. What took a little longer was getting it working in embedded CSS. Though even here the only issue was tweaking the existing CSS content assist code to play nice. Part of my trickery here was to update the StructuredTextPartitionerForHTML to return ICSSPartitions.STYLE for the attribute value region of style attributes. In this way it was easy to hook in the CSS content assist computer for the HTML content type and the ICSSPartitions.STYLE partition type and things magically worked. Okay almost magically worked, minus the slight tweaking to deal with some offset issues because of the pesky "" surrounding the attribute value regions. This update also comes with more fun JUnits. A suite of tests for CSS documents as well as a suite of tests for CSS embedded in HTML documents. Testing both for CSS in between style tags and inside style attribute value regions. While doing this I did notice that the syntax highlighting inside of the style attribute value regions does not work correctly, but this is not something new because testing without my patch yielded the same issues. I will be opening another bug to track fixing that issue.
Created attachment 156558 [details] Enhancement Patch with JUnits - Update 4 Added some JUnits.
Created attachment 156559 [details] JUnit JSP testing project ZIP - Update 1 Updated for additional JUnits.
Created attachment 156568 [details] Enhancement Patch with JUnits - Update 5 Cleaned up some formatting stuff and snuck in a very simple fix for bug 103771.
Created attachment 156642 [details] Enhancement Patch with JUnits - Update 6 Needed to include fix from Bug 295961.
Created attachment 157147 [details] Enhancement Patch with JUnits - Update 7 Included fix for Bug 107138 because its patch for head had become stale and rather then re-writing it for head later to be replaced by this fix, thought it was more worth it just to include in here.
Created attachment 157148 [details] Enhancement Patch with JUnits - Update 8 Apologies, that last patch didn't actually have the updates in it.
Created attachment 157191 [details] Enhancement Patch with JUnits - Update 9 Add more embedded CSS content assist JUnits
Created attachment 158521 [details] Enhancement Patch with JUnits - Update 10 Same as last patch except re-created because some conflicts had developed in the test plugins due to some changes there, this patch should now apply cleanly.
Created attachment 159208 [details] Enhancement Patch with JUnits - Update 11 This update includes all of the items both Nick and Nitin requested be updated before committing. This includes: * some spelling fixes * no provisional packages, what needs to be API is, and what doesn't is hidden * remove the group surrounding the CSS content assist properties seeing as they are currently the only thing on the page * added CSS template content assist for appropriate regions on HTML and JSP documents * added the new preferences pages to the preference pages that show when right clicking on a file and choosing "preferences..." * there were some changes to HTMLMOdelQueryImpl that should not have been there * separate out the changes to StructuredTextPartitionerForHTML, LineStyleProviderForEmbeddedCSS, & CSSContentAssistContext to support content assist in CSS attribute value regions, this is now tracked by Bug 302953 (note without the fix for Bug 302953 not all the JUnits in this patch will pass, but there was no good way to separate them out without making conflicting patches, and see as both patches should really go in at the same time this should be a problem) I discovered that Eclipse patches do not like empty files in that it will not include them in a patch. To this end for all the JUnits to pass you must manually add to blank files. org.eclipse.wst.css.ui.tests/testresources/contentassist/test2.css org.eclipse.wst.html.ui.tests/testresources/contentassist/test0.html I also fixed a bunch of copyright dates I forgot to update or that were copy/pasted incorrectly
Created attachment 159236 [details] Enhancement Patch with JUnits - Update 12 Same as comment #39 except fixes model memory leak that Nick found. Don't know how that one slipped by.
(In reply to comment #40) > Created an attachment (id=159236) [details] > Enhancement Patch with JUnits - Update 12 > > Same as comment #39 except fixes model memory leak that Nick found. Don't know > how that one slipped by. Hey Ian, Right now, I'm getting a couple failing unit tests. For HTML, it seems like test0.html is missing. For JSP, testJSPInScriptTagProposals is failing where pages 0, 2, and 3 expect 83 proposals but have 94 instead. For CSS, it's missing test2.css. Thanks.
*** Bug 117351 has been marked as a duplicate of this bug. ***
(In reply to comment #41) > Right now, I'm getting a couple failing unit tests. For HTML, it seems like > test0.html is missing. For JSP, testJSPInScriptTagProposals is failing where > pages 0, 2, and 3 expect 83 proposals but have 94 instead. For CSS, it's > missing test2.css. > > Thanks. Hi Nick, I am guessing you did not read all the way through comment #39 :) > I discovered that Eclipse patches do not like empty files in that it will not > include them in a patch. To this end for all the JUnits to pass you must > manually add to blank files. > > org.eclipse.wst.css.ui.tests/testresources/contentassist/test2.css > org.eclipse.wst.html.ui.tests/testresources/contentassist/test0.html > Let me know if you have any other issues.
(In reply to comment #43) > (In reply to comment #41) > > Right now, I'm getting a couple failing unit tests. For HTML, it seems like > > test0.html is missing. For JSP, testJSPInScriptTagProposals is failing where > > pages 0, 2, and 3 expect 83 proposals but have 94 instead. For CSS, it's > > missing test2.css. > > > > Thanks. > > Hi Nick, I am guessing you did not read all the way through comment #39 :) Caught me. > > > I discovered that Eclipse patches do not like empty files in that it will not > > include them in a patch. To this end for all the JUnits to pass you must > > manually add to blank files. > > > > org.eclipse.wst.css.ui.tests/testresources/contentassist/test2.css > > org.eclipse.wst.html.ui.tests/testresources/contentassist/test0.html > > > > > Let me know if you have any other issues.
Created attachment 159996 [details] Enhancement Patch with JUnits - Update 13 Same as last patch except removed script region tests because that is really something that should be tested by JSDT.
Created attachment 160112 [details] Enhancement Patch with JUnits - Update 14 Made it so that content types would automatically pick up the content assist computers registered for their parent content types. This means that if you have a content type that extends XML you will automatically pick up the XML content assist assuming your StructuredTextViewerConfiguration#getContentAssistProcessors returns an instance of StructuredContentAssistProcessor. This basically insures we do not break any adapters. The one case that adopters will loose some content assist is if their content type extends contentTypeA but their StructuredTextViewerConfiguration extends a StructuredTextViewerConfiguration designed for contentTypeB and are not overriding #getContentAssistProcessors. In this case the adopter will be getting content assist for contentTypeA and not contentTypeB. This is a very rare scenario and can be delt with by the adopter by using the new completionProposal#proposalComputerExtendedActivation extension.
Created attachment 160121 [details] Enhancement Patch with JUnits - Update 15 Made it so JSP taglib tags will not be suggested in attribute value regions for XHTML style documents. Also fixed the icon being used for these types of suggestions.
Created attachment 160251 [details] Enhancement Patch with JUnits - Update 16 Patch updated to take advantage of changes brought by the patch for bug 303974. This patch does NOT include the patch for bug 303974 but it DOES include code that depends on the patch for bug 303974, therefor this patch will NOT compile without the patch for bug 303974 also applied.
Created attachment 160357 [details] Enhancement Patch with JUnits - Update 17 Nick is telling me the last patch would not apply, possible cause is I may have accidentally included a testing zip in the last patch. Hopefully this patch works.
Code has been released. Thanks for the persistence, Ian.
*** Bug 96769 has been marked as a duplicate of this bug. ***
