[MapScript] Expose metadata as a proper object

[dmorissette]

In bug 734 Sean write:

This is an issue much on my mind lately.  See:

  http://mapserver.gis.umn.edu/cgi-bin/wiki.pl?RefactoringMetadataObject

We could avoid implementing metadata separately for Map, Layer, and Class.

[dmorissette]

I agree with your idea in principle, but I would call the new object hasTableObj
instead of StringDictionary, just because that's the name we're used to:

   typedef struct {
       hashObj **items;
   } hashTableObj;

That's reusing an existing name, but since the type definition changes there is
no risk of side-effects since we'll have to revisit all code that uses this
datatype before we can compile anyway.

[sgillies@…]

I've started working on tests and will move on to code this aft, probably in
maphash.c.  I'll make very sure everything works before I commit changes.

As far as mapscript goes, I'm specifying these methods

  setItem(string key, string value)
  getValue(string key)
  removeItem(string key)
  nextKey(string prevkey)
  clear( )
  copy( ) : hashTableObj

What do you think?  Too verbose?  Does anybody prefer 

  set
  get
  remove
  next

?

I am also going to see about keys() and values() methods that would break
new ground for SWIG mapscript by returning sequences.

[sdlime]

For what it's worth I prefer the shorter method names. We've tried to do that 
with the other objects too...

[dmorissette]

There are pros and cons to both forms so either way is fine with me.

[sgillies@…]

OK, work on this is done.  Before I commit I want to explain what I've done
and why I think a new tag is called for.

The hashObjTable is now a structure.  The hash tables functions msLookupHashTable,
msInsertHashTable, msRemoveHashTable, msNextKeyFromHashTable, etc now take a 
hash table as primary argument by *reference*.  Otherwise they function the
same.

We are using hash tables everywhere in MapServer so my changes, while shallow,
are very broad.  If we had to roll them back it would be a drag to do it file 
by file -- therefore I am petitioning to tag my files before commiting with a
name that we won't confuse with any release tag.

I've left the getMetaData, setMetaData, removeMetaData, nextMetaDataKey 
methods of mapObj, layerObj, and classObj in the mapscript module. We can start 
planning on how and when to deprecate them.

Methods of the new hashTableObj (HashTable) class are: set, get, remove, clear,
and nextKey.  This covers existing functionality.

BTW, some recent commits to mapfile.c, map.h, and mapsymbol.c blocked me from
proceding because of segfaults.  I had to roll back 2 revisions on each of
these before I could get anything to run.  Should I commit my working code
now?  It may not conflict with the most recent stuff in CVS, but then again
it might.

[sgillies@…]

Daniel, about the names:

I am against short and vague method names like "set" and "get" everywhere
else in SWIG mapscript.  IMO, a method that returns an instance of Line
(for example) should be "getLine".  Makes the API self-explaining.

On the other hand, the new HashTable has such a limited set of responsibilities
that I think we can get away with it.  It is only a container for pairs of
strings so there is no ambiguity.  I think that users also expect very easy
and concise access to a hash table.  Without a lot of hacking we won't be 
able to make our metadata objects be PHP or Python dictionaries with

   layer.metadata['wms_srs'] = 'EPSG:4326'

access, but short methods like 

   layer.metadata.set('wms_srs', 'EPSG:4326') 

are the next best thing.

And then there is the issue that the PHP 4 mapscript module already reserves
"get" and "set" for object attributes ... do you think you'll change this for
the future PHP 5 module?

[fwarmerdam]

Sean,

In reference to you second last comment, my feeling is:
 o A tag isn't really needed.  Your changes are necessary and appropriate.  If
we run into problems we should just fix up the details rather than roll
everything back.

 o You should be integrating again the head, not a "last working" version.  If
the current code is crashing lets get it fixed before you do your big commit. 

[sgillies@…]

Check.  Will wait on the commit.

[sgillies@…]

OK, now that 715 is closed, I have committed changes.  My Python unit tests
pass and the following:

  valgrind ./shp2img -m tests/test.map -o test.png

produces no new errors or leaks.

[sgillies@…]

Committed changes to SWIG mapscript.

Took the opportunity to do some house cleaning on mapscript.i ...
it's getting too bulky.  From now on we'll be putting class
extensions in their own .i files.  See hashtable.i and
owsrequest.i.

[dmorissette]

Those changes have broken PHP MapScript. Fixing it now.  I'll also expose the
new hashTableObj at the same time, but maintain the old methods for backwards
compatibility (I'll just mark them deprecated).

[dmorissette]

I have committed a fix to mapscript/php3/mapscript_i.c so that things build fine
and I am in the process of porting hashtable.i to php_mapscript.

I noticed that the get() method returns a 'char*' and not a 'const char *'. Is
there any reason why it cannot return a 'const char *'?  Is this something
that's not supported by SWIG?

Oh, and about Sean's question in comment #6: I thought that it was _get() and
_set() that PHP 5 had reserved? I'll have to check again because if it's get()
and set() with no underscore then we may have to overload them or do something
special.  One nice thing about those _get and _set methods in PHP 5 is that they
will allow us to trap changes (assignments) to class member variables
automatically, and to get rid of the current get/set methods that we currently
have to use to set values on all objects in PHP MapScript.

[sgillies@…]

Daniel, 

1) sorry about neglecting the php module.  

2) SWIG doesn't discriminate between char * and const char *.  Each are
   converted to strings.

3) I was asking if for PHP 5 you would keep your existing 

     $oMap->set("labelitem", "foo");

   syntax for backwards compatibility or would switch to

     $oMap->labelitem = "foo";

   and free up 'set' and 'get' for other uses.  


When do you think we would deprecate the old methods?  4.4 too soon?

[sgillies@…]

New unit tests in python/tests/cases/hashtest.py pass.  Made changes to map.h
and HISTORY.TXT

[dmorissette]

Back to your question #3 about set() and PHP5: PHP MapScript now works with PHP5
(see bug 718), so it seems that we don't need to rewrite the module for PHP5 and
old PHP4 scripts will continue to work. So we are unlikely to do a full rewrite
of MapScript for PHP5 contrary to what I expected. In this context the only
thing that may happen with respect to the set() method is that we may deprecate
it at some point, but it would never go away completely, or not soon anyway.