msGetSymbolIndex() shouldn't call msAddImageSymbol()

[sgillies@…]

Am going to remove the call to msAddImageSymbol from msGetSymbolIndex.
And then extend symbolSetObj with an index method to be used like

   index = the_map.symbolset.index('circle')

msGetSymbolIndex should simply iterate through the symbols and do
a string comp on the names, right?  And if it doesn't find the index,
raise an error and return -1.  

So what's up with the msAddImageSymbol call?  Holy side-effects Batman!  Is
there other code that relies on this side-effect?

[dmorissette]

This call isn't there by accident, I'll try to get back to you with more
details, or Steve can possibly provide some.

[assefa]

I think that is used when a user specifies an image symbol directly in his map
file. So removing it completly may cause problems with existing applications.

[sgillies@…]

Daniel, Assefa, thanks for the clarification.  I'd forgotten about using the
symbol filename directly.

I think what I will do is to pull everything except the call to msAddImageSymbol
out into a new function, like this

int msGetSymbolIndexNoSideEffects(...) {
    ...
}

int msGetSymbolIndex() {
    ...
    msGetSymbolIndexNoSideEffects(...);
    msAddImageSymbol(...);
    ...
}

[dmorissette]

May I ask why you need to do that?  
Also I'm sure a better name could be found for msGetSymbolIndexNoSideEffects(...)

[sgillies@…]

Here's the motivation:

In mapscript (SWIG) we can only set a symbol by its index, like this

    the_style = the_class.getStyle(0)
    the_style.symbol = 1

This is a limitation, but that's the way it is now.  To help work
around this, I am implementing an index method to extend 
symbolSetObj.  If the user has available the names of symbols, she
can then do this

    the_style = the_class.getStyle(0)
    the_style.symbol = the_map.symbolset.index('circle')

It is, IMO, far better to require of a user only that they know the
names, which are descriptive, of symbols than to require of them,
as we do now, that they know which symbol is at which index in the
symbolset.  Furthermore, when we have a collection-ish class such
as symbolSetObj, an index method is a natural feature that users
expect.

The best thing would be to rewrite the style.symbol getter to
accept strings or ints but this would only work for Python because
the Perl SWIG stuff is lagging a bit behind in the area of shadow
class features.  Because of this we need a way to get the index
of a symbol using the symbol name.  The symbolset is the only thing
that knows the index of the symbol, hence an index method.

Indeed, there is a better name than 'msGetSymbolIndexNoSideEffects'.
You know what it is?  It's 'msGetSymbolIndex'.  But that name is
already taken by a function that should more accurately be named
'msGetSymbolIndexAndMaybeCreateANewSymbolAsASideEffect'.  Look at
msGetLayerIndex -- it does exactly one thing with no side effects
and that's the kind of function I need.

If you don't feel right about my altering the existing function, I'll
create a new one in mapsymbol.c and leave the existing code alone.

[dmorissette]

Please, Sean, this is an open source project and we have to all work together in
the same direction or we'll get nowhere.

There is indeed an historical reason behind this name and dual behavior.  At the
time we had an exchange with Steve and considered calling the function
msGetOrAddSymbolByName(), but this name, even though it was better than what you
suggest, didn't sound very well either.  In the interest of keeping the
MapServer API small, Steve suggested that we just extend msGetSymbolIndex() to
allocate the image symbol if an image file with the specified name existed. This
also had the benefit of being in line with the way symbols are loaded/allocated
when a mapfile is read. I can forward you the whole email thread if you want.

My opinion is that we should keep things as they are since they work and the
behavior is predictable and already works for what you need AFAIK.  That doesn't
mean I'm proud of the name, but MapServer wouldn't be where it is today if we
didn't have an open mind and kept changing the code everytime that there is
something that isn't in line with our way of thinking.

[sgillies@…]

Sorry, Daniel, I did get carried away with the naming.  My initial comments
about side effects seemed to have set a bad tone right off the bat.  I meant
it to be a joke, not accusing anyone of writing bad code.

I hope I did persuade that the work itself is justified, if not the naming.
I'll pick a final name that is as blame-free as it is descriptive.

[dmorissette]

If you are going to create a separate function anyway then we might as well do
things the right way, and use msGetSymbolIndex() for the version of the function
that just looks up existing symbols, and find a new name for the version of the
function that calls msAddImageSymbol(). Also make sure that we replace all
existing calls to msGetSymbolIndex() with calls to the new function.

[dmorissette]

I'll need to fix this one before 4.2 because the same issue came up in bug 601.

[dmorissette]

I still can't come up with a good name for the new function.

I'm thinking that the simplest might be to add a boolean parameter to the
existing msGetSymbolIndex() function to control whether it should try to
allocate a new image symbol or not if not found in the symbolset:

int msGetSymbolIndex(symbolSetObj *set, char *name, int try_addimage_if_notfound)

I would do that after lunch if there is no objection.

[dmorissette]

I have committed the msGetSymbolIndex() with the new try_addimage_if_notfound
parameter and updated all the code, including mapscript.i, to pass the new
argument.  I couldn't test mapscript.i, hopefully I didn't break anything.