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

Fix #2668 Increase precision of Query Panel Circle/BBox Details #2720

Merged
merged 5 commits into from
Mar 9, 2018

Conversation

kappu72
Copy link
Contributor

@kappu72 kappu72 commented Mar 9, 2018

Description

If Query Panel Geometry details has proj 4326 the precision off numeric fields is increased to 6th decimal, the step is related to the current scale, and the update is debounced by 1 sec.

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
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
All numeric fields has tow decimal precision, and the step is always one

What is the new behaviour?
If proj is 4326, coordinates fields has 6 decimal precision, the step is related to the scale and the update is debounced by one sec. The radius has 2 decimal precision.
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:

@ghost ghost removed the pending review label Mar 9, 2018
this.props.onShowPanel(false);
};

roundValue = (val, prec = 1000000) => Math.round(val * prec) / prec;
getStep = (zoom = 1) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can translate this into a formula, in any case we shouldn't use ifs

const step = this.getStep(zoom);
return name === 'radius' && step * 100000 || step;
};
isWGS84 = () => (this.props.geometry || {}).projection === 'EPSG:4326' || !this.props.useMapProjection;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use proj4 to know the uom of the projection, and generalize EPSG:4326 to uom === 'degrees'

@@ -77,7 +79,7 @@ class GeometryDetails extends React.Component {
projection: this.props.geometry.projection
};

this.props.onChangeDrawingStatus("replace", undefined, "queryform", [geometry]);
this.props.onChangeDrawingStatus({geometry: [geometry]});
Copy link
Contributor

Choose a reason for hiding this comment

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

is CHANGE_DRAWING_STATUS without a "command" (e.g. replace) supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe was better to change the name, but isn't directly calling the onChangeDrawingStatus action. That action is debounced and then called by enhancer

Copy link
Contributor

Choose a reason for hiding this comment

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

Actions have a standard interface, you are changing it here

const propsStreamFactory = require('../../../misc/enhancers/propsStreamFactory');


const dataStreamFactory = $props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done to debounce the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

it also changes the interface to call onChangeDrawingStatus, which is not immediately clear, look at the other question a bove

Copy link
Contributor Author

@kappu72 kappu72 Mar 9, 2018

Choose a reason for hiding this comment

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

see answer above

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, if you put a "transparent" debounce in the middle, it should be transparent, if I remove it the same code will work, without debouncing. We want to enhance, not to change.

@coveralls
Copy link

coveralls commented Mar 9, 2018

Coverage Status

Coverage increased (+0.008%) to 80.505% when pulling c902c7c on kappu72:i#2668 into ebb2dd1 on geosolutions-it:master.

this.props.onShowPanel(false);
};

getStep = (zoom = 1) => {
if ( zoom >= 21 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we translate this into a formula?

const {compose, withHandlers} = require('recompose');
const {debounce} = require("lodash");

module.exports = compose(withHandlers((initProp) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we generalize this into a "debouncing" enhancer? I don't get the purpose of having single use enhancers in separate files

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.

Take a look on my PR for required changes.
I removed debouncing because it caused more issues tha benefits. Please add a test for your changes

const {debounce} = require("lodash");
const emptyFunc = () => {};
/**
* This enhancer debounce an action or of a passed debounceTime
Copy link
Member

Choose a reason for hiding this comment

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

This enhancer de-bounce a method passed as prop of the given time.

@offtherailz offtherailz changed the title Fix #2668 Fix #2668 Increase precision of Query Panel Circle/BBox Details Mar 9, 2018
@offtherailz offtherailz merged commit 55bdb6e into geosolutions-it:master Mar 9, 2018
@ghost ghost removed In Test review labels Mar 9, 2018
kappu72 added a commit to kappu72/MapStore2 that referenced this pull request Mar 12, 2018
@chiaracurcio chiaracurcio self-assigned this Apr 19, 2018
@allyoucanmap allyoucanmap mentioned this pull request Aug 9, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase precision of Query Panel Circle/BBox Details
5 participants