Closed HPLegion closed 2 years ago
One more thought: The artist could inherit from Annotation instead of Text. Annotations have slightly more features (e.g. arrows), but vice versa that could run the risk of just reinventing the wheel and making stuff more complicated. We also don't know if users already have further processing in place that assumes a Text Artist.
Hi Hannes! Thanks for the PR!
Note that I am currently on holidays so I won't be able to review this till 2022.
22 Dec 2021 17:21:55 Hannes Pahl @.***>:
One more thought: The artist could inherit from Annotation instead of Text. Annotations have slightly more features (e.g. arrows), but vice versa that could run the risk of just reinventing the wheel and making stuff more complicated. We also don't know if users already have further processing in place that assumes a Text Artist.
— Reply to this email directly, view it on GitHub[https://github.com/cphyc/matplotlib-label-lines/pull/74#issuecomment-999698569], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABJJII56CDTNEDZZZ775E7DUSH2XLANCNFSM5KN326MQ]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. You are receiving this because you were mentioned. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACgAAAAoCAYAAACM/rhtAAAABHNCSVQICAgIfAhkiAAAABxJREFUWIXtwQEBAAAAgiD/r25IQAEAAAAAAHwaGSgAAaRY3EgAAAAASUVORK5CYII=###24x24:true###][Tracking image][https://github.com/notifications/beacon/ABJJII5YRTMKTZLMCSSEBX3USH2XLA5CNFSM5KN326M2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHOLDBCI.gif]
Hi, thanks for your review :-). It will be a few days before I can work on the changes, but none of them look too terrifying :-P
Hi, just FYI that this is still on my TODO list, I am just quite busy right now :)
No rush!
@HPLegion I took the liberty to resolve merge conflicts and get this in the main branch (and released as of v0.5.0!).
Thanks for the valuable contribution :)
Hi, thanks, glad it's in :)
Sorry that I did not get around to comment on your remarks. I think I had just been uncertain about using |
in types, but I guess that is covered by the future import. And with the property I must have gotten confused by the matplotlib practices since they don't use properties in the usual Pythonic way for historical reasons.
No worries - I'm glad your changes have been merged it, this really makes the code better.
Hi @cphyc
Recently, I saw that there were suggestions in the issue tracker to develop a custom Artist for line labels.
I have given it a shot and I think I am quite happy with the result (it is my first time working with MPL Artists though, so I am more than happy to have someone give this a careful review)
Thanks to some hints I found on StackOverflow (https://stackoverflow.com/a/53111799) the labels now autorotate when the plot is rescaled (
at least it works on my machine in an interactive plot window ;-)EDIT: I have now added a test case for this behaviour ), so it should fix #36I have also used the chance to try and simplify the code a little bit by knocking out obsolete edge cases and making better use of matplotlibs internal conversion instead of rewriting existing code.
Please let me know what you think, I won't be offended if you think this is too big a change.
I have subtly changed the behaviour of the pure Artist compared to the labelLine function, but users of the labelLine function should still experience the same results and control flow as before.
I feel like there is some additional functionality there that should not be part of the Artist itself, otherwise things become too convoluted.EDIT: In hindsight I realised that the outline effect should really be part of the artist, but I think the drop_label feature should not be, I don't think matplotlib typically "consumes" information anywhere.I had to change a few testcases as well, but I think the new baselines actually look better than before, and one test case was broken because of changes in matplotlib as far as I can tell.
EDIT: I have also added a test case for vertical lines
Cheers!