Layers.java public synchronized Layer set(int index, Layer layer) not binding correctly

[stleusc]

https://github.com/mapsforge/vtm/blob/6eecba7c022de8967160d78a758ba20db96db359/vtm/src/org/oscim/map/Layers.java#L180C4-L214C6

The set function should behave like calling remove() followed by add().
However, set only UNBINDS the old layer, never binds the new layer.

Likely could be fixed and simplified by just calling remove() add().

[devemux86]

@stleusc thanks for the report.

The remove also changes the group pointers, so better not mix them.

We could just add the missing bindings from the add.

[stleusc]

but should the group pointers NOT be updated?

Looking at public synchronized void add(int index, Layer layer) it seems it does bindings for 'groups'.
But mGroupList is not touched in that case.

I think there is something else missing regarding groups?!

[devemux86]

but should the group pointers NOT be updated?

That's why the remove (used with or without groups) should not be used in regular set.
Just copy these lines from regular add.

---

The regular add(int index, Layer layer) and set(int index, Layer layer) override List methods
and do not use groups (can only use GroupLayer).

For groups must use the extra add(Layer layer, int group) method.

[stleusc]

Guess I need to ask in a better way...

IF I plan to add a layer (no group) and then remove it, I would do the follwing:

add(index, layer):
remove(layer);

But that does modify the mGroupList on remove!

It seems liket that is not correct?!

I understand how I could make set(index, layer) work but if I just want to remove a layer that still seems to be incorrect or not?

Somehow the generic remove function needs to know if we are in need of messing with mGroupList. And that is NOT always the case.

[devemux86]

But that does modify the mGroupList on remove!

No, the remove does not modify the mGroupList.

Anyway groups should either be used for all levels or not.
Using groups and add layers without group makes no sense.
Better not mix them.

[stleusc]

I don't think we understand each other.... I try to explain better.

I want to be able to remove a layer, and at a later time, add a layer.

I call

    @Override
    public synchronized void add(int index, Layer layer) {
        if (mLayerList.contains(layer)) {
            log.warn("layer already exists");
            return;
        }

        // bind added layer
        if (layer instanceof UpdateListener)
            mMap.events.bind((UpdateListener) layer);
        if (layer instanceof InputListener)
            mMap.input.bind((InputListener) layer);
        // add zoom limit to tile manager
        if (layer instanceof ZoomLimiter.IZoomLimiter)
            ((ZoomLimiter.IZoomLimiter) layer).addZoomLimit();

        // bind added group layer
        if (layer instanceof GroupLayer) {
            GroupLayer groupLayer = (GroupLayer) layer;
            for (Layer gl : groupLayer.layers) {
                if (gl instanceof UpdateListener)
                    mMap.events.bind((UpdateListener) gl);
                if (gl instanceof InputListener)
                    mMap.input.bind((InputListener) gl);
                if (gl instanceof ZoomLimiter.IZoomLimiter)
                    ((ZoomLimiter.IZoomLimiter) gl).addZoomLimit();
            }
        }

        layer.setEnableHandler(mEnableHandler);
        mLayerList.add(index, layer);
        mDirtyLayers = true;
    }

and

    @Override
    public synchronized Layer remove(int index) {
        mDirtyLayers = true;

        Layer remove = mLayerList.remove(index);

        // unbind removed layer
        if (remove instanceof UpdateListener)
            mMap.events.unbind((UpdateListener) remove);
        if (remove instanceof InputListener)
            mMap.input.unbind((InputListener) remove);
        // remove zoom limit from tile manager
        if (remove instanceof ZoomLimiter.IZoomLimiter)
            ((ZoomLimiter.IZoomLimiter) remove).removeZoomLimit();

        // unbind removed group layer
        if (remove instanceof GroupLayer) {
            GroupLayer groupLayer = (GroupLayer) remove;
            for (Layer gl : groupLayer.layers) {
                if (gl instanceof UpdateListener)
                    mMap.events.unbind((UpdateListener) gl);
                if (gl instanceof InputListener)
                    mMap.input.unbind((InputListener) gl);
                if (gl instanceof ZoomLimiter.IZoomLimiter)
                    ((ZoomLimiter.IZoomLimiter) gl).removeZoomLimit();
            }
        }

~~~ FROM HERE
        // update layer group pointers
        for (Integer group : mGroupIndex.keySet()) {
            int pointer = mGroupIndex.get(group);
            if (pointer > index)
                mGroupIndex.put(group, pointer - 1);
        }
~~~ TO HERE

        remove.setEnableHandler(null);
        return remove;
    }

Why does the second function contain the portion marked? Does that NOT mess with mGroupIndex?

You said early on, I should NOT call remove followed by an add.
I am confused... I am not using groups. Just those 2 functions.

Does this make more sense now?

[devemux86]

Why does the second function contain the portion marked? Does that NOT mess with mGroupIndex?

If layers size changes, the group index should be updated. And like already mentioned:

Anyway groups should either be used for all levels or not.
Using groups and add layers without group makes no sense.
Better not mix them.

So if not using groups there is no problem.
And if using groups, must use them for all layers.

I said we should not call remove / add inside the set,
but just copy the relevant section.