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

Multiple Improvements & v5 Release Preparation #1532

Merged
merged 22 commits into from
Jun 4, 2023
Merged

Multiple Improvements & v5 Release Preparation #1532

merged 22 commits into from
Jun 4, 2023

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented May 25, 2023

Includes a major refactoring and reorganization with the aim of reducing the scope of responsibility for source files.

For changes to functionality and API, see the CHANGELOG. Note that this PR includes breaking changes.

Example application improvements:

  • Reorganized drawer menu
  • Added welcome dialog to example app when running at 'demo.fleaflet.dev' only
  • Removed unnecessary/bloat demo pages
  • Overhauled Marker Layer page
  • Improved other demo pages

Other meta changes:

  • Updated CHANGELOG and pubspec.yaml
  • Fixed GitHub not being able to interpret license
  • Add automated pub.dev publishing action on tag creation
  • Cut ties with leaflet.js in README
    I think we're starting to progress in our own direction, and I want to reduce the expectations for feature parity with it. Of course, I'm happy to look at it for inspiration as we sometimes do.

    It is true that the concepts of this library are similar, but our aim is not necessarily feature parity - we want to implement core features that there's high demand for, and don't really suit plugin architectures.
    This is partially because FM is on a much smaller scale than leaflet.js, and there's only 4 maintainers, so we rely on the community for a lot of feature additions - hence, we prefer plugins, as it also reduces the maintenance cost for us.

@JaffaKetchup JaffaKetchup changed the title [WIP] Assorted improvements preparing for v5 May 25, 2023
…esolves #1460, #777, #952)

Major refactoring and re-organization to improve understandibility
@JaffaKetchup JaffaKetchup changed the title Assorted improvements preparing for v5 Assorted improvements & v5 preparing for v5 May 29, 2023
@JaffaKetchup JaffaKetchup changed the title Assorted improvements & v5 preparing for v5 Multiple Improvements & v5 Release Preparation May 29, 2023
@JaffaKetchup JaffaKetchup marked this pull request as ready for review May 29, 2023 22:28
@rorystephenson
Copy link
Contributor

Hey @JaffaKetchup nice work!

Great to see the demo app too.

FYI I just found a bug I introduced in the v4 changes which is worth including in this: #1534 .

TBH it's probably worth cherry-picking and releasing as a hotfix on v4 as well because it breaks tileupdatetransformers that rely on movement ids (like the one for animated movement in the example app).

…`MapEventMove`

Originally from #1534 (3eca34e) - thanks @rorystephenson!

Co-Authored-By: Rory Stephenson <[email protected]>
@JaffaKetchup
Copy link
Member Author

JaffaKetchup commented May 30, 2023

Hi @rorystephenson,
Great spot! I've copied your fix into this manually in commit 434ce32 to avoid conflicts, and added you as a co-author.
EDIT: Not sure about cherry picking backwards for this, as it's still quite an unused feature and doesn't prevent movement - I'm happy for it to wait for v5.

@JaffaKetchup JaffaKetchup marked this pull request as ready for review May 30, 2023 10:33
@JaffaKetchup JaffaKetchup marked this pull request as draft May 30, 2023 13:12
@JaffaKetchup JaffaKetchup linked an issue May 30, 2023 that may be closed by this pull request
3 tasks
@JaffaKetchup JaffaKetchup marked this pull request as ready for review May 30, 2023 15:29
@JaffaKetchup JaffaKetchup requested a review from mootw June 1, 2023 08:13
Removed unnecessary Java installation from Windows GitHub Actions builder
Deprecated `AnchorAlign.none` in favour of `AnchorAlign.center` or `null`
Improved response/emission time of `onTap`/`MapEventTap` when `InteractiveFlag.doubleTapZoom` is disabled
Improved `MarkerLayer`/`Layer` interoperability
Improved/reorganized example application
Updated CHANGELOG
Improved documentation of marker anchor methods
Improved CHANGELOG
Copy link
Collaborator

@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.

