-
-
Notifications
You must be signed in to change notification settings - Fork 868
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
Creating centered label for polygons #1220
Conversation
Will have a look at this today. Am I right in thinking you're talking about #800 trying to do the same? |
Haven't had chance to test yet, will try to in the next day or so... A few thoughts, although most of these are kind of possible future additions, rather than the specifics of this PR (but feel worth mentioning now), maybe worth bearing in mind as it's hard to move code about later. Most of these would probably involve a fair bit of work though, so may not be worth holding up a PR for them... a) Would it be more useful to have the main label code outside of the polygon class/code (but possibly called from poly_layer) ? I think a labelling solution could be handy for other classes to call (eg some use polylines with onTap plugin iirc, people may want to write their own poly code), and may be good to have one central solution that becomes more developed and may make b) & c) feel more natural rather than having lots of poly options, which are really label options...) ? b) If I'm reading the code right, if the poly isn't wide enough, the text won't display (maybe there could be an option to allow text overlap..or make smaller to fit?). I'm not sure if that feels a bit odd...,but happy to be convinced on that... c) What happens when zoomed in, does the text get smaller each time, or stay the same...could that be an option? d) TextLayouts are typically quite expensive performance wise iirc, so it may be useful to cache them up and reuse or something. e) What should happen if flutter_map is rotated...should text counter-rotate, so it's always upright (or maybe it could be an option passed in) ? f) Typically I would draw labels on a separate layer (so all are on the top), in case of overlapping polys, would text be overlapped, so only partially visible ? I can see both being valid here.... As mentioned, these are mainly future thoughts...it could be that we want to allow one smaller PR in the interim, and develop a slightly more involved solution separately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great PR :). There are a few rough edges that I think are a bit odd or need to be changed, as @ibrierley mentioned.
-
When text overlaps the polygon's edges, the text becomes hidden behind the border. Also, should text be allowed to overflow at all? See this screenshot:
This needs to be fixed by:
a) Ensuring text remains above border paint
b) Cutting text as it overlaps the border (maybe an option?) -
Currently, text remains the same size based on screen size, not current zoom level - it matches the behaviour of markers. See this screenshot, and compare to the last:
I'm not sure if this is the behaviour we want, so maybe it could be made into an option. However, this may have severe performance side effects, so more attention is needed. -
Text does not rotate with the screen. See this screenshot of a different rotation:
Again, I'm not sure of the behaviour we want here. So maybe an option would be good - not sure about performance though. -
There is no example of using these labels. This is an easy fix, just need to edit the existing polygon page and add a few labels.
I haven't tested performance here, so I cannot make any remarks about that.
@ibrierley's point (a) makes sense as well, but I'm not sure how much effort and refactoring this will take, so this will need to be discussed at more length.
Again, thanks for this great start, I just needs a bit more work before it can be merged.
(I will close #800 in favour of this)
R.e refactoring, we could probably simply create a new label.dart file, with
and call with...
Then anyone else can call Label.paintText(canvas, polygonOpt.offsets, polygonOpt.label, polygonOpt.labelStyle); whenever they want ? And if labels get developed more, which does make sense, all the code could go in there ? |
For this one, you can simply move the paintText after the _paintBorder command eg...
Maybe points 2&3 are ok, as long as people are aware, i.e it's not causing any performance problems on anything that already exists. |
Had a chance to test, I like this PR, especially if it has the tweaks mentioned (separate file and paint call after border paint). I think it's inevitable with some weird shapes there could be overlaps, and the rotation feels fine really as all the other text is aligned. Anti-rotated text is a nice to have, but shouldn't hold back the PR I think. Performance wise....I think it should be ok as is, as I suspect no one will try and add that many labels, as otherwise you will get lots of overlapping labels looking messy, and performance tweaks can be added later. (Maybe there could be more issues on a large display like desktop/web) Overlapping Polys (so one polygon will cut off the text of a poly behind it), is a little odd, but I think either one needs collision detection, or draw all labels on the top layer, but I guess that's for a future update. I can see in the future there may want to be further more complex updates, like rotation, collision detection, hence maybe it needs its own class/file. |
I did some of the tweaks mentioned that I thought were important, as for collision detection, rotation and some other problems like overlapping polygons I won't be able to solve right now, so I think it would be nice to think about it on further updates. |
Looks good, will give it a test tomorrow. Thanks for all your work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for the help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the great PR: we've been needing this for a long time.
I'll add it to the new documentation, and I'll merge this now.
Someone did a PR with centered labels but didn't finish it.
I did a few tweaks to use it in my project but I think it would be a good addition to the package