Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated patch to allow an ExternalGraphic to be used instead of the GetLegendGraphic service in the WMS GetCapabilities LegendURL #400

Closed
wants to merge 1 commit into from

Conversation

mbenowit
Copy link

This is a replacement for pull request #349.

…etLegendGraphic service in the WMS GetCapabilities LegendURL
@mbenowit
Copy link
Author

Hopefully this one merges more cleanly. And I'm not sure I fully understand the second issue Andrea mentioned on #349, but does #397 help?

@aaime
Copy link
Member

aaime commented Nov 17, 2013

Mike, it's similar, but not same. Mauro some time ago implemented this extension to GeoServer that allows it to get either a layer group, or a comma separate list of layers, in GetLegendGraphic, which ends up building a full legend with titles. The new graphic option should be taken into account in the rendered image legend graphic builder, instead of using the SLD to generate the legend.

@aaime
Copy link
Member

aaime commented Nov 23, 2013

Hi, I had another look at the patch and tried it out a bit (the original objective was to try and complete GetLegendGraphics multilayer code).
However trying it out I found a couple of things that concern me, and started a discussion on geoserver-devel:
http://osgeo-org.1560.x6.nabble.com/Style-with-pre-rendered-legend-td5090799.html

I invite you to participate to the discussion, if possible.

@jodygarnett
Copy link
Member

Just to summarise the email discussion.

Pull request looks okay (allows use of an externally served image).

Discussion highlighted some reservations about this approach... the use of an externally served image (while technically correct) is not quite what is expected and disrupt the code for multi-layer legend.

Actions:

  • Add an "upload" option to allow image to be placed in styles directory
  • Use relative URL to reference file in styles directory
  • If an external URL is used we will need to validate on input to ensure GeoServer can see the resulting image

Question: Do you have a requirement to reference an external image?

@emiliom
Copy link

emiliom commented Mar 7, 2014

Apologies for parachuting into this Issue, but Google got me here. I have this exact need. Relative to the last comments:

  • In my case, the ability to handle an external URL (external image) would be nice, but not a priority.
  • A UI widget to upload an image is also nice, but also not the top priority.

First and foremost, I need to be able to specify a static image to replace the dynamic GetLegendGraphic. If that means manually (via the shell) copying the image files directly into the styles directory in the data directory, that's a small price to pay compared to the broader ability of specifying my own legend graphic. I'd rather see that basic capability rolled out soon, first, than wait for the extra niceties to be added before the basic capability is available.

Thank you!

@jodygarnett
Copy link
Member

Thanks Emilio. In this case conversation has stalled out as the original
patch got us 50% of the way there. I would be happy to accept it if there
was a commitment to do the next bit.

As it is we were hoping to talk to the patch authors and plan accordingly.
If you are interest in picking up this one I encourage you to give it a go.

On Saturday, March 8, 2014, Emilio Mayorga notifications@github.com wrote:

Apologies for parachuting into this Issue, but Google got me here. I have
this exact need. Relative to the last comments:

  • In my case, the ability to handle an external URL (external image)
    would be nice, but not a priority.
  • A UI widget to upload an image is also nice, but also not the top
    priority.

First and foremost, I need to be able to specify a static image to replace
the dynamic GetLegendGraphic. If that means manually (via the shell)
copying the image files directly into the styles directory in the data
directory, that's a small price to pay compared to the broader ability of
specifying my own legend graphic. I'd rather see that basic capability
rolled out soon, first, than wait for the extra niceties to be added before
the basic capability is available.

Thank you!

Reply to this email directly or view it on GitHubhttps://github.com//pull/400#issuecomment-37070197
.

Jody Garnett

@emiliom
Copy link

emiliom commented Mar 10, 2014

After reading the discussion threads across several github Issues, I thought it sounded like this was getting close to completion. Oh well. Unfortunately I can't help with Java development. Thanks for all the great work on GeoServer!

@jodygarnett
Copy link
Member

Yep, there is a gap between having a working implementation and getting it
accepted in the next release :)

Jody Garnett

On Mon, Mar 10, 2014 at 4:42 PM, Emilio Mayorga notifications@github.comwrote:

After reading the discussion threads across several github Issues, I
thought it sounded like this was getting close to completion. Oh well.
Unfortunately I can't help with Java development. Thanks for all the great
work on GeoServer!

