Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crsselector plugin #3191 #3410

Merged
merged 13 commits into from
Jan 21, 2019
Merged

Conversation

baloola
Copy link
Contributor

@baloola baloola commented Dec 13, 2018

Description

This PR adds the possibility to select the CRS for the map.
The listed projections should be added and defined in the configuration file.
Once the user selects the new projection, the view re-render the map.

Issues

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature

What is the current behavior? (You can also link to an open issue here)
#3191

What is the new behavior?
see the discribtion

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link

coveralls commented Dec 13, 2018

Coverage Status

Coverage decreased (-0.1%) to 80.645% when pulling acbcdbe on baloola:crs_selector into b07332f on geosolutions-it:c042_crs.

this.props.changeInputValue(e.target.value);
}

focusNext = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?


render() {

var list = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to use const wherever possible, let otherwise, never use var.

}
)(Selector);
/**
* CRSSelector Plugin is a plugin that shows the coordinate of the mouse position in a selected crs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update description... I presume this has been copied from the similar control we have for MousePosition....

const PropTypes = require('prop-types');
const { ListGroupItem, ListGroup, FormControl } = require('react-bootstrap');

class CustomMenu extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component name and filename should be the same, unless there is any particular reason. Also, component jsx file should start with an uppercase letter.

@tdipisa tdipisa modified the milestones: 2019.01.01, 2019.01.00 Dec 18, 2018
@geosolutions-it geosolutions-it deleted a comment Jan 4, 2019
@tdipisa tdipisa requested review from offtherailz and removed request for allyoucanmap January 7, 2019 14:54
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix as for comments

. Please configure to add projection 3003 and use this Map to reproduce errors

  • Some errors found Performing GetFeatureInfo in 3003
    ** steps to reproduce (after switch to 3003)
  • Switch to 3003
  • Click on the map
    )
    Result:
    After some time you will see this error:
Subscriber.js:238 Uncaught TypeError: Cannot read property 'x' of null
    at geocentricFromWgs84 (datumUtils.js:236)
    at __webpack_exports__.a (datum_transform.js:37)
    at Function.transform (transform.js:57)
    at Object.reproject (CoordinatesUtils.js:164)
    at Object.buildRequest (wms.js:42)
    at buildIdentifyRequest (MapInfoUtils.js:141)
    at eval (identify.js:148)
    at Array.forEach (<anonymous>)
    at Lifecycle.componentWillReceiveProps (identify.js:147)
    at eval (ReactCompositeComponent.js:608)
    at measureLifeCyclePerf (ReactCompositeComponent.js:73)
    at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:607)
    at ReactCompositeComponentWrapper.receiveComponent (ReactCompositeComponent.js:544)
    at Object.receiveComponent (ReactReconciler.js:122)
    at ReactCompositeComponentWrapper._updateRenderedComponent (ReactCompositeComponent.js:751)
    at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:721)

and also this, sometimes:

Uncaught RangeError: Maximum call stack size exceeded
    at Array.map (<anonymous>)
    at MapPlugin._this.renderLayers (Map.jsx:267)
    at MapPlugin.render (Map.jsx:351)
    at MapPlugin.tryRender (index.js:34)
    at eval (ReactCompositeComponent.js:793)
    at measureLifeCyclePerf (ReactCompositeComponent.js:73)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (ReactCompositeComponent.js:792)
    at ReactCompositeComponentWrapper._renderValidatedComponent (ReactCompositeComponent.js:819)
    at ReactCompositeComponentWrapper._updateRenderedComponent (ReactCompositeComponent.js:743)
    at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:721)
    at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:642)
    at ReactCompositeComponentWrapper.receiveComponent (ReactCompositeComponent.js:544)
    at Object.receiveComponent (ReactReconciler.js:122)
    at ReactCompositeComponentWrapper._updateRenderedComponent (ReactCompositeComponent.js:751)
    at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:721)
    at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:642)

The application stops working

  • Duplicated notifications
    If you have more than one layer on the map, you will see duplicated warning notification when switch to 3003
    image

WFS table widget

  • WFS table widget still not working when map viewport is bigger than the world in 4326 (shows an error). Investigate reason and check what to do.

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done, I think. nice job!

Pending things:

  • WFS Table : did you investigated on that problem yet?
  • When I try to load the map in a CRS that is not configured, I have this page:
    image
    I'd like instead to see a better explaination. This can be done by managing the exception in a better way:
    • When you trigger the exception, do not use only "EPSG:4326" but a full javascript error containing the information you need.
    • Add at least an entry to the description when you say that the application don't support the map's EPSG.
  • When you click 2 times for feature info, the map moves in unexpected way.
  • Disable the plugin. I see it is automatically disabled when switch to 3D. I think the CRSSelector should also disappear in leaflet map.

@geosolutions-it geosolutions-it deleted a comment Jan 15, 2019
@geosolutions-it geosolutions-it deleted a comment Jan 15, 2019
@baloola
Copy link
Contributor Author

baloola commented Jan 15, 2019

try to re-configure the 'EPSG:3003' in localConfig.json and it should work.
I will disable the plugin in leaflet.

const containerClass = this.props.vertical ? 'background-preview-icon-container-vertical' : 'background-preview-icon-container-horizontal';
const type = this.props.layer.visibility ? ' bg-primary' : ' bg-body';
const invalid = this.props.layer.invalid ? ' disabled-icon' : '';
const invalid = ((validCrs || compatibleWmts || this.props.layer.type === "wms" || this.props.layer.type === "empty") && !this.props.layer.invalid ) ? '' : ' disabled-icon';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this boolean, and calculate the className after.

let tempCenter = view.getCenter();
let projectionExtent = view.getProjection().getExtent();
const currentProjectionName = view.getProjection().getCode();
if (currentProjectionName === 'EPSG:4326' || currentProjectionName === 'EPSG:900913' || currentProjectionName === 'EPSG:3857') tempCenter[0] = CoordinatesUtils.normalizeLng(tempCenter[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pass ESLint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it does, but I'll refactor it.

@tdipisa tdipisa modified the milestones: 2019.01.00, 2019.01.01 Jan 16, 2019
@baloola baloola changed the base branch from master to c042_crs January 16, 2019 11:30
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't re-format files - commit only effective changes:

  • localConfig.json
  • icons.less
  • Vector layer has some problems with projections like 3003. I report the issues I found, probably caused by the same issue:
    • in the GetFeatureInfo, using EPSG:3003 the marker goes out of the viewport, while everything else seems to work fine.
      marker-gfi
    • The same of getfeatureinfo is for search:
      image
      search_vector
  • Switching prj
  • also annotation have problems, when save
    annotation_crs

Instead:

  • advanced filter works well
  • Switching CRS most of the times works (from 3857 to 4326). The problem seems to appear only in 3003

The rest of the things seems to work.

@ghost ghost assigned offtherailz Jan 21, 2019
@offtherailz
Copy link
Member

I open (#3477) for the pending tasks:

  • WFS Table do not work when zoom out in 4326
  • When you try to load the map in a CRS that is not configured, I have a "not found" page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants