GeoServer
  1. GeoServer
  2. GEOS-654

Register GetMapProducers directly and remove the factories

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.7.0-RC1
    • Fix Version/s: 1.7.0-RC2
    • Component/s: WMS
    • Labels:
      None
    • Number of attachments :
      0

      Description

      For the moment we do have GetMapProducerSpi and GetMapProducer, and we do register the factories onto the context.
      I suggest to remove the factories and register directly the GetMapProducer, since it's simpler.

      For producers that are not thread safe, just register them as prototypes.
      For producers that do need external configuration to decide on a code path (like svg), just add a delegate producers that performs the decision.

        Activity

        Hide
        Gabriel Roldan added a comment - - edited

        Agreed there should be more unit tests. That gets clearer by the fact there wasn't a previous test for that format options or it would have failed after the changes.

        Show
        Gabriel Roldan added a comment - - edited Agreed there should be more unit tests. That gets clearer by the fact there wasn't a previous test for that format options or it would have failed after the changes.
        Hide
        Gabriel Roldan added a comment -

        So, by Andrea's comment, it seems the kml output formats could be expressed in various ways? are there a limited set of them so we can use aliases and advertise in the capabilities? or should I find a way to make a sloppy matching without the factory's canProduce() method?

        Show
        Gabriel Roldan added a comment - So, by Andrea's comment, it seems the kml output formats could be expressed in various ways? are there a limited set of them so we can use aliases and advertise in the capabilities? or should I find a way to make a sloppy matching without the factory's canProduce() method?
        Hide
        Gabriel Roldan added a comment - - edited

        Worked on this and I think I have a fair solution.
        This would be the deal:

        interface GetMapProducer{
          
          /** the actual content type for the response header, does not need to match the requested output format */
          String getContentType();
          
          /** the output format names this producer can be invoked by, and the ones to state in capabilities */
          List<String>getOutputFormatNames();
        
          /** shall be called before produceMap with the actual output format the request was made with. Some producers depend on it */
          void setOutputFormat(String outputFormat);
          
          /** the output format the request was made with */
          String getOutputFormat();
        
          void produceMap();
          void setMapContext(WMSMapContext context);
          ....
        }
        

        This way we decouple the mime type to be written as a response header and what we state in getcaps, better separation of concerns than having a mixin of them.
        Also, this is so since some producers does completelly different one thing and the other. for example, the openlayers one states the format application/openlayers, but the mime type it sends is text/html.
        Others (png, tiff, geotiff) makes a decision based on the requested output format, producing a different result based on it (png vs png8, tiff vs tiff8, etc), but the output mime type shall be the same "image/png"
        That is certainly awful, like making a code path fork depending on the output format requested seems to imply those should be different MapProducers... we could fix than later, but for now this is the proposal to avoid regressions

        comments?

        Show
        Gabriel Roldan added a comment - - edited Worked on this and I think I have a fair solution. This would be the deal: interface GetMapProducer{ /** the actual content type for the response header, does not need to match the requested output format */ String getContentType(); /** the output format names this producer can be invoked by, and the ones to state in capabilities */ List< String >getOutputFormatNames(); /** shall be called before produceMap with the actual output format the request was made with. Some producers depend on it */ void setOutputFormat( String outputFormat); /** the output format the request was made with */ String getOutputFormat(); void produceMap(); void setMapContext(WMSMapContext context); .... } This way we decouple the mime type to be written as a response header and what we state in getcaps, better separation of concerns than having a mixin of them. Also, this is so since some producers does completelly different one thing and the other. for example, the openlayers one states the format application/openlayers, but the mime type it sends is text/html. Others (png, tiff, geotiff) makes a decision based on the requested output format, producing a different result based on it (png vs png8, tiff vs tiff8, etc), but the output mime type shall be the same "image/png" That is certainly awful, like making a code path fork depending on the output format requested seems to imply those should be different MapProducers... we could fix than later, but for now this is the proposal to avoid regressions comments?
        Hide
        Gabriel Roldan added a comment -

        marking as resolved so anyone that notices and wants to test for a bit can do it

        Show
        Gabriel Roldan added a comment - marking as resolved so anyone that notices and wants to test for a bit can do it
        Hide
        Andrea Aime added a comment -

        All these issues have been in "resolved" state for at least one month (many of them, much more than that). Since no one has reopened them, I'm mass-switching them to closed state. Reopen if you feel the issue has not been addressed properly.

        Show
        Andrea Aime added a comment - All these issues have been in "resolved" state for at least one month (many of them, much more than that). Since no one has reopened them, I'm mass-switching them to closed state. Reopen if you feel the issue has not been addressed properly.

          People

          • Assignee:
            Gabriel Roldan
            Reporter:
            Andrea Aime
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1 day
              1d
              Remaining:
              Remaining Estimate - 1 day
              1d
              Logged:
              Time Spent - Not Specified
              Not Specified