Measure tool widget - Add fixed position support for the popup

[belug]

While working with the measure tool I think that the popup is always in the way of the mouse. So adding a way to fix it to a corner of the screen seem to be a good idea. I've seen it like that in other GIS like QGIS or geomap.

[cdeschenes]

Patch for measure tool

[cdeschenes]

Please review!

[belug]

Please comment the closable at false! and why you added the width.

[cdeschenes]

A commented version of the patch

[belug]

adube could give me point of view on this one?

I think it's good like that, the only thing is the fixed width, but it's needed for the positionning, even getSize() fail if it's not there.

[adube]

I'll take a look.

[adube]

Here's a review.

• the patch is not valid :
  • if statements are wrong (true + if false - instead of - if else)
  • broken if statement
• try to respect the 80 characters length when possible
• the Ext.Window object has a alignTo method we could use to align the window inside the map panel by simply using it. Why not use that instead of hacking a width and position ?

[adube]

the close button don't close the tooltip when user is drawing with the tool... after double click, the closable windows will continue to apear

That kind of comment belongs here, after a commit and not in the code. Imagine if we did that for all boolean statement :)

Also, would you please correct the English ?

[cdeschenes]

the popup is not an Ext.Window... It's an Ext.ToolTip. The ToolTip class don't have the alignTo method for this reason, I can't use it.

[adube]

cdeschenes, sorry I wasn't clear enough. Let me rephrase it : why not use a Ext.Window instead of a Ext.ToolTip ? We would then be able to use its alignTo method.

[cdeschenes]

After the replacement of the tooltip object by Ext.Window object I'm unable to use the method alignTo on measureWindow with mapPanel as parameter (causing "Uncaught Element.alignToXY with an element that doesn't exist" errors). It seem impossible to fix this in the current context. I don't have anymore time to spend on this feature so, you can commit my last patch to add optional fixed position feature.

[adube]

cdeschenes, would you see your issue with belug, please ? Also, as I already mentioned, your patch is not valid. You need to fix it first.

Also, belug will take care of the commit and review.

[belug]

For the windows transformation we where not able to find a place where this.mappanel would be active and that would be executed only once on the opening of the tool.

and having the alignTo in the loop for updating of the measures would cancel the drag and drop. And this.mappanel is invalid most of the time in that loop.

So back to the tooltip with the calculated positionning.

For what i see it's only the double if instead of the if else and the tabs used for indentation to change in the last patch.

[adube]

belug, thanks for the explanations.

About the patch, make sure to remove the long comments and put them in your commit details here, in the ticket. For the rest, I'll leave it up to you.

Thanks.

[cdeschenes]

In the patch, the closable parameter of the first measureToolTip is now false because it's impossible to close tooltip here anyway. The width of measureToolTip is now set to 200 because the hack for top right position need to substract the tooltip width.

[cdeschenes]

Performance issue patch

[cdeschenes]

I made a new patch to improve performance of this widget in Internet Explorer 7 and 8... I finaly change the tooltip object for the Ext.Window because with Window object, I can create only one instance and use it any time in the widget. Currently we need to recreate instance of Ext.Tooltip each time the mouse move... It's a big performance issue especially for bad browser like Internet Explorer.

[cdeschenes]

Please note on line 418 : I used the old IE patch to set the final popup position for all browser... In the old version of the code, the tooltip position was probably define at mouse by default... Ext.Window don't have this behavior.

[cdeschenes]

I forget the window title in the last patch

[belug]

Fixed in r1367

thanks adube and cdeschenes

[adube]

It would be nice to have the sample updated showing this new option in action. Would you do that, please ?

If so, make sure to update the sample.php -> set status to "updated".

Thanks,

adube