I'm pretty ok with all of this, just a note, my mobile is bust atm, so just testing on web, so some bits like rotation I haven't tested, but don't see any obvious reason why that should be changed.

Main ponderings.

I think there's a few examples that have been removed, but I feel like some of them are handy, that keep cropping up like anchors and maxBounds, so wondering if they should be kept. Not sure the removal outweighs the "bloat".

Any reason for moving some core stuff to misc ? I don't really care tbh, just sometimes in general I find misc folders a kind of slack dumping ground :D.

Couple of bits that are making me scratch my chin, but I don't think are related to this PR (but maybe V4?), but wondering if we should double check before releasing...

The Reset Tile Layer example, doesn't seem to display the tiles after a reset. I don't think they did in very old releases, but then did in recent ones unless I'm mistaken...
I note in the example at https://demo.fleaflet.dev/#/ on my max two finger zoom doesn't work. I know there's been a few odd mac related things that probably aren't flutter_map specific, but may be worth double checking as well (it may just be me).

example/lib/pages/fallback_url_offline_page.dart Outdated Show resolved Hide resolved
lib/plugin_api.dart Outdated Show resolved Hide resolved
@JaffaKetchup
Copy link
Member Author

@ibrierley Thanks.

I think there's a few examples that have been removed, but I feel like some of them are handy, that keep cropping up like anchors and maxBounds, so wondering if they should be kept. Not sure the removal outweighs the "bloat".

I removed the following example pages:

  • Fallback URL for AssetTileProvider
    I originally removed this because I considered it to be similar to the same functionality for the NetworkTileProvider - happy to add it back if you think it would be better
  • Live Location demo & Live Location feature in Map Controller page
    This didn't work anyway, and there's much better solutions with plugins, and we don't have to deal with permissions
  • Network Tile Provider page
    With the changes made to the providers previously, this page is redundant and is the same as the Home page
  • Widgets
    There's nothing special anymore - since (I think?) v2, widgets can just be inserted directly into children, and I don't think this needs much demonstration

I consolidated the following pages into one new Markers page:

  • Tap To Add
  • Marker Rotation
  • Marker Anchor

Any reason for moving some core stuff to misc ? I don't really care tbh, just sometimes in general I find misc folders a kind of slack dumping ground :D.

That's precisely what it is! I would say the most 'core' part of this library is the widget and widget state. This was never in "core". 'core' suggests utmost importance to me, and things that are coupled everywhere, whereas 'misc' or 'utils' implies shared code but that is independent. All files in "misc" are independent (not closely related to FM state, etc.).

The Reset Tile Layer example, doesn't seem to display the tiles after a reset. I don't think they did in very old releases, but then did in recent ones unless I'm mistaken...

As far as I can see, the example is broken. I don't know when it broke, but I'm assuming it broke with the Tile Layer rewrites. But is resetting even useful anymore? Not sure myself.

I note in the example at https://demo.fleaflet.dev/#/ on my max two finger zoom doesn't work. I know there's been a few odd mac related things that probably aren't flutter_map specific, but may be worth double checking as well (it may just be me).

Mac gestures have been buggy for ages. I unfortunately can't check (don't have a touch screen Mac). I'm thinking that the next update should be a gestures cleanup and addition of rotation support with mice/cursor, but that's just a pipedream at the moment.

example/lib/main.dart Show resolved Hide resolved
Copy link
Collaborator

@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.

I think I'm fine with all of it, except I'd quite like maxBounds example to be retained (or included as part of another one).

@ibrierley ibrierley self-requested a review June 4, 2023 11:00
@JaffaKetchup JaffaKetchup merged commit 79b54e4 into master Jun 4, 2023
@JaffaKetchup JaffaKetchup deleted the jaffa branch June 4, 2023 11:01
Copy link
Contributor

@rorystephenson rorystephenson left a comment

Choose a reason for hiding this comment

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

Hey @JaffaKetchup one little comment/question. Sorry I didn't have time to review this and raise it before merging.

lib/src/layer/marker_layer.dart Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants