GRECLIPSE
  1. GRECLIPSE
  2. GRECLIPSE-1029

Ability to augment/customize the outline view for Groovy scripts and other files

    Details

    • Type: New Feature New Feature
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.2Release
    • Fix Version/s: 2.5.0Release
    • Component/s: Editor
    • Labels:
      None
    • Number of attachments :
      6

      Description

      Currently, the outline view for scripts is not too useful. It would be nice to be able to have some custom outline views available for scripts.

      Also, in the long term, we should consider exposing an extension point so that third party plugins can provide even more ousomization.

        Issue Links

          Activity

          Hide
          Andrew Eisenberg added a comment -

          There is a method JavaEditor.setOutlinePageInput() that could be overridden to supply this information. We would essentially need to pass in a mock IJavaElement that has its children ordered in an appropriate way.

          Show
          Andrew Eisenberg added a comment - There is a method JavaEditor.setOutlinePageInput() that could be overridden to supply this information. We would essentially need to pass in a mock IJavaElement that has its children ordered in an appropriate way.
          Hide
          Maxime HAMM added a comment -

          Here is my proposition
          1. The groovy Outline page manage to fill the outline view and also to synchronize the tree nodes with the editor content. The groovy editor checks all extenders; the first one that returns an outline page will win... by default (of course) the standard outline page is used.
          2. The extender receives the IProject and the GroovyCompilationUnit. It should inherit and implement the methods below :
          a. OCompilationUnit.refreshChildren : this method will allow the extender to build and refresh the IOJavaElement tree
          b. OCompilationUnit.getChildrenAt(int caretOffset) : this method is used to synchronize the Editor view with the outline view
          c. OMethod.getElementNameNode, OMethod.getReturnTypeName, OType.getElementNameNode : those methods will determine the outline lines labels
          (I have’nt yet implemented a OField classe because I did’nt need it for Jsoresso, but I will do it later)
          Maxime

          Show
          Maxime HAMM added a comment - Here is my proposition 1. The groovy Outline page manage to fill the outline view and also to synchronize the tree nodes with the editor content. The groovy editor checks all extenders; the first one that returns an outline page will win... by default (of course) the standard outline page is used. 2. The extender receives the IProject and the GroovyCompilationUnit. It should inherit and implement the methods below : a. OCompilationUnit.refreshChildren : this method will allow the extender to build and refresh the IOJavaElement tree b. OCompilationUnit.getChildrenAt(int caretOffset) : this method is used to synchronize the Editor view with the outline view c. OMethod.getElementNameNode, OMethod.getReturnTypeName, OType.getElementNameNode : those methods will determine the outline lines labels (I have’nt yet implemented a OField classe because I did’nt need it for Jsoresso, but I will do it later) Maxime
          Hide
          Maxime HAMM added a comment - - edited

          The first file is the patch for the org.codehaus.groovy.eclipse.ui project
          The second file is the Jspresso DSL extender

          Show
          Maxime HAMM added a comment - - edited The first file is the patch for the org.codehaus.groovy.eclipse.ui project The second file is the Jspresso DSL extender
          Maxime HAMM made changes -
          Field Original Value New Value
          Attachment patch-0411.patch [ 54620 ]
          Attachment jspresso.zip [ 54621 ]
          Hide
          Andrew Eisenberg added a comment -

          This patch looks good, but there are a few things that need to be worked on before we can apply it.

          • There is no OField class
          • In IOutlineExtender.getGroovyOutlinePageForUnit change method signature to remove the third element. It is redundant since the GroovyCompilationUnit can be retrieved directly from the GroovyEditor.
          • I saw a few e.printStackTrace(). They should be removed. Instead, GroovyCore.logException(...)
          • I don't understand why you overrode getDeclaringType() in OMethod and OType. I think this method can be removed (or tell me why it should remain).
          • In OCompilationUnit, override buildStructure() should be a noop and return true. This method will probably never get called, but if it ever is by accident, it could cause some big problems with the mode.
          • Delete: OCompilationUnit.getGroovyNode(). Seems like this method shouldn't be there.
          • Here is a big one...OutlineExtenderRegistry should not base which extender to use only on the nature. This can be problematic because let's say someone produces an extender for vanilla groovy projects so that scripts can have a custom outline view. We can safely assume that there will only be one extender per compilation unit, but I don't think we can assume there will be one per file.

          Here is a suggestion:

          1. add new method to IOutlineExtender:

          boolean appliesTo(GroovyCompilationUnit);

          Returns true if should be used for a particular file or false if not applicable.

          2. this will allow someone to create a script outline enhancer (which I am planning to do) that applies to all projects with a groovy nature

          3. Now, it might be that we want to lower the priority of the groovy script extender so that (for example) the jspresso plugin would have its extender run instead of the default groovy one. You may need to then force the ordering of the OutlineExtenderRegistry.natureToExtenderMap field so that extenders associated with groovy projects appear last.

          • Why does OMethod have a getElementNameNode()? That feels very specific to your own implementation. Can you document why this is general purpose and how to use it, or else remove it? I see that the method is used in OMethodInfo.getNameSourceStart() and OMethodInfo.getNameSourceEnd(). Instead of getElementNameNode(), why not have getNameStart() and getNameEnd()?
          • Last, of course, is discussing the testing, which I'll do in another comment.
          Show
          Andrew Eisenberg added a comment - This patch looks good, but there are a few things that need to be worked on before we can apply it. There is no OField class In IOutlineExtender.getGroovyOutlinePageForUnit change method signature to remove the third element. It is redundant since the GroovyCompilationUnit can be retrieved directly from the GroovyEditor. I saw a few e.printStackTrace() . They should be removed. Instead, GroovyCore.logException(...) I don't understand why you overrode getDeclaringType() in OMethod and OType. I think this method can be removed (or tell me why it should remain). In OCompilationUnit, override buildStructure() should be a noop and return true. This method will probably never get called, but if it ever is by accident, it could cause some big problems with the mode. Delete: OCompilationUnit.getGroovyNode(). Seems like this method shouldn't be there. Here is a big one...OutlineExtenderRegistry should not base which extender to use only on the nature. This can be problematic because let's say someone produces an extender for vanilla groovy projects so that scripts can have a custom outline view. We can safely assume that there will only be one extender per compilation unit, but I don't think we can assume there will be one per file. Here is a suggestion: 1. add new method to IOutlineExtender: boolean appliesTo(GroovyCompilationUnit); Returns true if should be used for a particular file or false if not applicable. 2. this will allow someone to create a script outline enhancer (which I am planning to do) that applies to all projects with a groovy nature 3. Now, it might be that we want to lower the priority of the groovy script extender so that (for example) the jspresso plugin would have its extender run instead of the default groovy one. You may need to then force the ordering of the OutlineExtenderRegistry.natureToExtenderMap field so that extenders associated with groovy projects appear last. Why does OMethod have a getElementNameNode()? That feels very specific to your own implementation. Can you document why this is general purpose and how to use it, or else remove it? I see that the method is used in OMethodInfo.getNameSourceStart() and OMethodInfo.getNameSourceEnd(). Instead of getElementNameNode(), why not have getNameStart() and getNameEnd()? Last, of course, is discussing the testing, which I'll do in another comment.
          Hide
          Andrew Eisenberg added a comment -

          From the mailing list:

          About tests :
          1) test the extension point stuff:

          • if there is no extenders then the outline contains the classical methods (main, etc.)
          • if there is one extenders that should create an outline O1 then the created outline is an O1 instance.
          • if there is two declared extenders, the first one wins.
            2) test the outline content.
            Let's say we have a test extender that produces an outline that contains a tree of OTypes that should be synchronized with groovy methods and inline methods. Each node contains also OMethod and OField sub nodes linked to the groovy...
          • test the outline content consistency
          • change one method : test outline content consistency
          • change one line method : test outline content consistency
          • remove one method : test outline content consistency
          • remove one inline method : test outline content consistency
          • add one method : test outline content consistency
          • add one inline method : test outline content consistency
          • select one IOElement : test selected groovy method consistency
          • select one groovy method : test selected IOElement node

          Regards,
          Maxime

          This seems to me to be a fairly complete list of tests. One comment, though...your test:

          • if there is two declared extenders, the first one wins.

          The behavior will be undefined if two or more extenders apply to the same file. I think this is OK. But, you will also have to add a test for boolean IOutlineExtender.appliesTo(GroovyCompilationUnit).

          Now, here are some suggestions and APIs that you can use to create the tests:

          • check out all of the plugins in https://svn.codehaus.org/groovy/eclipse/trunk/ide-test/ and from https://svn.codehaus.org/groovy/eclipse/trunk/test/
          • You can add your tests to the org.codehaus.groovy.eclipse.tests plugin
          • Your test class can subclass org.codehaus.groovy.eclipse.test.EclipseTestCase.
          • The test plugin already defines two test natures that you can use in your tests. You can create a couple of test extensions for the outline extender in the plugin.
          • The EclipseTestCase class has a field of type TestProject. You can use this field to perform many operations on java projects like add/remove natures, add/remove java and groovy classes, etc. This should be sufficient for what you need to do.
          • We don't do SWT testing, so there is no need to test that clicking on an outline selection will take you to the correct part of the code. This is manual testing that we do just prior to releases.

          Let me know if you have any questions.

          Show
          Andrew Eisenberg added a comment - From the mailing list: About tests : 1) test the extension point stuff: if there is no extenders then the outline contains the classical methods (main, etc.) if there is one extenders that should create an outline O1 then the created outline is an O1 instance. if there is two declared extenders, the first one wins. 2) test the outline content. Let's say we have a test extender that produces an outline that contains a tree of OTypes that should be synchronized with groovy methods and inline methods. Each node contains also OMethod and OField sub nodes linked to the groovy... test the outline content consistency change one method : test outline content consistency change one line method : test outline content consistency remove one method : test outline content consistency remove one inline method : test outline content consistency add one method : test outline content consistency add one inline method : test outline content consistency select one IOElement : test selected groovy method consistency select one groovy method : test selected IOElement node Regards, Maxime This seems to me to be a fairly complete list of tests. One comment, though...your test: if there is two declared extenders, the first one wins. The behavior will be undefined if two or more extenders apply to the same file. I think this is OK. But, you will also have to add a test for boolean IOutlineExtender.appliesTo(GroovyCompilationUnit) . Now, here are some suggestions and APIs that you can use to create the tests: check out all of the plugins in https://svn.codehaus.org/groovy/eclipse/trunk/ide-test/ and from https://svn.codehaus.org/groovy/eclipse/trunk/test/ You can add your tests to the org.codehaus.groovy.eclipse.tests plugin Your test class can subclass org.codehaus.groovy.eclipse.test.EclipseTestCase. The test plugin already defines two test natures that you can use in your tests. You can create a couple of test extensions for the outline extender in the plugin. The EclipseTestCase class has a field of type TestProject. You can use this field to perform many operations on java projects like add/remove natures, add/remove java and groovy classes, etc. This should be sufficient for what you need to do. We don't do SWT testing, so there is no need to test that clicking on an outline selection will take you to the correct part of the code. This is manual testing that we do just prior to releases. Let me know if you have any questions.
          Hide
          Maxime HAMM added a comment -

          Hi Andrew

          Here is a new proposal. I think I succeed to take account the changes you asked me. I also added some Javadoc.

          I attach a first draft code for tests... I think the first tests are fine... but tests about outline content aren't reliable : i would have base my tests onto Tree/TreeItems but i didn't find any solution...then I base my test on OType/OMethod/OFields... What do you think about that ?

          Regards,
          Maxime

          Show
          Maxime HAMM added a comment - Hi Andrew Here is a new proposal. I think I succeed to take account the changes you asked me. I also added some Javadoc. I attach a first draft code for tests... I think the first tests are fine... but tests about outline content aren't reliable : i would have base my tests onto Tree/TreeItems but i didn't find any solution...then I base my test on OType/OMethod/OFields... What do you think about that ? Regards, Maxime
          Maxime HAMM made changes -
          Maxime HAMM made changes -
          Attachment patch-0411.patch [ 54620 ]
          Hide
          Andrew Eisenberg added a comment -

          This looks great. The test coverage is fine as it is. It is not easy to do tests based on JFace components so, we usually test the model that the JFace components are based on. This has worked fine for us in the past.

          I see what you mean that the tests are unreliable. There is one test that is failing when run with the other tests, but it passes when it runs alone. I'm not exactly sure what the problem is, but it is probably something to do with the working copy or the indexes not being completely initialized.

          I'll have a look at it tomorrow.

          Show
          Andrew Eisenberg added a comment - This looks great. The test coverage is fine as it is. It is not easy to do tests based on JFace components so, we usually test the model that the JFace components are based on. This has worked fine for us in the past. I see what you mean that the tests are unreliable. There is one test that is failing when run with the other tests, but it passes when it runs alone. I'm not exactly sure what the problem is, but it is probably something to do with the working copy or the indexes not being completely initialized. I'll have a look at it tomorrow.
          Hide
          Andrew Eisenberg added a comment -

          OK. I fixed the tests. They are now passing reliably for me. I had to make a few changes to the tests as well as one or two changes to the plugin code.

          The biggest change that I made was to ensure that the groovy compilation unit was already a working copy and already reconciled before creating the outline view. This would ensure that GroocyCompilationUnit.getModuleNode() never returns null.

          This makes the tests pass, but it does make me a little concerned that we will get into situations when opening and editor and the outline view will be empty because the outline page was initialized too early (and could not get a ModuleNode). I don't know if this will be a problem, but we will see.

          Show
          Andrew Eisenberg added a comment - OK. I fixed the tests. They are now passing reliably for me. I had to make a few changes to the tests as well as one or two changes to the plugin code. The biggest change that I made was to ensure that the groovy compilation unit was already a working copy and already reconciled before creating the outline view. This would ensure that GroocyCompilationUnit.getModuleNode() never returns null. This makes the tests pass, but it does make me a little concerned that we will get into situations when opening and editor and the outline view will be empty because the outline page was initialized too early (and could not get a ModuleNode). I don't know if this will be a problem, but we will see.
          Hide
          Andrew Eisenberg added a comment -

          Resolving this issue now and committing the code. Thanks for your contribution!

          Show
          Andrew Eisenberg added a comment - Resolving this issue now and committing the code. Thanks for your contribution!
          Andrew Eisenberg made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Assignee Andrew Eisenberg [ werdna ]
          Fix Version/s 2.1.3Release [ 17240 ]
          Resolution Fixed [ 1 ]
          Hide
          Andrew Eisenberg added a comment -

          On second thought. Not quite ready to commit this.

          The major problem now is the expectation that once you are using an extended Outline page now, then all contained JavaElements will be OJavaElements. This doesn't really work since, for example in a script, you may have a standard method declaration or standard class declarations. These need to appear in the outline view as well, and I don't think we should be using wrappers for them.

          Show
          Andrew Eisenberg added a comment - On second thought. Not quite ready to commit this. The major problem now is the expectation that once you are using an extended Outline page now, then all contained JavaElements will be OJavaElements. This doesn't really work since, for example in a script, you may have a standard method declaration or standard class declarations. These need to appear in the outline view as well, and I don't think we should be using wrappers for them.
          Andrew Eisenberg made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Maxime HAMM added a comment - - edited

          Hello Andrew

          OK, the groovy outline view should also be able to receive normal IJavaElement or may be a mix of IJavaElement and IOJavaElement...

          Here is a new proposition (incremental patches to apply after previous ones),

          Thanks a lot... i'am happy to contribute to groovy eclipse code :o)

          Show
          Maxime HAMM added a comment - - edited Hello Andrew OK, the groovy outline view should also be able to receive normal IJavaElement or may be a mix of IJavaElement and IOJavaElement... Here is a new proposition (incremental patches to apply after previous ones), Thanks a lot... i'am happy to contribute to groovy eclipse code :o)
          Maxime HAMM made changes -
          Hide
          Andrew Eisenberg added a comment - - edited

          Thanks for the updated patch, but I have already made a bunch of local changes. Now, it is going to be very hard to apply your new changes. Times like this, I wish we were using git. Don't worry about this. I'll incorporate your changes.

          Show
          Andrew Eisenberg added a comment - - edited Thanks for the updated patch, but I have already made a bunch of local changes. Now, it is going to be very hard to apply your new changes. Times like this, I wish we were using git. Don't worry about this. I'll incorporate your changes.
          Hide
          Maxime HAMM added a comment -

          Just for me to know... will you finalize this soon ?

          Show
          Maxime HAMM added a comment - Just for me to know... will you finalize this soon ?
          Hide
          Andrew Eisenberg added a comment -

          Yes. I hope to do so today or tomorrow.

          Show
          Andrew Eisenberg added a comment - Yes. I hope to do so today or tomorrow.
          Hide
          Andrew Eisenberg added a comment -

          Here is the final patch that I will be committing. Take a look at it. The major changes are these:

          • Use getSourceElementAt() instead of getSourceReferenceAt(). This makes the OJavaElements more aligned with IJavaElements
          • Override buildStructure as a no-op in OCompilationUnit to make sure the method never gets called and causes some really bad things to happen
          • Created the GroovyScriptOutlineExtender for all groovy scripts
          • The OutlineExtenderRegistry will try the GroovyScriptOutlineExtender if no other outline extenders are used. It is not registered as a regular extension point
          • Allow multiple extenders to be registered per nature
          • Add some more tests for the groovy script extender.

          Thanks for the initial work on this. I don't think this would ever had been implemented without your patch.

          Show
          Andrew Eisenberg added a comment - Here is the final patch that I will be committing. Take a look at it. The major changes are these: Use getSourceElementAt() instead of getSourceReferenceAt(). This makes the OJavaElements more aligned with IJavaElements Override buildStructure as a no-op in OCompilationUnit to make sure the method never gets called and causes some really bad things to happen Created the GroovyScriptOutlineExtender for all groovy scripts The OutlineExtenderRegistry will try the GroovyScriptOutlineExtender if no other outline extenders are used. It is not registered as a regular extension point Allow multiple extenders to be registered per nature Add some more tests for the groovy script extender. Thanks for the initial work on this. I don't think this would ever had been implemented without your patch.
          Andrew Eisenberg made changes -
          Attachment GRECLIPSE-1029-final.patch [ 54682 ]
          Hide
          Andrew Eisenberg added a comment -

          Work is committed. Will be available in next dev build. Two things I notice:

          1. When using an extender, most of the outline buttons are disabled. They are explicitly removed from within GroovyOutlinePage.initializeViewer(). Was there a reason for this? (I never use them and I don't even know if they work in Groovy).
          2. If you start off with your Groovy file as a regular file, but then turn it into a script, the outline view does not change to reflect that. You must close and re-open the editor before the extended outline view is used. This is not really a big problem, I think.
          Show
          Andrew Eisenberg added a comment - Work is committed. Will be available in next dev build. Two things I notice: When using an extender, most of the outline buttons are disabled. They are explicitly removed from within GroovyOutlinePage.initializeViewer(). Was there a reason for this? (I never use them and I don't even know if they work in Groovy). If you start off with your Groovy file as a regular file, but then turn it into a script, the outline view does not change to reflect that. You must close and re-open the editor before the extended outline view is used. This is not really a big problem, I think.
          Hide
          Andrew Eisenberg added a comment -

          Initial work is complete. May open new issues for the bullet points in the previous comments.

          Show
          Andrew Eisenberg added a comment - Initial work is complete. May open new issues for the bullet points in the previous comments.
          Andrew Eisenberg made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Maxime HAMM added a comment - - edited

          Thank you Andrew !

          About the "getSourceReferenceAt" : this method is used to select a part of code in the editor when some node is selected... the outline page expected a "ISourceReference" that can be JavaElement but may be something else ? That's not important, i see that you kept the return type as ISourceReference...

          About the button, yes i think they are currently not working... I was thinking someone will work on that later and add some specific Groovy buttons... may be i will do it for my Jspresso plugin.

          It works fine in debug mode... i'm waiting the groovy eclipse site update to finalize my tests...

          Show
          Maxime HAMM added a comment - - edited Thank you Andrew ! About the "getSourceReferenceAt" : this method is used to select a part of code in the editor when some node is selected... the outline page expected a "ISourceReference" that can be JavaElement but may be something else ? That's not important, i see that you kept the return type as ISourceReference... About the button, yes i think they are currently not working... I was thinking someone will work on that later and add some specific Groovy buttons... may be i will do it for my Jspresso plugin. It works fine in debug mode... i'm waiting the groovy eclipse site update to finalize my tests...
          Hide
          Andrew Eisenberg added a comment -

          Dev build is now available. Let me know if it works for you.

          Show
          Andrew Eisenberg added a comment - Dev build is now available. Let me know if it works for you.
          Hide
          Maxime HAMM added a comment -

          Hello Andrew !
          It works fine,
          It is a new great feature for Jspresso DSL !
          Thank you for your help
          Maxime

          Show
          Maxime HAMM added a comment - Hello Andrew ! It works fine, It is a new great feature for Jspresso DSL ! Thank you for your help Maxime
          Maxime HAMM made changes -
          Attachment jspresso.zip [ 54621 ]
          Hide
          Maxime HAMM added a comment -

          Here is the Jspresso implementation for the outline feature
          Hope this will help some other Groovy Eclipse developpers
          Maxime
          www.jspresso.org

          Show
          Maxime HAMM added a comment - Here is the Jspresso implementation for the outline feature Hope this will help some other Groovy Eclipse developpers Maxime www.jspresso.org
          Maxime HAMM made changes -
          Attachment Jspresso outline.zip [ 54717 ]
          Andrew Eisenberg made changes -
          Link This issue is depended upon by GRECLIPSE-626 [ GRECLIPSE-626 ]

            People

            • Assignee:
              Andrew Eisenberg
              Reporter:
              Andrew Eisenberg
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: