Ticket #248 (closed enhancement: fixed)

Opened 5 years ago

Last modified 4 years ago

New widget for recentering on coordinates

Reported by: alex Owned by: fredj
Priority: major Milestone: 1.1 Release
Component: General.client Version: SVN
Keywords: Cc:
State: Review

Description

Hello,

attached is a patch adding a widget that allows users to recenter anywhere on the map by providing coordinates and, optionally, a scale picked in the predefined scales list.

The widget may be integrated using some code like:

        var locatePanel = { 
            title: OpenLayers.i18n('Locate'),
            items: {
                xtype: 'coordsrecenter',
                border: false,
                scales: foo.config.scales,
                //defaultScale: 4,
                showCenter: true,
                map: foo.map.map
            }
        };

Some optional config parameters are available:
- scales: list of available scales. If not provided, the scale selection combo is not displayed.

- defaultScale: default scale to zoom to if no scale info is provided (for instance with above scale selection combo). If no scale value is detected using this parameter or the scale selection combo, current scale is used.

- showCenter: boolean indicating if a vector cross must be displayed on the recentering point.

UI strings translations are provided in the patch (en, fr, de). Note that "de" strings are simply copied from the "en" strings (not actually translated).

Attachments

patch_mapfish_CoordsRecenter.txt (12.2 kB) - added by alex 5 years ago.
New widget Coords Recenter?
patch_mapfish_CoordsRecenter.2.txt (12.8 kB) - added by alex 5 years ago.
Updated patch, removing previous center symbol when performing several recentering in a row
widgetCoordsRecenter.patch (12.5 kB) - added by alex 5 years ago.
Patch update taking into account Patrick's suggestions
widgetCoordsRecenter_factorized.patch (27.0 kB) - added by alex 5 years ago.
Added web service recentering class written by Oliver
patch-248-r1313.diff (34.9 kB) - added by pgiraud 5 years ago.
patch-248-sandbox-MapFishAeai.diff (12.6 kB) - added by pgiraud 5 years ago.
patch-248-r1328.diff (17.6 kB) - added by pgiraud 5 years ago.
new patch against trunk, doesn't include WS recenter class
patch-248-r1328_Ws_only.diff (16.0 kB) - added by ochriste 4 years ago.
new patch against trunk, only for WS recenter class
patch-248-r1328-B0.diff (18.5 kB) - added by pgiraud 4 years ago.
patch-248-r1328-B1.diff (19.3 kB) - added by pgiraud 4 years ago.
final patch (for the coords part)
patch-248-r1331_missing_dolayout.diff (323 bytes) - added by ochriste 4 years ago.
missing doLayout in expand method
patch-248-r1331_misc.diff (0.8 kB) - added by ochriste 4 years ago.
correct dolayout and render condition (supercede previous patch)
patch.diff (0.5 kB) - added by aabt 4 years ago.
Corrects parenthesis mismatch
patch-MapFish-248-r1332-A0.diff (0.7 kB) - added by elemoine 4 years ago.

Change History

Changed 5 years ago by alex

New widget Coords Recenter?

Changed 5 years ago by alex

The widget has been successfully tested with IE6, IE7, FF3, Opera 9.52, Safari, Chrome.

What's the policy for unit tests? I don't see many tests for mapfish widgets.

Changed 5 years ago by alex

Updated patch, removing previous center symbol when performing several recentering in a row

Changed 5 years ago by alex

Patch update taking into account Patrick's suggestions

Changed 5 years ago by alex

Added web service recentering class written by Oliver

Changed 5 years ago by alex

Changed 5 years ago by pgiraud

Here's a new patch.

Difference between last patch :

  • classnames management removed, the textfields and comboboxes rendering are managed by the formPanel. The combobox rendering issue in accordion layout is by-passed.

If user wants a specific width for the form items, he'll have to set it in defaults on the container (recenter widget instance),

  • the scales weren't working correctly. In the new patch, I provide a Scale Combo Factory?.
  • the patch also includes implementation examples in layout.html and window.html

Changed 5 years ago by pgiraud

Changed 5 years ago by alex

Hello Pierre, thanks for the improvements/corrections.

However it seems that you have used old versions of the widgets (the patches in the ticket are indeed outdated, sorry).

For instance in your patch the recenterOnBbox() function is missing in Base.js: https://trac.mapfish.org/trac/mapfish/browser/sandbox/camptocamp/MapFishAeai/client/mfbase/mapfish/widgets/recenter/Base.js#L181

and the onWsRecenterSelect() in Ws.js is really different. See: https://trac.mapfish.org/trac/mapfish/browser/sandbox/camptocamp/MapFishAeai/client/mfbase/mapfish/widgets/recenter/Ws.js#L248

Changed 5 years ago by pgiraud

I worked on a new patch based on the current svn version in the Map Fish Aeai? sandbox. It has to be apply to this sandbox code.

Please give it a look. Also notice that I didn't changed anything in Ws.js because I had no way to test it so it is most probably broken. For example, fillComponent is to be changed to addItems with possible minor modifications.

