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

Add 'isFilled' field to Polygon class, fix issue with polygons doted border drawing and created example page for Polygons #501

Merged
merged 9 commits into from
Mar 26, 2022

Conversation

BaptistePires
Copy link
Contributor

@BaptistePires BaptistePires commented Jan 23, 2020

Hello,
First of all, thanks for this awesome package, I find it really usefull.

I added a new field in the Polygon class to allow users to fill or not the polygon, it's called isFilled (by default it's false, so it wont change the behavior of current applications that may be working with Polygons) and it's checked in the PolygonPainter class :

   final paint = Paint()

  ..style = polygonOpt.isFilled ? PaintingStyle.fill : PaintingStyle.stroke  

  ..color = polygonOpt.color;  

When I was doing that, I found that when I was drawing polygons with doted borders, the
last one was not doted, I fixed that by updating the method _paintDottedLine,
I only updated those lines :
for (var i = 0; i < offsets.length - 1; i++){
var o0 = offsets[i];
var o1 = offsets[i + 1];
Now it's :
for (var i = 0; i < offsets.length; i++){
var o0 = offsets[i % offsets.length];
var o1 = offsets[(i + 1) % offsets.length];
Thank's to that, when it's i + 1 it takes offsets[0] and draw the line between the last and the first point.

I also added an example page for the polygon as there was not.

Here are some screenshots to explain what I said :

If you need any other information, I'll provide it.

Thanks for reading !

The new field 'isFilled' in Polygon class allow you to chose if you want the Polygon to be filled when drawn. When I have adding this I also found that there was an issue with doted borders drawing. The last border was missing, I fixed it (_paintDottedLine method). Also added an example screen for the Polygon Layer.
@BaptistePires BaptistePires changed the title Add 'isFilled' field to Polygon class, fix issue with doted border draws and created example page for Polygons Add 'isFilled' field to Polygon class, fix issue with polygons doted border drawing and created example page for Polygons Jan 23, 2020
@johnpryan
Copy link
Collaborator

LGTM - could you take a look at the travis build failures and fix the merge conflicts?

@BaptistePires
Copy link
Contributor Author

BaptistePires commented Jan 31, 2020

Alright, I have a lot of stuff going on, I wont be able to fix this before feb 3

@BaptistePires
Copy link
Contributor Author

BaptistePires commented Feb 10, 2020

The conflicts seem resolved, but the Travis CI build (the one in the pull request, not the one you mentioned) fail does not seem to be related to mycode, I did not updated lib/src/layer/tile_provider/tile_provider.dart

@BaptistePires
Copy link
Contributor Author

I have checked again and I can't find why the Travis build failed, I ran dartfmt with Android Studio on the whole project, but nothing was updated

@johnpryan
Copy link
Collaborator

polygon_layer.dart has a new _paintBorder method, can you take a look at the merge conflicts?

The CI build is failing due to a lint rule flutter analyze should show the lint.

@kengu
Copy link
Contributor

kengu commented Mar 19, 2021

@Wyseh Can you resolve these conflicts? If not, should we close the PR?

@BaptistePires
Copy link
Contributor Author

I'll try to take some time to do it during this week but I currently have a lot of work, if it's not done within this week you can close it

@github-actions
Copy link

github-actions bot commented May 2, 2021

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 2, 2021
@S-Man42
Copy link

S-Man42 commented Jul 18, 2021

Any progress here? Would love to see this feature :)

@github-actions github-actions bot removed the Stale label Jul 19, 2021
@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Aug 19, 2021
@ibrierley
Copy link
Collaborator

This was a long time ago, but makes sense to me...is this still an issue and a PR that's wanted ? (If so I think you need to repull upstream I think and see if its still ok)

@S-Man42
Copy link

S-Man42 commented Jan 30, 2022

Still required. When implementing, you maybe think about #972 this as well (Border Color different from filled color)

@ibrierley
Copy link
Collaborator

Yes, some of the original poly options on that aren't too clear on that for me (maybe that's also a docs issue). I've previously wondered whether the polyline/polygon needs some further development that could be done as a plugin with new options and features (so as not to break the current usage), but maybe that's a good discussion to have.

@github-actions github-actions bot removed the Stale label Jan 31, 2022
@aytunch
Copy link
Contributor

aytunch commented Feb 12, 2022

Is this going to be merged friends?
This is a nice addition

@JaffaKetchup
Copy link
Member

@BaptistePires Can you pull upstream so I can take a look? Thanks!

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Mar 15, 2022
@S-Man42
Copy link

S-Man42 commented Mar 15, 2022

Any progess here?

@JaffaKetchup
Copy link
Member

Just need @BaptistePires to pull upstream, then ready for review.

@BaptistePires
Copy link
Contributor Author

Hi, I've been very busy this year, and I really didn't have time to take care of this, I'll try to get it done by the end of the week. Sorry for the inconvenience.

@JaffaKetchup
Copy link
Member

No problem!

@BaptistePires
Copy link
Contributor Author

I resolved the conflicts but I wasn't able to test it yet, I'll come back as soon as I've tested it.

@JaffaKetchup
Copy link
Member

It appears not all files are formatted correctly, can you run dart format . --fix just to make sure (when you're ready). Many thanks!

@BaptistePires
Copy link
Contributor Author

BaptistePires commented Mar 24, 2022

As this PR is quite old, I'll have to update my code to make it work with new attributes and methods. As I said above, I wasn't able to test it yet, I'll do this as soon as I can.

@BaptistePires
Copy link
Contributor Author

I was able to test it and it's working on my device

@JaffaKetchup
Copy link
Member

Thanks, I'll test it soon as well, and hopefully I can merge.

@JaffaKetchup
Copy link
Member

I am testing right now. Quick question, did you have problems compiling the example app for Android? I don't think it's your fault, but I can't do it for some reason. (I've noticed it before as well.) Using web for now.

@JaffaKetchup JaffaKetchup self-requested a review March 26, 2022 09:18
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. I'm not sure why the Android app isn't compiling for me, but that can be fixed for another time.
Will merge now. Thanks for your contribution!

@JaffaKetchup JaffaKetchup merged commit 261dbe5 into fleaflet:master Mar 26, 2022
@BaptistePires
Copy link
Contributor Author

Yes I had issues related to the minSdkVersion iirc, I thought it was because my environment was a bit old.
Thanks a lot!

@sjoulbak
Copy link

sjoulbak commented Jun 8, 2022

@JaffaKetchup While upgrading this package from 0.14.0 to 1.0.0, we found out that our polygons weren't filled anymore by default. Perhaps it could be helpful to add this to the changelog or change the default value of isFilled from false to true?

@ibrierley
Copy link
Collaborator

Yes, good spot. I don't think we should change this now though. Updating to 1.0.0 should be ok to have breaking changes..it just shouldn't have had a change before that, and I guess changing it back will introduce a breaking change again when there shouldn't be any :D. (I think I sound as clear as mud on that).

It should get added to changelog though (and we should try and watch out for things like that in the future better).

@JaffaKetchup
Copy link
Member

Yeah when I was doing the changeling I did include this change but didn't completely specify it. I'll add it to the migration instructions in the new docs.
Thanks for the heads up!

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

Successfully merging this pull request may close these issues.

8 participants