ibrierley / flutter_map_line_editor

A basic line/poly editor that works with flutter_map and dragmarkers.
MIT License
44 stars 25 forks source link

Support for `flutter_map` version `4.0.0` #34

Closed josxha closed 1 year ago

josxha commented 1 year ago

This pull requests adds support for new new major version of flutter_map by implementing the changes suggested in https://github.com/fleaflet/flutter_map/pull/1455#issue-1607273266.

Additional changes:

I tested the pull request on my android device where it worked well.

ibrierley commented 1 year ago

Thanks for this, really appreciate it, as I haven't had much time of late. Will try and just get it checked over the next day or two.

ibrierley commented 1 year ago

Just tested, I'm fine with all of this thanks ! Are you ok to make the suggested changes ?

josxha commented 1 year ago

Thanks a lot for the code review and testing @pablojimpas! I added your requested changes. πŸ‘

I would also like to suggest to add a selection of platforms integrations to the example project or alternatively to add these directories to git ignore.

Arguments for a push:

Arguments for a gitignore :

pablojimpas commented 1 year ago

I would also like to suggest to add a selection of platforms integrations to the example project or alternatively to add these directories to git ignore.

Most Flutter packages include a full example app with all (or at least some) platform-specific projects in their repository. I think it's a good practice to have both a full example app and a small example (as minimal as possible) showcasing how to use the package in a way that can be easily copy-pasted into the users' app.

My general idea will look like this:

example
β”œβ”€β”€ demo_app
β”‚Β Β  β”œβ”€β”€ analysis_options.yaml
β”‚Β Β  β”œβ”€β”€ android
β”‚Β Β  β”œβ”€β”€ ios
β”‚Β Β  β”œβ”€β”€ lib
β”‚Β Β  β”œβ”€β”€ linux
β”‚Β Β  β”œβ”€β”€ macos
β”‚Β Β  β”œβ”€β”€ pubspec.lock
β”‚Β Β  β”œβ”€β”€ pubspec.yaml
β”‚Β Β  β”œβ”€β”€ README.md
β”‚Β Β  β”œβ”€β”€ web
β”‚Β Β  └── windows
β”œβ”€β”€ main.dart
└── README.md

With this approach, the contents of example/main.dart will appear in the example tab for everyone to copy, and then we will maintain more detailed and complex examples under the β€œfull example” demo app.

I will prepare a PR for this suggestion if @ibrierley agrees. Furthermore, I would like to make a PR for flutter_map_dragmarker syncing the changes made here. And then, another PR to this repo with the aim of using dragmarker.dart as a pub dependency rather than including the file directly into this repo and maintaining it in two different places.

ibrierley commented 1 year ago

Heya, I'm a bit unsure what the extra stuff brings, do you mean you can just install the app, rather than running it from your IDE or something ? (I'm only really familiar with the normal IDE integration of Android Studio etc). I don't have any issue with extra files generally, but if it's more maintenance, then it's more of an issue (I don't have a mass of time atm so a bit wary, but I'm also happy to add any collaborators or whatever for others to be able to do more !).

For the dragmarker sync, I'm fine with that, but double check it all, as iirc there was some issue where I ended up making the line editor not dependent on dragmarker plugin. It was a long while ago though, I think it was related to the retaining of state with different dragmarkers. I have a feeling this is fixed potentially after I added Keys to dragmarker, just you may find you need to pass a Key through to each Dragmarker or something similar (may have been related to something like this PR

pablojimpas commented 1 year ago

Heya, I'm a bit unsure what the extra stuff brings, do you mean you can just install the app, rather than running it from your IDE or something ? (I'm only really familiar with the normal IDE integration of Android Studio etc). I don't have any issue with extra files generally, but if it's more maintenance, then it's more of an issue (I don't have a mass of time atm so a bit wary, but I'm also happy to add any collaborators or whatever for others to be able to do more !).

Having a folder for each specific platform allows anyone to just execute flutter run to launch the example app instead of recreating the project first with flutter create . and overall brings a faster development cycle for contributors since they can test their changes directly. I doubt that it creates a lot of maintenance effort, since those platform-specific files are only created once and rarely touched again (unless you're actually building a real production app).

I am happy to help you and become a maintainer of both this package and flutter_map_dragmarker since I'm relying on them heavily in my app, so I want to see them thrive. Talk to me privately on Discord or elsewhere if you find this appealing.

For the dragmarker sync, I'm fine with that, but double check it all, as iirc there was some issue where I ended up making the line editor not dependent on dragmarker plugin. It was a long while ago though, I think it was related to the retaining of state with different dragmarkers. I have a feeling this is fixed potentially after I added Keys to dragmarker, just you may find you need to pass a Key through to each Dragmarker or something similar (may have been related to something like this PR

Got it! I will double-check the history of both repos to make sure that I don't fall into the same issue.

ibrierley commented 1 year ago

Ok, that all sounds great. Is this waiting on anything else now, otherwise I can test and merge. (I'm guessing the rest will be part of a separate PR). I'll ping you via email (happy to chat on Discord if easier as you mention), or if anyone else is interested as well, let me know.

pablojimpas commented 1 year ago

Yes @ibrierley, the other stuff will be part of future PRs, this one is ready as is. You are welcome to give it a last test and merge it.

ibrierley commented 1 year ago

Just thinking, won't we ideally want to change the version of line_editor now, eg to 5.0 ? Then it will be easier for some to resolve issues when it gets published ?

josxha commented 1 year ago

Just thinking, won't we ideally want to change the version of line_editor now, eg to 5.0 ? Then it will be easier for some to resolve issues when it gets published ?

I don't think that a major release is necessary because of it's backwards compatibility. A minor release should be enough since there are no breaking changes?

...btw is there any discussion going on in Discord? May I join the discussion?

ibrierley commented 1 year ago

Good question actually, I'm a little unclear on dependency changes. We need it to update, for pub.dev new version I think...as to whether its a major or not...it's probably fine without, as the major changes are with flutter_map alone which has updated its major...

There's no discussion currently on Discord, I'm happy to discuss anything there, or I typically use email more often (can use my github username @gmail.com if anyone ever needs), or ianb on flutter_map.