Hi everyone,

I was just wondering whether someone would be willing to have a look at this PR? I feel like it's in a complete or nearly-complete state but need some guidance - the two failing CI jobs seem to be:

• PR cleanliness:
  The added unit test was written first by @Abitamim, and then expanded by myself. When I expanded the test to check the newly-added reversed arrow heads, I changed both the test name (temp_test_boxarrow → test_boxarrow_adjustment) and the name of the output image (roadsign_test_image.png → boxarrow_adjustment_test_image.png) to reflect this. The 'file both added and deleted' error given seems to be a result of adding then deleting roadsign_test_image.png.
• Python 3.11 on macos-13:
  The failing test here (test_blitting_events) is marked as flaky, with the comment # subprocesses can struggle to get the display, so rerun a few times. I feel it may be that this was just a random instance of the test failing multiple times in a row; unfortunately I don't have access to macos-supported hardware for rerunning this locally, so I was wondering if someone would be able to take a look.

Any help would be greatly appreciated. Thanks!

PR-cleanliness: This is a check so that we don't merge commits with unneeded images, which would increase repo size. You can either squash and force-push. Or ignore this and we'll squash-merge at the end.

Ignore the macos test failure. It's unrelated.

Thank you for getting back so quickly! Would it be okay if you handled the squashing - I'm relatively new to git, and I'm not sure of the best method to use for squashing to keep a relatively clean but traceable commit history?

Since I'm going to need to make some relatively major changes to the contents of this PR (a redo of the arrow drawing code and a rewrite of its unit test), would it be better for me to amend my last commit (d9b0b32) as suggested in the contributing guide, or make a few new commits? I'm not quite sure how to add more commits to an open PR - do I just push the changes to my development branch?

@CharlieThornton33 yes you can just push more commits to your current branch and they will appear in this PR. Since you already have several commits they will ultimately need squashing one way or the other (and I see you already asked that we handle that), so I do not think you gain anything from the —amend option.

I've just redone the padding (only for LArrow and RArrow for the moment), and I think the issue with the text escaping the outline has been fixed. I set the arrow body to dynamically increase in length to make sure there's always a gap of at least pad between the edge of the text closest to the arrow head and the position where that edge would first intercept the outline if moved towards the tip:

import numpy as np
import matplotlib.pyplot as plt

fig, ax = plt.subplots(figsize=(3, 8))

t0 = ax.text(0.5, 0.9, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=-90",
                      fc="lightblue", ec="steelblue", lw=2))

t1 = ax.text(0.5, 0.7, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=70",
                      fc="lightblue", ec="steelblue", lw=2))

t2 = ax.text(0.5, 0.5, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=100",
                      fc="lightblue", ec="steelblue", lw=2))

t3 = ax.text(0.5, 0.3, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=130",
                      fc="lightblue", ec="steelblue", lw=2))

t4 = ax.text(0.5, 0.1, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=170",
                      fc="lightblue", ec="steelblue", lw=2))

plt.show()

It's also a bit concerning that the CI workflow seems to run on every commit to this PR - am I okay to disable tests in my commit messages until the one where I write the new test to save on CI resources?

I've just noticed - all of the pytest test runs are about to fail because I forgot to remove the flawed test_boxarrow_adjustment test. Is there any way to cancel a CI test run?

@CharlieThornton33 I wouldn't worry about the CI failing, but if they are still running when you push another commit they will be interrupted and re-started.

Hi,

I'm afraid I won't be able to finish this PR for a few weeks - I'm currently in exam season at university and I'm having to focus on them at the moment. As soon as I've finished exams (mid June), I'll get back on it and finish the rest of the implementation.

That's ok.

I'm going to try to write a new unit test tomorrow, then hopefully this should be okay to merge.

Hi,

I'm unsure about why the two Windows tests on Azure failed, and I don't think it's likely to be a direct result of my addition of test_boxarrow_adjustment.

The python 3.12 job failed test_fontcache_thread_safe, seemingly by some sort of timeout, and a child process appears to have lingered a bit after bash was stopped.

The python 3.13 job seemed to be slightly confused with versions - it was trying to link a free-threaded version of python at build time during C/C++ compilation (python313t.lib), but the only installed version in this CI job is the non-free-threaded python313.lib.

I'd really appreciate any help/guidance; apart from these failing jobs, I'm quite happy with the feature implementation as it is now and would be really grateful if someone could have a look and see whether it's okay.

I think this should be all of the remaining issues fixed - I rectified the padding for reversed arrow heads (including a missing + dxx in the trapezoidal case), and updated test_boxarrow_adjustment accordingly.

Sorry, I'm quite scrace with time right now. Proper review will have to wait for some more days.

From a quick look: Do we get into trouble with these if the heads become even smaller?

There's no margin, so for very mall heads, the box line will touch the text. Can we add a margin here as well? Argument: Concerning space for text, these are basically like the flat versions

because the tip is generally only a narrow extension, into which text does not fit.

Also please check what the edge case

is / should be doing. If we scale the overshoot in these cases to 0
, the second one (full margin) is still ok, the first one (negative) may be a bit tight - OTOH we may not need the full margin?

Hi everyone,

I was just wanting to get some input on how I should handle padding; should the margins around text form a complete rectangle? Currently, only a rectangle of width pad between the text and the arrow-head is guaranteed clear, and this can lead to some really close calls where text almost (and in some circumstances possibly could) leaves the arrow boundary. I feel like the most logical solution is to make sure the area shown dashed below lies fully inside the arrow, but I'd like to see if anyone has any better ideas.

Here are the options for regular arrows with different angles

1. tip base is at text + pad
2. tip base is at text
3. tip point is at text + pad
4. tip is moved so that the text box with horizontal pad only is inside
5. tip is moved so that the text box with horizontal and vertical pad is inside

I've roughly encoded my preferece, though that's somewhat subjective - maybe the helper lines are also distracting: green = good, yellow = ok, red = problematic

Overall, if we want a simple criterion 1) is the best solution.
If we want to be more refined, use 2) if "the tip is large enough", otherwise 1). - We have to define "large enough": A simple heuristic solution would be some multiple of the pad, e.g. if the tip has a length of at least 2x pad. In theory, the crition of 5 (padded text is within the tip) could be taken as "large enough" but im inclined to think that a suitable chose heuristic is good enough.

Additional images for arrows with headwidth=1 (would also hold for headwidth < 1).

Here 1) is clearly the preferred solution. Note that the above criterion "head is large enough" realized by "padded text is within the tip" is always false for 2) so the more refined version above would also cover this case. (But it's still probably easier to short-cut on headwidth <= 1).

@timhoffm I'm hesitant to implement the above; while the padding code becomes much simpler, for example for LArrow:

# Apply padding to text on head side
if self.head_width > 1 and width_adjustment / (pad * tan_half_angle) > 1:
    # Pad text into head by amount pad
     x0 += pad

a, b = 1.214, 0.250  # Empirical factors to align text
padding_offset = (a * pad) + (b * mutation_size)
x0 -= padding_offset

this breaks public API, as it doesn't match the preestablished text positioning.

I'm not following, which is probably because I'm not deep enough in the topic:

• Is the code snippet you show is a hypothetical implementation of my examples? What are these empirical factors?
• What exactly breaks wrt. to the existing solution; i.e. in which cases and what is different?

We may consider breaking preestablished behavior if the solution is overall more consistent and much simpler to implement. But that requires a full overview on on the impact it would have.