Always report skipped feature hits for the original layer #4854

Merged
merged 2 commits into from Feb 22, 2016

Conversation

Projects
None yet
7 participants
@ahocevar
Member

ahocevar commented Feb 16, 2016

In the process of getting to the bottom of #4822, it occurred to me why so many iterations of patching the select interaction fixed one issue, only to create another: hit detection of features should return the same features and layers, no matter be features selected or not.

I think this fix makes the affected code easier to read and understand, and turns the select interaction into one that could almost be loved.

Fixes #4822.

@ahocevar ahocevar changed the title from Hit-detect skipped features, but not on unmanaged layer to Always report skipped feature hits for the original layer Feb 17, 2016

@marcjansen

This comment has been minimized.

Show comment
Hide comment
@marcjansen

marcjansen Feb 17, 2016

Member

Nice branch name 😉

Member

marcjansen commented Feb 17, 2016

Nice branch name 😉

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Feb 17, 2016

Member

Nice branch name 😉

Agreed.

Other things I like:

  • Net 7 lines removed!
  • And that is with something like 15 test lines added!

Things that make me scared:

  • This code always breaks.
  • This changes a test in a way that makes it seem like applications would break, but I'm so scared of the select interaction that I don't know if this is expected behavior or not.

In summary:

  • Andreas is awesome for his perseverance with the select interaction.
  • I hope that someone else who uses the select interaction can review this.
  • It would be really nice to have a clear understanding of what was broken before and what is fixed now.
Member

tschaub commented Feb 17, 2016

Nice branch name 😉

Agreed.

Other things I like:

  • Net 7 lines removed!
  • And that is with something like 15 test lines added!

Things that make me scared:

  • This code always breaks.
  • This changes a test in a way that makes it seem like applications would break, but I'm so scared of the select interaction that I don't know if this is expected behavior or not.

In summary:

  • Andreas is awesome for his perseverance with the select interaction.
  • I hope that someone else who uses the select interaction can review this.
  • It would be really nice to have a clear understanding of what was broken before and what is fixed now.
@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 17, 2016

Member

@tschaub I wrote a blog post about the whole story: http://ahocevar.net/openlayers/2016/02/17/trilogy-of-error.html. I hope it explains the whole evolution of these problems.

Member

ahocevar commented Feb 17, 2016

@tschaub I wrote a blog post about the whole story: http://ahocevar.net/openlayers/2016/02/17/trilogy-of-error.html. I hope it explains the whole evolution of these problems.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 17, 2016

Member

@tschaub:

This changes a test in a way that makes it seem like applications would break, but I'm so scared of the select interaction that I don't know if this is expected behavior or not.

There is no real need to make this change, so I changed it back to the way it was.

The key change of this pull request is that skipped features are now hit-detected the same way as if they had never been changed on their original layer. This means that they will be hit-detected on their original layer only, even when duplicated on one or more unmanaged layers.

Member

ahocevar commented Feb 17, 2016

@tschaub:

This changes a test in a way that makes it seem like applications would break, but I'm so scared of the select interaction that I don't know if this is expected behavior or not.

There is no real need to make this change, so I changed it back to the way it was.

The key change of this pull request is that skipped features are now hit-detected the same way as if they had never been changed on their original layer. This means that they will be hit-detected on their original layer only, even when duplicated on one or more unmanaged layers.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 17, 2016

Member

It would be really nice to have a clear understanding of what was broken before and what is fixed now.

When a feature is selected, the Select interaction calls ol.Map#skipFeature() with the selected feature, and copies the feature to its internal unmanaged layer. Before this change, an already selected feature was reported by ol.Map#forEachFeatureAtPixel to come from a null layer. After this change, it is still reported to come from the layer that it was selected from. The Select interaction had a lot of code to deal with this change of layers for a selected feature, which could now be removed.

For user created unmanaged layers, nothing changes, because a user cannot call ol.Map#skipFeature().

Member

ahocevar commented Feb 17, 2016

It would be really nice to have a clear understanding of what was broken before and what is fixed now.

