Skip to content
New issue

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

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

Already on GitHub? # to your account

[BUG] Emit move event even when source is custom #1231 #1232

Merged
merged 2 commits into from
May 12, 2022
Merged

[BUG] Emit move event even when source is custom #1231 #1232

merged 2 commits into from
May 12, 2022

Conversation

hschendel
Copy link
Contributor

As outlined in #1231, and discussed in Discord on 2022-05-11, certain zoom changes do not cause a move event to be emitted. This appears to be a bug, as probably everything changing the bounds should cause such an event. I am using flutter_map together with the flutter_map_marker_cluster plugin which e.g. zooms in if you tap on a cluster and thus changes the bounds, and I need to be able to react to that.

Therefore, I propose the following change to MapState._handleMoveEmit():

If source is custom, and targetZoom or targetCenter are different than before, a move event is emitted.

Thank you for your time and for flutter_map in general 😃

Copy link
Contributor

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

As an aside, that whole area of code (all the if statements), just makes me ponder why it wasn't written to just emit an event with a source and cut out loads of lines, but it would probably break usage to change all of that now. So I don't think I have a problem with this, it shouldn't affect anything else really.

@hschendel
Copy link
Contributor Author

I also tried out removing the condition that source must be custom, so that the event would be emitted for all event sources not covered before, whenever zoom or center changes. That broke the flutter_map test, so I removed it.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM as a fix, but maybe needs to be looked at in the long-term future. However, the analyser needs to be appeased before I can merge :)

@JaffaKetchup
Copy link
Member

JaffaKetchup commented May 12, 2022

Looks like some unrelated changes have caused the checks to fail, so I will have to explore this in more detail. I will override the checks this time, as this code seems to be OK :) @ibrierley, you happy for this to be merged?

@hschendel
Copy link
Contributor Author

I fixed the thing found by dart analyze in the example, so let's see how the pipeline likes this ;-)

@JaffaKetchup
Copy link
Member

Thanks, that seems to have fixed it. Will merge now :)

Many thanks for your contribution!

@JaffaKetchup JaffaKetchup merged commit 0f1d5c4 into fleaflet:master May 12, 2022
@hschendel
Copy link
Contributor Author

Thank you @ibrierley and @JaffaKetchup :)

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

Successfully merging this pull request may close these issues.

[BUG] Zoom change without center change does not emit move event if event source is custom
3 participants