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-googlemap-china-offset: fix style issues #65

Conversation

johnd0e
Copy link
Contributor

@johnd0e johnd0e commented Jan 17, 2019

lint:

  • let is not in es5
  • 'HK_LENGTH' is assigned a value but never used
  • 'original' is defined but never used

style:

  • == vs ===
  • Mixed spaces and tabs
  • fix indents, missed/extra/trailing spaces, missed semi

lint:
* `let` is not in es5
* 'HK_LENGTH' is assigned a value but never used
* 'original' is defined but never used

style:
* == vs ===
* Mixed spaces and tabs
* fix indents, missed/extra/trailing spaces, missed semi
@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

And there is still error in L.GridLayer.prototype._setZoomTransform

L.GridLayer.prototype._setZoomTransform = (function (original) {
  return function (level, center, zoom) {
    center = window.plugin.fixChinaOffset.getLatLng(center, this.options.type);
    original.apply(this, arguments);
  };
})(L.GridLayer.prototype._setZoomTransform);
  • zoom is defined but never used
  • center is assigned a value but never used

So center is not propagated back to arguments.

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

'original' is defined but never used

I suppose this also should be fixed adding something like original.apply(this, arguments);

@modos189
Copy link
Contributor

Please update PR to new commit

@modos189
Copy link
Contributor

modos189 commented Jan 17, 2019

So center is not propagated back to arguments.

No, it is propagated back. This is a bug in es linter, or browser?
Maybe original.apply(this, [level, center, zoom]);

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

No, it is propagated back. This is a bug in es linter, or browser?

You are right. I suppose bug in eslint.
Anyway, your second variant looks cleaner:

Maybe original.apply(this, [level, center, zoom]);

And please comment about this:

'original' is defined but never used

Was it really unused argument?
Or there is missing original.apply(this, arguments);

@modos189
Copy link
Contributor

Was it really unused argument?

Here, indeed, unused variable. I tried calling original method as in other cases, but then errors occur when calling class Bounds

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

Merged (need to test again).

And I am going to review the code one more time.
E.g. I doubt that we need PRCoords as object. Wouldn't it better just have two distinct functions?

@modos189
Copy link
Contributor

Method isInGoogle can be completely removed. It is not used

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

It is not used

As I can see it is used as fast precheck if latlng is in bounds of square, which covering China.
Should be renamed.

@modos189
Copy link
Contributor

But why? The plugin still needs to be installed separately for those from China.

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

Perhaps for those who are not China, but still want to see real map offset while browsing China.

Could you make some tests comparing performance between these two functions?
(May be we do not need optimization here)

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

@modos189
Copy link
Contributor

I'm confused. Method isInGoogle is used. It as roughly and thoroughly checked the coordinates.
This method calling from gcjObfs.

Another thing is that there is no need to make it public.

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

Another thing is that there is no need to make it public.

Well, it could be useful for someone, who wants to reuse this function.

@modos189
Copy link
Contributor

Um. Well, maybe as two distinct functions

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

I failed to identify the source of obfuscation algorithm in our code.
As it differs from old (which was referenced as https://on4wp7.codeplex.com/SourceControl/changeset/view/21483#353936).
And it also differs from included with new link: https://github.com/Artoria2e5/PRCoords

So it's sort a mess...

I'm prefer that policy about external sources (and images) should be included unmodified (and inlined only in build-process).

@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 17, 2019

Well, maybe as two distinct functions

Agree, as there are 2 distinct action, and there are several methods to do every of them.
So better to divide them in order to make support simpler (e.g. tomorrow we'll prefer to replace one function)

@modos189 modos189 merged commit 555f025 into IITC-CE:fix-google-map-offsets-in-china Jan 17, 2019
@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 18, 2019

Parent issue: #64

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

Successfully merging this pull request may close these issues.

None yet

2 participants