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

refactor: merge MapInternalController with MapControllerImpl #1738

Merged
merged 7 commits into from
Dec 7, 2023

Conversation

josxha
Copy link
Contributor

@josxha josxha commented Nov 26, 2023

Motivation

  • FlutterMapImpl has become an internal object in flutter_map version 5. Since then there are currently two abstration levels of the map controller (MapController -> MapControllerImpl -> MapInternalController).
  • Most of the time MapControllerImpl calls its underlying MapInternalController and passes it's arguments though in addition to setting some defaults.
  • Two internal controller layers require complex bindings to glue them together.

Changes in this pull request

  • Merge the MapInternalController with MapControllerImpl
  • Allow "raw" methods (naming inspired by the flutter sdk) to be called internally with all parameters while still hiding them from the public API.
  • Remove FlutterMap prefix, use a shorter Map prefix instead
    • Rename FlutterMapInteractiveViewerto MapInteractiveViewer
    • Rename FlutterMapNetworkImageProvider to MapNetworkImageProvider
    • Rename FlutterMapInheritedModel to MapInheritedModel

Roadmap

  1. Refactor the map controller logic (this PR)
  2. Add animation logic to the internal map controller (follow up PR)
  3. Continue work on multi!: rewrite gesture handling #1733 that needs to have animations.

@TesteurManiak
Copy link
Collaborator

I don't mind the name changes, but I would still recommend to create deprecated typedefs for the old names just to not instantly breaks code from the previous version and inform users that they should migrate to the new names, for example:

@Deprecated('Use MapInteractiveViewer instead')
typedef FlutterMapInteractiveViewer = MapInteractiveViewer;

@josxha
Copy link
Contributor Author

josxha commented Nov 26, 2023

Typedefs are a great idea for deprecations.👍🏼 Do we need to deprecate internal code though? 🤔 The renamed classes are not exported and therefore not directly exposed outside of the package.

@TesteurManiak
Copy link
Collaborator

Typedefs are a great idea for deprecations.👍🏼 Do we need to deprecate internal code though? 🤔 The renamed classes are not exported and therefore not directly exposed outside of the package.

True if all classes are internal, no need for deprecation. I thought that some of them were exposed, but if it's not the case then don't bother 👍

@JaffaKetchup
Copy link
Member

It's probably worthwhile reading through the history of these objects, in the discussion at #1548 and the #1551 PR, which I think are related, and might explain the reasons why these are separated like they are. Not against this PR as such, just been a lot of messing around in this area 'recently'.

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.

I think it's pretty much good (except for my point above) except for these couple of things.

IMO in general, I prefer to avoid changing names when it doesn't add clarity or better match what it's assigned to. But I realise that's just my personal way of doing things, so I won't let that block this.

@josxha
Copy link
Contributor Author

josxha commented Nov 28, 2023

It's probably worthwhile reading through the history of these objects, in the discussion at #1548 and the #1551 PR

Thanks for the links! In that case we should probably invite @rorystephenson to this discussion as well.
I try to summarize some points while reading along:

  • Have MapController as public API and hide internal methods and variables from external and plugin access.
  • "MapControllerImpl is given direct access to another internal controller which manipulates the map state." (Sadly the reasons for this haven't been discussed further.)
  • Making MapController a sealed class has been mentioned but not implemented, maybe worth adding here.
  • We want to have an immutable FlutterMapState
  • "All changes to the map state are [...] managed by [the] FlutterMapStateController."
  • "MapController is a completely separate concept from a widget's state. The controller is intended to let you control the state, not access and directly manipulate it, which is important in reactive programming. You want to be able to tell the map to "move here" or "change the camera to these bounds" and then react to any state changes that result (if you desire). Directly manipulating the state, for example to change it's center/zoom value, creates coupling.

As far as I can tell this PR doesn't violates the principles mentioned in the other discussions. This pull request doesn't even breaks public APIs, it's all internal.

The concept of two internal layers for the controller has indeed been introduced in #1551 but was not mentioned in the discussion. @rorystephenson, could you give some details about the design choice? I'm curious to hear what you think about this PR too, maybe I'm overlooking something here.

@josxha josxha added this to the v7.0 milestone Dec 2, 2023
@JaffaKetchup JaffaKetchup self-requested a review December 6, 2023 18:03
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.

If we're sure this isn't impacting any advanced use-cases where people might be implementing their own highly-specialised controller, I'm OK with this.
If this isn't breaking, does it need the ! in the title?

@josxha
Copy link
Contributor Author

josxha commented Dec 6, 2023

If we're sure this isn't impacting any advanced use-cases where people might be implementing their own highly-specialised controller, I'm OK with this.

I'm quite confident here because of the changes made in #1551 (users don't extend from MapController but use it as parameter like flutter_map_animations)

If this isn't breaking, does it need the ! in the title?

Oh, yes that's correct - changing it.

@josxha josxha changed the title refactor!: merge MapInternalController with MapControllerImpl refactor: merge MapInternalController with MapControllerImpl Dec 6, 2023
@josxha
Copy link
Contributor Author

josxha commented Dec 7, 2023

hi @JaffaKetchup I merged the master branch.
Additionally i wanted to use radians2Degrees instead of radianToDeg() first but then removed it again in favor of #1763 because it's off topic. Do you want to have another quick look at it before we merge?

@JaffaKetchup
Copy link
Member

I think it's all good if that's the only thing that's changed!

@josxha josxha merged commit 5c122f0 into fleaflet:master Dec 7, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants