[OpenLayers-Dev] unnecessary method invocations ...

[Erik Uzureau]

Hi Paul what a great email from you -- it's cool to know that someone else
is looking through the guts of this stuff. Your comments are very welcome.
i am responding inline below....

On 9/26/07, Paul Spencer <pspencer at dmsolutions.ca> wrote:
> I'm starting to look at the impact of modifying rendering in
> singleTile layers to not clear the existing tile until the new tile
> arrives.  As I try to grok the code related to this, I'm running into
> a couple of things I think are unnecessary.  My understanding of the
> code is still perhaps too limited to understand if the following are
> extraneous or not, but I thought I would throw it out there and see
> if others can explain why they are necessary:
>
> At the end of initSingleTile in Grid.js, there is a call to:
>
> this.removeExcessTiles(1,1);
>
> This seems unnecessary to me as several lines before, this.grid is
> set to an empty array and exactly one new tile is added to it.  It
> does not seem to me that this.grid can contain any more than one tile
> at the point removeExcessTiles is called and therefore this is pure
> overhead (not much, I grant).

So I believe the reason I put that in there was for the case where you
have a gridded layer and you switch it mid-operation to a single-tile layer.
I vaguely remember thinking at some point that we should support that
case. Of course, realistically, that's pretty absurd. As you say, though,
at least at this point, the overhead is pretty minimal.

If we can all agree that switching the 'singleTile' variable mid-run is NOT
something that we want to support, then I think we could safely remove
that. I think the best route for that sort of change is a 2.6 ticket with the
simple patch (note that it will probably break tests, so we'll have to update
that too).

>
> In Tile.js moveTo, this.clear() is called prior to calling this.draw
> (), which also calls this.clear().  It seems calling this.clear in
> moveTo is unnecessary.  This is not quite true because there is a
> code path involving the redraw variable where this.draw() is not
> called and so the clear would need to be done in the alternate path.
> Of course, if you are moving a layer and not redrawing it, should you
> clear it?

This is a great find and something that I actually squeezed my head
over quite a bit just last week with a MetaCarta single-tiled application
that I'm building. Ran into the same exact problem as I'm sure you are, that
the tile gets cleared, then the request goes out and the map is blank while
you wait for the new data to load. clearly not ideal.

Chris helped me think this one through a few times and yes I think you're
totally right. The clear definitely does *not* need to be in there. The clear()
belongs *immediately* before the actual drawing happens and nowhere
else.

The case you bring up about the 'redraw' option is a good one too and but
it turns out that's only used to facilitate the spiralTileLoad()
function.... which
is to say that the tiles which are moved but not drawn are almost all drawn
after all tiles have been moved.

I meant to make a ticket for this but then ended up finding a
different workaround
in my application and forgot to get to it.

I've opened
http://trac.openlayers.org/ticket/1017

and put a patch in there. Unfortunately, the 2.5 release is pretty
much on its way out the door, so this will also have to wait for 2.6.
:-(

Erik
> Discussion invited :)
>
> Cheers
>
> Paul
>
> +-----------------------------------------------------------------+
> |Paul Spencer                          pspencer at dmsolutions.ca    |
> +-----------------------------------------------------------------+
> |Chief Technology Officer                                         |
> |DM Solutions Group Inc                http://www.dmsolutions.ca/ |
> +-----------------------------------------------------------------+
>
>
>
>
>
> _______________________________________________
> Dev mailing list
> Dev at openlayers.org
> http://openlayers.org/mailman/listinfo/dev
>