Details

    • Type: Sub-task Sub-task
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 8.0-M1
    • Component/s: None
    • Labels:
      None

      Description

      Hey guys - I am going to try something. I am worried that I have been too pig headed about this. The real motivation for Layer was to make the constructors make sense - not to make Andrea's life difficult. I think with my object-oriented hat on, wanting each class to have a minimal responsibility, I have made things too difficult / risky.

      I am going to look at pulling the getStyle() and getFeatureSource() and getQuer() methods up into Layer (and having them return "empty" content). I can explain in the javadocs that these methods are used by feature based renderers; and the default implementation only needs to be overridden if your Layer has something to say.

      I would hope this would avoid the amazing amount of casts witnessed in Andrea's and Micheal's patches. I don't think it impacts on the clarity of the interfaces; the constructors are still clear; we still have concrete implementations each of which has their own specific "workarounds" for providing a FeatureSource - and since it is not tired up in conditional code I hope it would be readable.

      1. geot-3658.patch
        17 kB
        Jody Garnett
      2. layer_defaults.patch
        7 kB
        Jody Garnett

        Issue Links

          Activity

          Hide
          Jody Garnett added a comment -
          Okay the result was clean; and allowed me to clean up the conditional code in MapLayer (so double win). I will attach a patch as soon as I figure out how to stop arguing with git.

          Andrea I hope this goes a long ways towards cutting down on the risk associated with this transition.
          Show
          Jody Garnett added a comment - Okay the result was clean; and allowed me to clean up the conditional code in MapLayer (so double win). I will attach a patch as soon as I figure out how to stop arguing with git. Andrea I hope this goes a long ways towards cutting down on the risk associated with this transition.
          Hide
          Jody Garnett added a comment -
          We may be able to remove the StyleLayer and RasterLayer interfaces after this change (as they were both half measures to address this issue).
          Show
          Jody Garnett added a comment - We may be able to remove the StyleLayer and RasterLayer interfaces after this change (as they were both half measures to address this issue).
          Hide
          Jody Garnett added a comment -
          Patch adds support for:

          * Layer.getStyle()
          * Layer.getQuery()
          * Layer.getFeatureSource()

          With associated implementation in the subclasses; and cleanup in MapLayer to avoid duplication.
          Show
          Jody Garnett added a comment - Patch adds support for: * Layer.getStyle() * Layer.getQuery() * Layer.getFeatureSource() With associated implementation in the subclasses; and cleanup in MapLayer to avoid duplication.
          Hide
          Andrea Aime added a comment - - edited
          Scratch scratch... this basically means I have to ditch that patch and start over, as most of the work was actually the casts, and I guess replace it with null checks instead...
          Show
          Andrea Aime added a comment - - edited Scratch scratch... this basically means I have to ditch that patch and start over, as most of the work was actually the casts, and I guess replace it with null checks instead...
          Hide
          Jody Garnett added a comment -
          Note the old MapLayer code did return null (the patch here preserves exactly the MapLayer functionality).

          Caught up with Andrea on IRC:

          1) I am going to try and return null objects so that null checks are not needed; the only one I am worried about is an "Empty" FeatureSource - I will need to make an place holder feature type of some sort.
          2) I am going to look into Andrea's GeoServer patch and see if it is faster to apply it; and then remove the casts (as they can at least be searched for easily).
          Show
          Jody Garnett added a comment - Note the old MapLayer code did return null (the patch here preserves exactly the MapLayer functionality). Caught up with Andrea on IRC: 1) I am going to try and return null objects so that null checks are not needed; the only one I am worried about is an "Empty" FeatureSource - I will need to make an place holder feature type of some sort. 2) I am going to look into Andrea's GeoServer patch and see if it is faster to apply it; and then remove the casts (as they can at least be searched for easily).
          Hide
          Jody Garnett added a comment -
          The above patch updates layer so it does not return null for anything; while geotools builds I have not been able to get geoserver tests to pass using this patch.
          Show
          Jody Garnett added a comment - The above patch updates layer so it does not return null for anything; while geotools builds I have not been able to get geoserver tests to pass using this patch.
          Hide
          Jody Garnett added a comment - - edited
          Scratch that the geoserver tests are failing due to :

          java.lang.AbstractMethodError: org.apache.xerces.dom.DocumentImpl.getXmlStandalone()Z
          at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.setDocumentInfo(DOM2TO.java:373)
          at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.parse(DOM2TO.java:127)
          at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.parse(DOM2TO.java:94)
          at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transformIdentity(TransformerImpl.java:661)
          at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:707)
          at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:313)
          at org.geoserver.data.CatalogWriter.write(CatalogWriter.java:253)
          Show
          Jody Garnett added a comment - - edited Scratch that the geoserver tests are failing due to : java.lang.AbstractMethodError: org.apache.xerces.dom.DocumentImpl.getXmlStandalone()Z at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.setDocumentInfo(DOM2TO.java:373) at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.parse(DOM2TO.java:127) at com.sun.org.apache.xalan.internal.xsltc.trax.DOM2TO.parse(DOM2TO.java:94) at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transformIdentity(TransformerImpl.java:661) at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:707) at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:313) at org.geoserver.data.CatalogWriter.write(CatalogWriter.java:253)
          Hide
          Andrea Aime added a comment -
          Jody, you should not be building with java 6
          Show
          Andrea Aime added a comment - Jody, you should not be building with java 6
          Hide
          Andrea Aime added a comment -
          Oh btw, to avoid that error xerces has to be removed from the geoserver classpath
          Show
          Andrea Aime added a comment - Oh btw, to avoid that error xerces has to be removed from the geoserver classpath

            People

            • Assignee:
              Jody Garnett
              Reporter:
              Jody Garnett
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: