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

Update coordinates of image/video sources #2184

Closed
wants to merge 5 commits into from
Closed

Update coordinates of image/video sources #2184

wants to merge 5 commits into from

Conversation

averas
Copy link
Contributor

@averas averas commented Feb 25, 2016

This fixes #1880 by adding support for updating the coordinates on image- and video sources.

While doing this I noticed this part which I don't really understand how it's supposed to work:

    onAdd: function(map) {
        this.map = map;
        if (this.image) {
            this.setCoordinates();
        }
    }

Won't a call without arguments to setCoordinates (perviously createTile) cause an error on the subsequent coordinates.map() in that function?

@@ -57,19 +56,27 @@ ImageSource.prototype = util.inherit(Evented, {
onAdd: function(map) {
this.map = map;
if (this.image) {
this.createTile();
this.setCoordinates();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method gets fired when the ImageSource is added to an instance of Map. There are two cases to consider:

  • the HTTP request for the image finishes before the ImageSource gets added to the map
  • the HTTP request for the image returns after the ImageSource gets added to the map

The code in this method deals with the first case.

I think the lack of an argument to setCoordinates is a mistake. The line should be

this.setCoordinates(this.coordinates)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I am completely with you on why the code exists, it was the intentional or unintentional lack of arguments that got me confused. If it in fact was unintentional everything makes total sense.

@lucaswoj
Copy link
Contributor

Thank you for the PR @averas! Looking forward to the next round of revisions, feedback from some other Mapbox-ers, and then shipping this!

@ansis
Copy link
Contributor

ansis commented Feb 26, 2016

Looks good to me!

Is there a better name than coordinates to use here? I realize the choice to use coordinates happened before this pull request but if we want to rename it now would be a good time.

corners, geometry?

@lucaswoj
Copy link
Contributor

I like coordinates.

Any last words before we 🚢 @jfirebaugh @mourner?

@mourner
Copy link
Member

mourner commented Feb 26, 2016

:shipit:

@jfirebaugh
Copy link
Contributor

👍

@lucaswoj
Copy link
Contributor

Merged in e7b551c 🚀

Thanks @averas!

@lucaswoj lucaswoj closed this Feb 26, 2016
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.

Update coordinates for ImageSource (and possibly VideoSource)
5 participants