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

[FEATURE] Re-instate Anchor on Marker #1552

Closed
rorystephenson opened this issue Jun 10, 2023 · 5 comments · Fixed by #1558
Closed

[FEATURE] Re-instate Anchor on Marker #1552

rorystephenson opened this issue Jun 10, 2023 · 5 comments · Fixed by #1558
Labels
feature This issue requests a new feature

Comments

@rorystephenson
Copy link
Contributor

What do you want implemented?

I'm opening this issue to continue a discussion I started here: #1532 (review)

In flutter_map 5.0.0 Marker's Anchor was replaced with AnchorPos. This was done for consistency with the newly added option to define AnchorPos at the MarkerLayer level. There are, however, two drawbacks to this approach:

  1. The Anchor is calculated every time the Marker is built, previously it was pre-calculated when creating the Marker instance.
  2. The default AnchorPos for a Marker is ambiguous for other packages that use Marker. The AnchorPos is nullable and null by default which makes sense to allow for the fallback to the MarkerLayer anchorPos. Unfortunately this means that other packages that use Marker need to check the source of MarkerLayer to determine what the default AnchorPos is and if that ever changes there will be inconsistencies.

What other alternatives are available?

  1. Re-instate the old behaviour. The pros are less calculations when building markers and the default AnchorPos is clear. The con is that it's no longer possible to define a default anchorPos at the MarkerLayer level which is clearly convenient given that for apps that don't have a huge number of Markers the performance impact of doing so will be zero.
  2. Change Marker's AnchorPos? to Anchor? (previously it was non-nullable) and define a AnchorPos.default (which just redirects to the default value). The pros are that users who are concerned about the performance impact can define the anchor at the Marker level, and the default AnchorPos is not as clear as a default parameter but it's clear enough. The cons are that Marker.anchor will be inconsistent with MarkerLayer.anchorPos and it may not be obvious that defining at the MarkerLayer level can increase calculations. I think that both of these can be mitigated by good documentation.

Can you provide any other information?

@JaffaKetchup I've elaborated on a couple of options. Let me know what you think.

Severity

Annoying: Currently have to use workarounds

@rorystephenson rorystephenson added the feature This issue requests a new feature label Jun 10, 2023
@JaffaKetchup
Copy link
Member

I think the best option out of those is the first one. The second one is too complex for users to understand.

I don't think the re-calculation will be much of an issue, but I can imagine your second point about ambiguity being potentially annoying.

However, I also wonder whether something like this would be OK? Unless I'm completely missing something, this wouldn't require a new calculation for every marker build, and it solves the second point.

// In constructor
    AnchorPos? anchor,
}) : _anchor = whatever(anchor);

// In class
final Anchor? _anchor;
Anchor get anchor => _anchor ?? layerAnchor;

@rorystephenson
Copy link
Contributor Author

@JaffaKetchup I think what you proposed is similar to the option 2 I suggested. See this branch for what that would look like: https://github.com/fleaflet/flutter_map/compare/master...rorystephenson:flutter_map:restore-marker-anchor?expand=1

MarkerLayer keeps the anchorPos option rather than anchor because anchor requires the width/height of the Marker which you can only know at the MarkerLayer level if they are all the same.

Alternatively we could revert to the old behaviour of only setting the anchor/anchorPos at the marker level.

@JaffaKetchup
Copy link
Member

@rorystephenson I think it's similar. The difference is the user still defines the Marker/Layer with an AnchorPos instead of Anchor, and AnchorPos.default wouldn't be required - it's just null.

@rorystephenson
Copy link
Contributor Author

@JaffaKetchup ahhh I misunderstood what you were proposing, much better!

As for the default value I've still included it because it is required by plugins which need to know what a Marker's anchor will be. For example flutter_map_marker_popup needs to know the anchor because it can position popups relative to the Marker. Adding a default value allows any change in the default to be automatically updated in packages which rely on it. See #1558.

@JaffaKetchup JaffaKetchup linked a pull request Jun 19, 2023 that will close this issue
@JaffaKetchup
Copy link
Member

Closed through merging of #1558. Thanks @rorystephenson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue requests a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants