Ticket #157 (closed defect: fixed)

Opened 11 months ago

Last modified 10 months ago

WMSCapabilitiesReader does not take into account its fields configuration

Reported by: bbinet Assigned to: bbinet
Priority: major Milestone: 0.6
Component: GeoExt.data.WMSCapabilitiesReader Version: trunk
Keywords: Cc:
State: Commit

Description

When reading a WMSCap document, this reader put everything in LayerRecord and does not care about the configured fields.

We should copy only fields defined in the recordType.

Patch to come soon.

Attachments

patch-157_r1358-A0.diff (5.0 kB) - added by bbinet on 09/11/09 15:41:46.
geoext-157.patch (7.7 kB) - added by ahocevar on 10/09/09 16:42:53.
applies to current trunk, includes patch for #162
157.patch (7.6 kB) - added by tschaub on 10/09/09 17:33:25.
wms caps reader modifications
patch-157_r1409-A3.diff (7.2 kB) - added by bbinet on 10/10/09 00:37:25.

Change History

09/11/09 15:41:46 changed by bbinet

  • attachment patch-157_r1358-A0.diff added.

09/11/09 15:50:19 changed by bbinet

  • state changed from None to Review.

patch-157_r1358-A0.diff is fixing the issue and adding tests to check that the reader is not copying fields in the LayerRecord that should not be copied.

This patch is also removing "name" from default fields since it is already available in LayerRecord as the "title" field (see LayerRecord class).

Tests pass on FF3.

09/29/09 15:59:30 changed by elemoine

It looks like #126 is first ine the commit pipe, so the patch attached to this ticket will need modifications.

10/08/09 17:27:13 changed by ahocevar

  • owner set to ahocevar.

10/08/09 22:34:34 changed by ahocevar

  • state changed from Review to Needs more work.

I am working on an updated patch now.

10/09/09 13:42:36 changed by ahocevar

The new patch will depend on #160

10/09/09 16:42:53 changed by ahocevar

  • attachment geoext-157.patch added.

applies to current trunk, includes patch for #162

10/09/09 16:50:38 changed by ahocevar

  • state changed from Needs more work to Review.

New patch reflects the intention of the original patch author, but the default record type has to include the title. Reason: In the default layer record, title is mapped to name (which makes sense for a LayerStore that does not know more about a layer than the OL.Layer's name property). If we have a more verbose title from a capabilities doc, we do not want this mapping, but have a less verbose name and a more verbose title. By adding the title back to the record definition, we remove the default mapping of title to name.

Please review. Tests pass in FF3.

I won't be online for much longer today, and have no internet acces this weekend. So if this patch is required before Sunday night (CEST), please feel free to commit on my behalf.

10/09/09 17:33:25 changed by tschaub

  • attachment 157.patch added.

wms caps reader modifications

10/09/09 17:35:57 changed by tschaub

  • owner changed from ahocevar to tschaub.
  • status changed from new to assigned.

Reviewing now.

10/09/09 18:42:57 changed by tschaub

  • owner changed from tschaub to bbinet.
  • status changed from assigned to new.
  • state changed from Review to Commit.

This looks good. Marking for commit and assigning to bbinet. If you don't get to it, I will.

10/10/09 00:05:40 changed by bbinet

  • state changed from Commit to Needs more work.

Why the two last patches are reverting changes from #150 ?

10/10/09 00:37:25 changed by bbinet

  • attachment patch-157_r1409-A3.diff added.

10/10/09 00:41:49 changed by bbinet

  • state changed from Needs more work to Review.

patch-157_r1409-A3.diff is an update of latest patch to preserve changes from #150. If it works for you, I'll commit it soon.

Tests still pass on FF3.

10/12/09 01:21:01 changed by tschaub

  • state changed from Review to Commit.

Yeah, I see that was a problem. I don't have time to check this out thoroughly, but I think you should commit. If you don't get to it, I'll give it a closer review later.

10/12/09 10:05:26 changed by bbinet

  • status changed from new to closed.
  • resolution set to fixed.

(In [1412]) WMSCapabilitiesReader does not take into account its fields configuration. r=ahocevar,tschaub (closes #157)

10/12/09 13:06:21 changed by ahocevar

Just a note to say that reverting #150 with my patch was an accident. I was not sure about the current state of the id discussion, and was not aware that this change came from #150. So to add that back to the patch was the right thing to do, and what was committed with r1412 is exactly what it should look like. Thanks bbinet and tschaub, and sorry for the confusion.

(follow-up: ↓ 15 ) 10/12/09 13:48:51 changed by ahocevar

(In [1416]) re-added test for correct id that was introduced with #150 and accidently removed with #157 (see #157)

(in reply to: ↑ 14 ) 10/12/09 14:07:56 changed by bbinet

Replying to ahocevar:

(In [1416]) re-added test for correct id that was introduced with #150 and accidently removed with #157 (see #157)

Sorry, but I think r1412 has not accidently removed any test. The test that checks for correct id that was introduced with #150 was still there in r1412.

10/12/09 15:20:56 changed by ahocevar

bbinet: sorry, you are absolutely right. reverted r1416 with r1417.