Reply to this email directly or view it on GitHubhttps://github.com//pull/400#issuecomment-37154822
.

@mike-es
Copy link
Contributor

mike-es commented Mar 14, 2014

Jody, I am taking over for mbenowit because he is no longer on the project for which this pull request was created (we work together).

To answer your question, we do not have a requirement to reference an external image. In fact, we are currently using this patch in our GeoServer and we put the images in the styles directory. Due to networking restrictions our production servers do not have internet access anyway.

Michael Romero

@jodygarnett
Copy link
Member

That is good, referring to an image in the style directory will be much
easier to manage (we will need to confirm that the items in the style
directory can be accessed by clients etc...). I wonder if we could ask for
some help figuring out how to do an image upload?

Do you think you could join me on geoserver-devel to discuss this - as we
will need some help to figure out a good approach.

Jody Garnett

On Sat, Mar 15, 2014 at 8:28 AM, Michael Romero notifications@github.comwrote:

Jody, I am taking over for mbenowit because he is no longer on the project
for which this pull request was created (we work together).

To answer your question, we do not have a requirement to reference an
external image. In fact, we are currently using this patch in our GeoServer
and we put the images in the styles directory. Due to networking
restrictions our production servers do not have internet access anyway.

Reply to this email directly or view it on GitHubhttps://github.com//pull/400#issuecomment-37698536
.

@aaime
Copy link
Member

aaime commented Mar 15, 2014

Hi, mind, the main reason why I did not merge the pull is that it will create inconsistent behavior for those asking legend graphics for layer groups, or calling directly GetLegendGraphics

@jodygarnett
Copy link
Member

Catching up with this @mike-es - The original #400 commit is a bit stale, since this is not my pull request I cannot push up the conflict resolution. I can start a new pull request if you like?

Looks like we need:

  1. An "upload" option to allow image to be placed in styles directory (for ExternalGraphics panel)
  2. Restrict Legend to relative URL to reference file in styles directory
  3. Integration with GetLegendGraphics that respects width/height settings, can either ignore the legend graphic or "cookie cut" out a square from the top left corner and rescale to the requested size.
  4. Integration with GetLegendGraphics for layer groups

See: https://github.com/jodygarnett/geoserver/tree/legend-capabilities

@jodygarnett
Copy link
Member

So I am reviewing this code with an eye towards updating BufferedImageLegendGraphicBuilder and I am struck by something.

If the goal is simply to allow the use of a user supplied image we would be better served by using this LegendInfo value (or even the SE 1.1 GraphicLegend) as part of the GetLegendGraphic response, and maintain the GetCapabilities document the way it is.

As it is the LegendCapabilitiesTest references:

We have the option of:

Or maintaining:

@jodygarnett
Copy link
Member

I have a rebase of the above patch, and an initial integration with GetLegendGraphics:

https://github.com/jodygarnett/geoserver/commits/legend-capabilities

Things to do:

  • Update ExternalGraphicPanel to only allow graphics relative to the styles directory
  • figure out how http://localhost:8080/styles/legend.png can be accessed during a test case (works fine from a running GeoServer)
  • replace GetLegendGraphicRequest set( Name, LegendInfo ) with set( String, LegendInfo ) - to save based on style name for layer group use
  • update user docs

To test the above branch:

  1. From the styles page edit the "grass" style to have grass_fill.png
  2. Click Auto-detect image size and type
  3. Test WMS GetCapabilities links
  4. Use Demo Request page to test use (no rescale)

http://localhost:8080/geoserver/wms?REQUEST=GetLegendGraphic&VERSION=1.0.0&FORMAT=image/png&WIDTH=32&HEIGHT=32&LAYER=tiger:poly_landmarks&STYLE=grass

  1. Use Demo Request page to test with rescale

http://localhost:8080/geoserver/wms?REQUEST=GetLegendGraphic&VERSION=1.0.0&FORMAT=image/png&WIDTH=22&HEIGHT=22&LAYER=tiger:poly_landmarks&STYLE=grass

grass_fill_legend

A couple things I have learned:

  • I initially went looking for LegendInfo as a property of LayerInfo, saving it in a hash map much like layer title ... too bad it was always null! Is there any point in adding a user interface for this one?
  • I wish we had a GraphicLegend associated with a NamedStyle would be easier to manage

@jodygarnett
Copy link
Member

Replaced with #551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants