Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#153 closed enhancement (fixed)

Make LegendPanel more generic to provide a common interface for different legend types

Reported by: bartvde Owned by: bbinet
Priority: major Milestone: 0.7
Component: GeoExt.LegendPanel Version: trunk
Keywords: Cc:
State: Commit

Description (last modified by ahocevar)

This came up in:

https://trac.mapfish.org/trac/mapfish/ticket/511#comment:5

After some discussion, it turns out that making the LegendPanel more generic, with a LayerLegend base class that legend implementations can extend, gives us easier access to the legend for free.

Encoding legends for printing can then be handled easily by an encoder structure (see http://trac.geoext.org/browser/sandbox/ahocevar/playground/ux/Printing/ux/data/PrintProvider.js?rev=1596#L470)

Attachments (8)

patch-153_r1483-A0.diff (4.6 KB) - added by bbinet 7 years ago.
patch-153_r1484-A1.diff (6.7 KB) - added by bbinet 7 years ago.
patch-153_r1486-A3.diff (4.8 KB) - added by bbinet 7 years ago.
patch-153_r1524_B0.diff (7.9 KB) - added by bbinet 7 years ago.
patch-153_r1524_B1.diff (8.8 KB) - added by bbinet 7 years ago.
geoext-153.patch (21.0 KB) - added by ahocevar 7 years ago.
Shows a way to have different LayerLegend classes supporting different layer types or layerRecord contents.
geoext-153.2.patch (29.7 KB) - added by ahocevar 7 years ago.
same as above, but with more appropriate class names and type negotiation moved from LegendPanel to LayerLegend
geoext-153.3.patch (57.7 KB) - added by ahocevar 7 years ago.
with tests

Download all attachments as: .zip

Change History (31)

comment:1 Changed 8 years ago by sbenthall

  • Milestone changed from 0.6 to 0.7

Changed 7 years ago by bbinet

comment:2 Changed 7 years ago by bbinet

  • State changed from None to Review

patch-153_r1483-A0.diff adds an interface to LegendPanel with the getLegend method, in order to get all the legend image urls from the LegendPanel.

I'm not sure about the name of this method "getLegend". Do you have a better idea? maybe "export"? How are such methods called in Ext?

Tests are included and pass in FF3.

comment:3 Changed 7 years ago by bbinet

  • Owner set to bbinet
  • State changed from Review to Needs more work
  • Status changed from new to assigned
  • missing test on items existence
  • remove test on components visibility since we could ask for LegendPanel getLegend even if hided

I will update the patch as soon as possible.

Changed 7 years ago by bbinet

comment:4 Changed 7 years ago by bbinet

patch-153_r1484-A1.diff is resolving the 2 issues mentioned in previous comment.

The patch also embed patch from #178 since it was needed to call layerstore.removeAll() in the tests.

This ticket still needs more work, according to the discussion with Tim (see the corresponding thread on dev mailing list). And on the other hand, even if the solution proposed with this patch is approved, then the patch is still incomplete because of the following reason:

if LegendPanel has not been rendered, then the panel does not contains any legend items yet, and the call to getLegend interface method will return an empty result. This means that we should refactor LegendPanel and put all the stuff related to legend items initialization in a bind method that could be called either on component rendering or by the getLegend interface method.

I'll wait to see how the discussion evolves with Tim before update the patch with this refactoring.

Changed 7 years ago by bbinet

comment:5 Changed 7 years ago by bbinet

patch-153_r1486-A3.diff is a simple update of previous patch that removes dependency on #178 which has been committed.

Changed 7 years ago by bbinet

comment:6 Changed 7 years ago by bbinet

  • State changed from Needs more work to Review

patch-153_r1524_B0.diff adds bind/unbind methods to LegendPanel, so that the LegendPanel can be bound with LayerStore, either from the onRender method or from the getLegend method which also need the legend components to be created (as explained in my previous comment).

Tests pass on FF3 and Chrome.

A review is welcome.

comment:7 Changed 7 years ago by bbinet

  • State changed from Review to Needs more work

Still needs more work: we should take in account the layer visibility.

Changed 7 years ago by bbinet

comment:8 Changed 7 years ago by bbinet

  • State changed from Needs more work to Review

patch-153_r1524_B1.diff fix the bug.

Now, if a layer visibility is set to false, then the getLegend interface method does not return the corresponding legend block anymore.

Specific tests are included to check this case. Tests pass on FF3, and chromium.

comment:9 follow-up: Changed 7 years ago by tschaub

Hey. Sorry to not have any time for this right now. Can you please update the ticket description if it is not an accurate representation of what the patch does (haven't looked closely).

If what you are proposing is actually a method to "get all the legend image urls from the LegendPanel," then I am opposed (with apologies). I think you are probably proposing something different, but I wanted to make sure this didn't get in without outside review if that is really what is being added.

I'll try to make some time for this in the next few days. And again, apologies if this is irrelevent - I've only been glancing at the mail and ticket descriptions.

comment:10 in reply to: ↑ 9 Changed 7 years ago by bbinet

Replying to tschaub:

Hey. Sorry to not have any time for this right now. Can you please update the ticket description if it is not an accurate representation of what the patch does (haven't looked closely).

Hey. I don't know how to update the ticket description, I may not have permission to do so.

So here would be the new description: Provide an easy interface to get a javascript representation object of all legend blocks from the LegendPanel.

If what you are proposing is actually a method to "get all the legend image urls from the LegendPanel," then I am opposed (with apologies). I think you are probably proposing something different, but I wanted to make sure this didn't get in without outside review if that is really what is being added.

My proposition was to get a js object representation of the legend panel element, as opposed as a list of urls. As LegendPanel currently supports only LegendImage/LegendWMS, then the generated object is composed of "text" label and "legend" object which are currently string url, but could be fill with an arbitrary js object by the LegendVector or whatever other class.

I'll try to make some time for this in the next few days. And again, apologies if this is irrelevent - I've only been glancing at the mail and ticket descriptions.

I hope this clarifies things. You're encouraged to take a look at the code.

Changed 7 years ago by ahocevar

Shows a way to have different LayerLegend classes supporting different layer types or layerRecord contents.

comment:11 Changed 7 years ago by ahocevar

  • State changed from Review to Needs Discussion

@tschaub: does attachment:geoext-153.patch implement what you were envisioning? It uses LegendImage.js just as a generic component to render an image, adds a LayerLegend.js base class with a static supports() method and a types array. LegendWMS.js was modified to provide an implementation working for any WMS layer, and LegendURL.js provides an implementation supporting layer records that have a legendURL field.

comment:12 Changed 7 years ago by ahocevar

@bbinet: with r1585, I have added encoders for legends to the printing ux. As you can see, this is quite simple with the proposed structure, because every legend type has a known structure:

            options.legend.items.each(function(cmp) {
                var encFn = GeoExt.ux.data.PrintProvider.encode.legend[cmp.getXType()];
                legends = legends.concat(encFn(cmp));
            }, this);
GeoExt.ux.data.PrintProvider.encode = {
    legend: {
        "gx_legendwms": function(legend) {
            return GeoExt.ux.data.PrintProvider.encode.legend.base(legend);
        },
        "gx_legendurl": function(legend) {
            return GeoExt.ux.data.PrintProvider.encode.legend.base(legend);
        },
        "base": function(legend) {
            var enc = [];
            legend.items.each(function(cmp) {
                if(cmp instanceof Ext.form.Label) {
                    enc.push({name: cmp.text, classes: []});
                } else if(cmp instanceof GeoExt.LegendImage) {
                    enc.push({
                        name: "",
                        icon: Ext.DomHelper.overwrite(
                            document.createElement("a"), {
                                tag: "a",
                                href: cmp.url
                            }).href,
                        classes: []});
                }
            }, this);
            return enc;
        }
    }

Changed 7 years ago by ahocevar

same as above, but with more appropriate class names and type negotiation moved from LegendPanel to LayerLegend

comment:13 Changed 7 years ago by ahocevar

  • Description modified (diff)
  • Summary changed from provide interface on LegendPanel to Make LegendPanel more generic to provide a common interface for different legend types

Changed 7 years ago by ahocevar

with tests

comment:14 Changed 7 years ago by ahocevar

  • State changed from Needs Discussion to Review

attachment:geoext-153.3.patch implements the proposed changes. A LayerLegend base class can be extended by more specific legend renderers. With the preferredTypes config option of the LegendPanel, application developers can influence the renderer that will be chosen.

Tests pass in FF3.5 and IE7. Please review.

comment:15 Changed 7 years ago by ahocevar

Instructions to apply attachment:geoext-153.3.patch:

Before applying the patch, use svn rename to rename two files:

$ svn rename tests/lib/GeoExt/widgets/LegendWMS.html tests/lib/GeoExt/widgets/WMSLegend.html
$ svn rename lib/GeoExt/widgets/LegendWMS.js lib/GeoExt/widgets/WMSLegend.js

Now apply the patch. Answer all questions with "n".

comment:16 Changed 7 years ago by bartvde

Hi Andreas, WMSLegend is missing a requires tag for LayerLegend:

@requires GeoExt/widgets/LayerLegend.js

This also applies to GeoExt.UrlLegend.

Also, did something break wrt legendOptions?

  {
    title: OpenLayers.i18n("TOCTabLegendTitle"),
    id: 'legend',
    autoScroll: true,
    xtype: 'gx_legendpanel',
    legendOptions: {
      imageFormat: 'image/png'
    },
    ascending: false,
    labelCls: 'startwmc',
    bodyStyle: 'padding-top: 10px;',
    cls: 'popup-variant1'
  }

I am getting gif again instead of png.

I'll look at the code more closely now after getting the build to work.

comment:17 Changed 7 years ago by ahocevar

Bart: I removed legendOptions, because you can now use defaults to give options to the legends created by the legend panel.

Look at the example also. Instead of legendOptions, you should now configure

    defaults: {
        imageFormat: 'image/png'
    }

I will add the missing @requires and upload a new patch.

comment:18 Changed 7 years ago by bartvde

Hi Andreas, thanks for the pointer, defaults works great. Another change, what happened to labelCls? How to do that now?

comment:19 Changed 7 years ago by bartvde

Okay the example explains this change as well:

    legendPanel = new GeoExt.LegendPanel({
        defaults: {
            labelCls: 'mylabel',
            style: 'padding:5px'
        },

sorry for the noise.

comment:20 Changed 7 years ago by bartvde

  • State changed from Review to Commit

Hi Andreas, excellent patch, I've only got a few minor comments. If they are resolved you can commit (at least for the second and third item, the first item is optional and I'll leave you to decide on that change):

  • LegendPanel: rename layerStore to layers if we are changing things anyway?
  • / api: xtype = gx_UrlLegend */ should be all lowercase (xtypes are case sensitive)
  • LayerLegend: base_link not correct should be container and not boxcomponent

comment:21 Changed 7 years ago by ahocevar

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [1605]) made LegendPanel extensible by LegendTypes, and added two LegendTypes. r=bartvde (closes #153)

comment:22 Changed 7 years ago by ahocevar

Bart, thanks for the review. I did not rename layerStore to layers. My opinion is that if you could provide either an array of OpenLayers.Layer or a LayerStore, the name "layers" would make sense. But we require a LayerStore here, so the name is good.

At some point before 1.0, we will want to review the API and take care of such things to make them consistent across the library.

comment:23 Changed 7 years ago by ahocevar

Also see r1607 for improvements to the legendpanel example.

Note: See TracTickets for help on using tickets.