When a feature is selected, the Select interaction calls ol.Map#skipFeature() with the selected feature, and copies the feature to its internal unmanaged layer. Before this change, an already selected feature was reported by ol.Map#forEachFeatureAtPixel to come from a null layer. After this change, it is still reported to come from the layer that it was selected from. The Select interaction had a lot of code to deal with this change of layers for a selected feature, which could now be removed.

For user created unmanaged layers, nothing changes, because a user cannot call ol.Map#skipFeature().

@ischas

This comment has been minimized.

Show comment
Hide comment
@ischas

ischas Feb 17, 2016

Contributor

I wrote a blog post about the whole story: http://ahocevar.net/openlayers/2015/02/17/trilogy-of-error.html.

@ahocevar So if you knew that all one year before (see publishing date), why didn't you fix it earlier? ;-)

Contributor

ischas commented Feb 17, 2016

I wrote a blog post about the whole story: http://ahocevar.net/openlayers/2015/02/17/trilogy-of-error.html.

@ahocevar So if you knew that all one year before (see publishing date), why didn't you fix it earlier? ;-)

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 17, 2016

Member

Thanks @ischas - I shouldn't be writing blog posts at 2am. Updated blog post URL: http://ahocevar.net/openlayers/2016/02/17/trilogy-of-error.html.

Member

ahocevar commented Feb 17, 2016

Thanks @ischas - I shouldn't be writing blog posts at 2am. Updated blog post URL: http://ahocevar.net/openlayers/2016/02/17/trilogy-of-error.html.

@ischas

This comment has been minimized.

Show comment
Hide comment
@ischas

ischas Feb 17, 2016

Contributor

@ahocevar 404! First link was good, but the publishing date was wrong! Maybe you must fix the blog post, too.

Contributor

ischas commented Feb 17, 2016

@ahocevar 404! First link was good, but the publishing date was wrong! Maybe you must fix the blog post, too.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 17, 2016

Member

@ischas Took a few seconds to update on the server. Now it's live.

Member

ahocevar commented Feb 17, 2016

@ischas Took a few seconds to update on the server. Now it's live.

@jonataswalker

This comment has been minimized.

Show comment
Hide comment
@jonataswalker

jonataswalker Feb 17, 2016

Contributor

Andreas is awesome for his perseverance with the select interaction.

No doubt. All of us are benefited at his cost.

Contributor

jonataswalker commented Feb 17, 2016

Andreas is awesome for his perseverance with the select interaction.

No doubt. All of us are benefited at his cost.

@bjornharrtell

This comment has been minimized.

Show comment
Hide comment
@bjornharrtell

bjornharrtell Feb 17, 2016

Contributor

Nice work @ahocevar, did some testing with an application of mine that makes good use of the Select interaction here and there and it works as expected.

This also makes #3453 easier to fix so I've rebased that on this PR.

Contributor

bjornharrtell commented Feb 17, 2016

Nice work @ahocevar, did some testing with an application of mine that makes good use of the Select interaction here and there and it works as expected.

This also makes #3453 easier to fix so I've rebased that on this PR.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Feb 21, 2016

Member

Anyone brave enough to review this? @fredj or @tsauerwein maybe?

Member

ahocevar commented Feb 21, 2016

Anyone brave enough to review this? @fredj or @tsauerwein maybe?

@fredj

This comment has been minimized.

Show comment
Hide comment
@fredj

fredj Feb 22, 2016

Member

LGTM, thanks Andreas

Member

fredj commented Feb 22, 2016

LGTM, thanks Andreas

ahocevar added a commit that referenced this pull request Feb 22, 2016

Merge pull request #4854 from ahocevar/fix-select-and-everything-that…
…-relies-on-unmanaged-layers-and-skipped-features-this-time-for-real---hopefully

Always report skipped feature hits for the original layer

@ahocevar ahocevar merged commit 6c7d681 into openlayers:master Feb 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 76.347%
Details

@ahocevar ahocevar deleted the ahocevar:fix-select-and-everything-that-relies-on-unmanaged-layers-and-skipped-features-this-time-for-real---hopefully branch Feb 22, 2016

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