Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
Hi All,

I've attached a patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

This fixes up the LegendPlotUtil class to handle the following cases:

 - The ShowInLegend setting of a Feature Type Style. Previously the code did not check this setting and always assumed visibility.
 - Whether a scale range has multiple Feature Type Style instances. Previously the code would always grab the first one regardless and render the appropriate icons off of that instance.

Please review. Thanks.

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
I found a defect in this patch. I'll post a second patch up for review shortly.

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
I've attached a revised patch for review (http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v2.patch)

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

weltonw
Hi Jackie,

Two things I noticed:

1/ Imagine you have a FeatureTypeStyleCollection with a themed PointTypeStyle and an unthemed CompositeTypeStyle.  The peek-ahead code gives the CompositeTypeStyle precedence, so nTotalRules is 1 and we just render the unthemed CompositeTypeStyle.  Now change it so you have a themed PointTypeStyle and a themed CompositeTypeStyle.  We have nTotalRules > 1, so we enter the 'layer is themed' section.  It iterates over the FeatureTypeStyleCollection and renders all rules for ALL FeatureTypeStyles.  So in the first example the legend reflects that the CompositeTypeStyle has precedence, but in the second example it doesn't.  Should it?

2/ There's a check for y < bottomLimit inside the Rule loop.  If true we break out of the loop, and then continue processing the next FeatureTypeStyle.  The y < bottomLimit check will be hit again, so no more rendering happens (as required).  We could add another y < bottomLimit check after the Rule loop and immediately break out of the FeatureTypeStyle loop.

Walt

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Jackie Ng
Sent: Thursday, November 14, 2013 4:57 AM
To: [hidden email]
Subject: Re: [mapguide-internals] Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

I've attached a revised patch for review
(http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v2.patch)

- Jackie



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/Patch-for-ticket-2379-GETMAPLEGENDIMAGE-does-not-handle-ShowInLegend-style-setting-and-multi-style-s-tp5089060p5089090.html
Sent from the MapGuide Internals mailing list archive at Nabble.com.
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
Hi Walt,

I've added a third version of the patch for review (http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v3.patch)

It adds an extra boolean variable to determine if a composite style was found in the peek-ahead code, and uses this variable to skip over any non-composite type styles in the themed code path.

And also includes the extra loop break after the rule loop as you suggested.

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

weltonw
I see one more issue.  If you have multiple CompositeTypeStyles in the collection and the first is unthemed, then the code will only populate the legend with this first one.  In the peek-ahead code, as soon as it finds the first CompositeTypeStyle it sets nRuleCount based on that FTS and breaks out of the loop.  So in this example bThemed ends up false.

Your updated code handles the case of multiple non-CompositeTypeStyles correctly.  It needs the same behavior for multiple CompositeTypeStyles.

Walt

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Jackie Ng
Sent: Thursday, November 14, 2013 10:49 PM
To: [hidden email]
Subject: Re: [mapguide-internals] Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Hi Walt,

I've added a third version of the patch for review
(http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v3.patch)

It adds an extra boolean variable to determine if a composite style was found in the peek-ahead code, and uses this variable to skip over any non-composite type styles in the themed code path.

And also includes the extra loop break after the rule loop as you suggested.

- Jackie



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/Patch-for-ticket-2379-GETMAPLEGENDIMAGE-does-not-handle-ShowInLegend-style-setting-and-multi-style-s-tp5089060p5089262.html
Sent from the MapGuide Internals mailing list archive at Nabble.com.
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
Hi Walt,

4th version of the patch posted (http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v4.patch)

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

weltonw
It's still not 100% right...

With patch 4, if the collection contains a single unthemed CompositeTypeStyle and some other non-CompositeTypeStyles, then nTotalRules will be > 1 and bThemed will be set to true.  The legend will end up displaying the theme icon and not the icon from the single rule in the CompositeTypeStyle.

The peek-ahead code needs to compute both the total # of composite rules and the total # of non-composite rules.  If there are composite rules, then these take precedence and determine if the legend shows the themed or unthemed icon.  Otherwise the # of non-composite rules determines the icon.

Also, singleFTS needs to be set correctly when the collection has either a single CompositeTypeStyle rule or a single non-CompositeTypeStyle rule.  Currently singleFTS always ends up being set to the last FTS in the collection.  If the collection has just a single CompositeTypeStyle rule, then that type style isn't necessarily the last one.

Walt

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Jackie Ng
Sent: Sunday, November 17, 2013 10:09 PM
To: [hidden email]
Subject: Re: [mapguide-internals] Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Hi Walt,

4th version of the patch posted
(http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v4.patch)

- Jackie



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/Patch-for-ticket-2379-GETMAPLEGENDIMAGE-does-not-handle-ShowInLegend-style-setting-and-multi-style-s-tp5089060p5089580.html
Sent from the MapGuide Internals mailing list archive at Nabble.com.
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Jackie Ng
Ok, 5th version of the patch is up, with unit tests to cover the cases discussed

http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v5.patch

- Jackie
Reply | Threaded
Open this post in threaded view
|

Re: Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

weltonw
Looks good to me now.  Thanks for taking care of the issues.

Walt

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Jackie Ng
Sent: Thursday, November 21, 2013 6:56 AM
To: [hidden email]
Subject: Re: [mapguide-internals] Patch for ticket 2379 (GETMAPLEGENDIMAGE does not handle ShowInLegend style setting and multi-style scale ranges properly)

Ok, 5th version of the patch is up, with unit tests to cover the cases discussed

http://trac.osgeo.org/mapguide/attachment/ticket/2379/2379_v5.patch

- Jackie



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/Patch-for-ticket-2379-GETMAPLEGENDIMAGE-does-not-handle-ShowInLegend-style-setting-and-multi-style-s-tp5089060p5090429.html
Sent from the MapGuide Internals mailing list archive at Nabble.com.
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
_______________________________________________
mapguide-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/mapguide-internals