Changed 5 years ago by pgiraud

Changed 5 years ago by pgiraud

new patch against trunk, doesn't include WS recenter class

Changed 5 years ago by pgiraud

  • state set to Review

I attached a new patch including renceter/Base.js, recenter/Coords.js and Scale Combo Factory?.js It *does not* include the recenter/Ws.js part.

Please review.

Changed 4 years ago by elemoine

  • state changed from Review to Needs more work

Review comments:

client/mfbase/mapfish/widgets/ScaleComboBoxFactory.js

  • Line 23: "Factory" isn't a Natural Docs keyword, make it "Function"
  • Line 24: Move the "Return" doc after the "Parameters" doc
  • Line 29: change from "options" to "comboConfig" for the name of the combo config object
  • Line 60: add missing semi-colon

client/mfbase/mapfish/widgets/recenter/Coords.js

  • Line 35: show an actual array of scales to make the example clearer
  • Line 68: improve the doc comments so we really understand why deferring adding items is needed
  • Line 68: it looks like addItems is called by "render" as opposed to "initComponent"
  • Line 40: "eventually" means "after an unspecified period of time" :-)

client/mfbase/mapfish/widgets/recenter/Base.js:

  • Line 76: remove "(private)" in the Natural Documentation
  • Line 86: ident is incorrect (use spaces instead of tabs)
  • Line 88: useless comment
  • Line 90: useless comment
  • Line 91: ident is incorrect (use spaces instead of tabs)
  • Line 93: in addition to "destroy" we need to deal with "expand", "collapse", "activate", "deactivate", "enable" and "disable", I'm particularly thinking about the vector layer, it must either be at least removed from the map when the component is collapsed, deactivated, or disabled. print/Base.js provides a good example.
  • Line 99: I think this is safer to register to the "render" event ("beforerender" here actually) rather than overriding the "render" method
  • Line 105: useless comment
  • Line 108: useless comment
  • Line 113: I think this is safer to register to the "expand" event rather than overriding the "expand" method
  • Line 129: we could make the addItems function return an error message saying that this method must be overridden by subclasses
  • Line 141: useless comment
  • Line 199: could be: zoom = zoom || this.defaultZoom || this.map.getZoom();

Changed 4 years ago by ochriste

new patch against trunk, only for WS recenter class

Changed 4 years ago by ochriste

I attached a new patch including rencenter/Ws.js, this complet Pierre patch for the other related files

Please review.

Changed 4 years ago by pgiraud

Oliver, can you confirm that my new patch doesn't break things in yours ?

Changed 4 years ago by pgiraud

Changed 4 years ago by pgiraud

  • state changed from Needs more work to Review

Eric and I had a quick review of the patch on IRC. I can provide the final patch.

Changed 4 years ago by pgiraud

final patch (for the coords part)

Changed 4 years ago by elemoine

  • state changed from Review to Commit

patch-248-r1328-B1.diff is good to go, please commit.

Changed 4 years ago by pgiraud

  • status changed from new to closed
  • resolution set to fixed

(In [1331]) add a recenter on coords widget it relies on a extension of a formPanel and a scale combobox factory r=elemoine, (Closes #248)

Changed 4 years ago by pgiraud

Oliver, a new ticket is now opened for the Ws.js part See #302

Changed 4 years ago by ochriste

missing doLayout in expand method

Changed 4 years ago by ochriste

  • status changed from closed to reopened
  • type changed from enhancement to defect
  • resolution fixed deleted

this.doLayout(); is required otherwise the elements are not rendered at all.

Changed 4 years ago by ochriste

correct dolayout and render condition (supercede previous patch)

Changed 4 years ago by ochriste

the new patch correct : missing doLayout in expand method AND if condition in the render method (reviewed by Pierre)

Changed 4 years ago by pgiraud

Ok, reviewed. You can commit ochriste.

Changed 4 years ago by ochriste

  • status changed from reopened to closed
  • resolution set to fixed

commited

Changed 4 years ago by elemoine

  • milestone set to 1.1 Release

Changed 4 years ago by aabt

  • status changed from closed to reopened
  • state changed from Commit to Review
  • resolution fixed deleted

Parenthesis mismatch in previous patch, which breaks layout in accordion.

Correction attached, please review.

Changed 4 years ago by aabt

Corrects parenthesis mismatch

Changed 4 years ago by elemoine

Changed 4 years ago by elemoine

aabt, I find the following logic clearer:

        if (!this.ownerCt.initialConfig.layout ||
            this.ownerCt.initialConfig.layout.toLowerCase != 'accordion') {
            this.addItems();
        }

Please commit if you agree.

Changed 4 years ago by elemoine

  • status changed from reopened to closed
  • resolution set to fixed

(In [1333]) New widget for recentering on coordinates, r=aabt,me p=aabt,me (closes #248)

Changed 4 years ago by elemoine

  • type changed from defect to enhancement
Note: See TracTickets for help on using tickets.