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

Fixed empty text bbox drawing. #2710

Closed
wants to merge 2 commits into from

Conversation

pelson
Copy link
Member

@pelson pelson commented Jan 8, 2014

Closes #2698

@WeatherGod
Copy link
Member

Before we go ripping out this sort of stuff, let's find out why it was
there in the first place. Digging through blame, looks like the last
substantial change to those lines was done by @mdboom a couple years ago:
pelson@127f474#diff-6

Thoughts?

@WeatherGod
Copy link
Member

Traced it back to a commit by @ivanov to fix #1583 :
pelson@3b857ab

@coralnut
Copy link

coralnut commented Jan 8, 2014

I don't know if this information is helpful or not, but here goes:

The type of code that I'm having a problem with in #2698 only started to be a problem since I updated my PC from Fedora 18 to Fedora 20. That is to say, the version of python-matplotlib that was included in F18 handled the code just fine, and the version of python-matplotlib that is included in F20 is what is having problems.

I can tell you that the version that I am having problems with in F20 is python-matplotlib-1.3.0-1.fc20.x86_64.
I can't tell you the number of the version that was included in F18 that worked properly.

Since I performed the update from F18 to F20 I have lost the ability to look on my PC to determine the version of python-matplotlib that handled the code properly. I have tried looking backwards in the Fedora package database to find the version of python-matplotlib that was included in F18, but I haven't been able to extract the version information. Sorry I can't be more helpful.

Before I get too long winded, my point is to say that I would be reluctant to point the finger at the most recent change that took place 2 years ago, as the problem just came along with the F18 to F20 upgrade, and it would seem (to me at least) that this is a problem with a change in the code that is more recent than a couple of years ago. Of course, if Fedora is really slow about updating their code, then I guess that these changes could have occurred a couple of years ago, and they finally found their way into F20.

I'm just trying to help elucidate when the problem came along, if any of this helps.

Thanks for your time.

@WeatherGod
Copy link
Member

I appreciate you taking the time to try and figure out when the problem
cropped up. I should note that you are looking at final releases, whereas I
was looking at actual commits. There was a long time between v1.1, v1.2 and
v1.3. The commit in question was done to fix an issue with rendering with
usetex=True that checked to see if a string was completely whitespace
instead of checking to see if it was just an empty string (whitespaces are
not relevant in tex, thereby causing issues).

If I am recalling my timeline correctly, this change would have come in
right around the 1.2 release, so it is possible it missed the 1.2 release,
making it seem like it was a recent change to you. My analysis and
understanding of the relevant code tells me that this is the cause.

I see two questions at this point: 1) Should we consider this a regression?
(neither the old nor the new behavior are documented, nor are there tests
to catch this, AFAIK) and 2) if we do want to restore the old behavior, how
do we do it such that #1583 does not come back?

@coralnut
Copy link

coralnut commented Jan 8, 2014

I'd like to offer my two-cents regarding whether the old behavior should be restored:

In my case I am trying to draw a plot legend that prints several lines of text information in different colors, in a pop-up box that floats over the data area of a chart. I am using an alpha setting of 0.5 so that any data that is plotted on the chart in the layer below the legend does not over-write the box.

To execute this objective (described in #2698) required that I make separate text writes to the plot, and then draw an empty pop-up box beneath them. The reason for this is that it's not possible to construct a multple-line string comprised of muti-colored lines of text, and pass the concatenated string to the box-drawing function and get the desired output. When one attempts to concatenate a string that is comprised of many different colored lines of text, the box-drawing routine responds by setting the entire block of text to the last-specified line color. The workaround for this problem is to draw the lines of colored text independently, and to then draw an empty box around them. Unfortunately, the new program behavior keeps this from happening, as it prevents an empty box from being drawn.

My personal opinion, for whatever it may be worth, is that it would be better to revert to the old behavior for these reasons:

a. Allowing whitespace strings would provide users with a method of drawing empty boxes.
b. It would make my code work again. ;)
c. It would make matplotlib act in compliance with the python standards for fill characters, which state that any character other than "{" and "}" are legitimate fill characters.
d In Python, whitespace is a legitimate string type. ( see "string.whitespace") It would make sense for matplotlib's box drawing function to honor legitimate python strings.

Thanks for your consideration.

@pelson
Copy link
Member Author

pelson commented Jan 9, 2014

Before we go ripping out this sort of stuff, let's find out why it was there in the first place.

Fair point. The truth is though that if a change is being made then I would have expected a test to be added - alas it wasn't.

I've now added a test case for that which was being highlighted in #1583 and I believe I've now fixed the problem at the source.

@tacaswell
Copy link
Member

@pelson This needs a re-base.

@pelson
Copy link
Member Author

pelson commented Jan 22, 2014

Closing - this is a bit of a headache. I'll keep the branch around for the next few months in the hope that I can take another look.

@pelson pelson closed this Jan 22, 2014
@coralnut
Copy link

I don't understand. Is the string parsing error something that is impossible to fix?

@coralnut
Copy link

coralnut commented Apr 2, 2014

Anybody?

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.

ax.text() fails to draw a box if the text content is full of blank spaces and linefeeds.
4 participants