macOS backend rework, initial PR - #32161

I think this is a nice direction to go, thank you for taking the time and effort to work on this major overhaul! Since it was so much content I had AI do a quick sweep of the code and read through all the comments and they all look valid and like things to look into. I'll paste it all below (2+), feel free to ignore if you don't want to read an AI review but it will take me a while to go through this all later so I wanted to leave you some initial comments for now. Overall 👍 on transitioning to this.

1. Framing

With this we are trying to shim in the backwards compatibility, is there a reason we don't just leave the _macosx.m file around which would allow people to keep importing/using the current system as-is. If someone needs to import the objective C or use a specific class from a module location then everything stays as it is. My thought is that we'd basically switch the default backend finding to point macosx to your new version and then if someone wanted the old they could do pyplot.use("macosx-legacy") to get that back as well. I haven't thought super hard about this though.

What this kind of reminds me of is the mplcairo backend (which I always wished would come into core at some point) and how we would name something like that if it did come into core because we already have the Cairo backends and would need some transition for those.
https://github.com/matplotlib/mplcairo

2. Grouping

_macos.m is now 1149 lines and the largest file in the set, holding four unrelated PyTypeObjects. Given the whole premise was per-file responsibility:

• Split the glue the way you split the Obj-C: _macos_FigureCanvas.m, _macos_FigureManager.m, etc., each exposing PyTypeObject *MPLGetFigureCanvasType(void), leaving _macos.m as module init + module-level functions.
• wait_for_stdin / flushEvents / stopWithEvent / handleSigint (_macos.m:62-151) are neither glue nor a paired class. They're the one part of that file that clearly wants MPLEventLoop.[hm] now, not in the follow-up PR.

Also, ~15 of those glue functions are near-identical BEGIN_OBJC_ENTRY / parse / one message send / RETURN_NULL_OR_NONE. A couple of macros would halve that.

The Obj-C→Python boundary leaks in one place: MPLFigureCanvas.m:423 calls PyErr_SetString directly, which contradicts the design you documented. updateLayerContentsWithBuffer: should return BOOL and let _macos.m raise.

3. Correctness bugs

Ranked roughly by severity.

FigureCanvasMac.draw never runs. backend_macos.py:45 and :138:

class FigureCanvasMac(_macos.FigureCanvas, FigureCanvasBase):
def draw(self): ...
class FigureCanvasMacAgg(FigureCanvasAgg, FigureCanvasMac): pass

MRO is FigureCanvasMacAgg → FigureCanvasAgg → FigureCanvasMac → …, so canvas.draw() resolves to FigureCanvasAgg.draw and your override is dead. Consequence: a user calling fig.canvas.draw() renders into Agg but never calls _request_display_layer, so the window shows stale content. (And if it were reached, its super().draw() would hit FigureCanvasBase.draw, a no-op — it would never render at all.) The legacy backend avoids this with a single class, Agg first: class FigureCanvasLegacyMac(FigureCanvasAgg, _macosx.FigureCanvas, FigureCanvasBase). Note test_backend_macosx.py's first test is exactly fig.canvas.draw() — it passes only because it runs against the legacy backend.

Middle and right mouse buttons are swapped during motion. backend_macos.py:83-89:

(MouseButton.MIDDLE, 1 << 1),
(MouseButton.RIGHT, 1 << 2),

+[NSEvent pressedMouseButtons] documents bit 1 as right and bit 2 as other/middle. The legacy code has it right (_legacymac.m:336-337), and your own _handleMouseDownOrUp: swaps correctly — so press/release and motion will disagree with each other.

Separately, _handleMouseDownOrUp: maps AppKit buttons 3/4 to 4/5, but _mpl_buttons maps those same physical buttons to MouseButton.BACK/FORWARD (8/9). Back/forward buttons report different values on press than on motion.

_needsDrawOnNextDisplayLayer is a latch, not a flag. MPLFigureCanvas.m:110-114 reads it but never clears it, and :486 overwrites rather than accumulates. Two bugs fall out:

• Lost update: draw_idle() sets it YES; a blit() before the runloop drains sets it back to NO; the pending re-render is silently dropped.
• Sticky YES: after any draw_idle(), every subsequent displayLayer: (e.g. AppKit-driven resize) triggers a full Python re-render forever.

Wants _needsDrawOnNextDisplayLayer |= needsDraw; on request, and clear it in displayLayer:.

Window resize ignores the backing scale. MPLFigureManager.m:133:

[window convertRectFromBacking:rect]; // return value discarded

rect stays in device pixels, so manager.resize() makes the window 2× too large on Retina. Should be rect = [window convertRectFromBacking:rect];.

Python method called on a zero-refcount object. _macos.m:483-497 — FigureManager__close_and_clear_window_impl calls [self->object close] before setPyObject:NULL. On the FigureManager_dealloc path (:500, reached whenever a figure is GC'd without plt.close()), -close synchronously fires windowWillClose: → MPLCallMethod(_pyObject, …) on an object already inside tp_dealloc. PyObject_GetAttrString takes it 0→1 and the release takes it back to 0, re-entering _Py_Dealloc. Fix is one line: clear pyObject before close. FigureCanvas_dealloc and NavigationToolbar2_dealloc already get this order right.

windowShouldClose: has the wrong signature. MPLFigureManager.m:102 takes (NSNotification *); NSWindowDelegate declares (NSWindow *)sender. It works only because messaging is dynamic.

self used before [super init…]. MPLFigureManager.m:51-64 calls [window setDelegate:self], makeFirstResponder:, and addSubview: before [super initWithWindow:window] on line 66. Move all the configuration after the super call.

NULL returned without an exception set. _macos.m:694-695:

MPLStringArray *strings = MPLGetStringArrayWithPySequence(args);
if ([strings count] != 4) return NULL;

If conversion succeeded but the count is wrong, this returns NULL with no error → SystemError. choose_save_file (:1029) gets this right.

Missing type check. _macos.m:440-444 — PyArg_ParseTuple(args, "O", …) then an unchecked cast to FigureCanvas *. NavigationToolbar2_init in the same file uses "O!" with &FigureCanvasType; do the same here. (Carried over from legacy, but trivial to fix now.)

tp_init swallows Obj-C exceptions. FigureManager_init (:455) and NavigationToolbar2_init (:669) return 0 unconditionally, so a caught Obj-C exception sets a Python error but init reports success. FigureCanvas_init:185 gets it right with return PyErr_Occurred() ? -1 : 0;.

errSetException can crash. _macos.m:56 — [[exception reason] UTF8String] returns NULL when reason is nil, and PyErr_SetString(…, NULL) segfaults.

Dispatch source outlives its fd. _macos.m:959-983 — this is a rewrite of the legacy NSFileHandle version, and it introduces two problems. The source is never stored or cancelled if SIGINT never arrives, and _allow_interrupt's finally closes rsock underneath a live DISPATCH_SOURCE_TYPE_READ — which Apple documents as a hard error (cancel first, close in the cancel handler). The handler block also strongly captures source, so a never-fired source leaks permanently.

flagsChanged: press/release detection. MPLFigureCanvas.m:299 — currentFlags > _previousModifierFlags compares bitmask magnitudes. Release Command (bit 20) while pressing Shift (bit 17) in one event and it reports a release. Want changed = current ^ previous; isPress = (current & changed) != 0.

Rubberband is a subview of a layer-hosted view. MPLFigureCanvas.m:176 adds MPLRubberbandView as a subview of the canvas, but :48-54 makes the canvas layer-hosting by assigning layer directly. Apple's layer-hosting docs (the same page the PR links) say a layer-hosting view must not have subviews. It'll probably render today; it's the kind of thing that breaks across macOS releases. A CAShapeLayer sublayer with lineDashPattern would be both supported and give you marching ants for free.

layer.contentsScale is never set. viewDidChangeBackingProperties (:77) is exactly where you'd set it, and AppKit does not maintain it for layer-hosted views. kCAGravityResize currently papers over this; setting it gives you a guaranteed 1:1 pixel mapping. (Also worth calling super here.)

kCGImageAlphaLast may defeat the whole point. MPLFigureCanvas.m:428 — non-premultiplied alpha typically forces Core Animation into a CPU conversion, which undercuts the sRGB/GPU-compositing work in MPLFigureManager. Since the layer is opaque:YES over white, kCGImageAlphaNoneSkipLast is probably what you want; worth measuring both.

[NSApp stop:] without a wake event. _macos.m:494 — stop: only takes effect once the next event is processed. You have stopWithEvent() for precisely this; using bare stop: here means closing the last window may not exit show() until unrelated input arrives.

4. Performance

The keymap dictionary is rebuilt on every keystroke. MPLFigureCanvas.m:189-210 allocates a 35-entry NSDictionary plus 35 NSNumbers per key event. Wants static + dispatch_once.

The cursor dictionary is worse. :456-462 allocates a dict, 5 NSNumbers, and calls all five [NSCursor …Cursor] accessors on every cursor change — which during pan/zoom is every mouse-move. A switch is both faster and clearer.

MPLGetStringWithPySequence on the hot path. set_message (_macos.m:742) fires on every motion event, and that helper (MPLUtils.m:140) builds an NSMutableArray, an NSString, and a [copy] just to pull one string out of a 1-tuple. PyArg_ParseTuple(args, "s", &cstr) is one allocation instead of three. Same pattern in set_window_title, _set_window_appearance, _set_window_mode, add_item.

choose_save_file holds the GIL across a modal panel. _macos.m:1044 — [panel runModal] blocks every other Python thread for as long as the user browses. Wrap in Py_BEGIN_ALLOW_THREADS. (Also __block on modalResponse at :1043 is vestigial, and :1050 returns 0 instead of NULL.)

_buttonWithCallbackName: is O(n) over two parallel arrays. MPLNavigationToolbar2.m:13-14, 96-112 — _buttons and _callbackNames kept index-synchronized by convention is fragile. NSView already has an identifier property: store the callback name there, key a dictionary on it, and both arrays disappear along with the indexOfObject: scans.

5. Idiom notes

• Duplicated modifier tables. Obj-C _keyStringWithString: maps NSEventModifierFlags→strings for keys; Python _mpl_modifiers maps the same flags→strings for mouse, with hardcoded 1 << 18 magic numbers, and the two sets already disagree (Obj-C has caps_lock, Python doesn't). Pick a side. Given the stated design, export the constants via PyModule_AddIntConstant and do it all in Python.
• Cursor magic numbers. MPLFigureCanvas.m:450-462 hardcodes 1/2/3/4/6/7 against Cursors. Worth an NS_ENUM in a header with a comment tying it to backend_tools.Cursors. Note WAIT (5) currently falls through to a nil cursor by accident; legacy had an explicit case 5: break; with a comment explaining macOS handles busy state itself.
• updateSelectedItem: (MPLNavigationToolbar2.m:158-167) mutates buttonType on the selected button but never restores it, so previously-selected buttons stay PushOnPushOff. Set the type at creation time instead.
• Auto Layout vs. autoresizing masks are mixed in both MPLNavigationToolbar2 (constraints on _messageField, manual frames for the button container) and installToolbar:. An NSStackView for the button container would delete _nextButtonX, the container frame math, and make addSeparator a two-line NSBox.
• @interface NSObject () at MPLAppDelegate.m:7 — a class extension on a class you don't own. Should be a category (@interface NSObject (MPLUndeclaredActions)) since it has no matching @implementation. Also [menuItem setTarget:menu] + submenuAction: at :71-72 is pre-10.0 idiom; setSubmenu: alone is sufficient. (The block-based menu DSL itself is lovely.)
• MPLGetStringWithPyString (MPLUtils.m:89-95) round-trips through PyUnicode_AsUTF8 + stringWithUTF8String:, which truncates at embedded NULs and costs an extra strlen. PyUnicode_AsUTF8AndSize + initWithBytes:length:encoding: handles both.
• MPLUtils.h pulls <Python.h> into every consumer — including MPLAppDelegate.h, which only wants the MPLStringDictionary typedef. A small MPLTypes.h would keep Python.h out of headers that don't need it. Relatedly, PY_SSIZE_T_CLEAN is defined only in _macos.m, so the other TUs get Python.h without it (harmless on 3.11+, but inconsistent).
• tp_name says _macosx in all four types (_macos.m:358, 581, 749, 885) — user-visible in reprs and error messages.
• Static naming is inconsistent in _macos.m: sLoaded/sLoadedLock vs IsRunningFromShow/FigureManagerHashTable vs appDelegate vs backend_inited vs originalSigintAction. Four conventions in one file.
• STOP_EVENT_LOOP is effectively dead — stopWithEvent() posts subtype 0, stop_event_loop posts subtype 2, and _start_event_loop breaks on any NSEventTypeApplicationDefined regardless.

6. Free-threading, build, testing

Py_MOD_GIL_NOT_USED (_macos.m:1099) is a claim I don't think holds. FigureManagerHashTable, IsRunningFromShow, backend_inited, appDelegate, and every self->object are unsynchronized. FigureManager_new requires the main thread, but FigureCanvas and Timer don't. Either drop the declaration for now or document that everything except FigureCanvas is main-thread-only.

Build: override_options: ['werror=true','optimization=s'] hardcodes -Os, so a --buildtype=debug build of matplotlib still can't get a debuggable macOS backend. Worth making that conditional. werror=true has precedent from _macosx, but pairing it with ~35 new warning flags meaningfully raises the odds that a future compiler breaks downstream packagers' builds. Also check NSBezelStyleSmallSquare (MPLNavigationToolbar2.m:129) — I believe that spelling only exists in the macOS 14+ SDK; worth confirming against your minimum supported Xcode, since deployment target is 10.14.

Thank you for the reply! I'll check more of this tomorrow, but I wanted to reply to a few points:

1. Framing

I think that the eventual plan is to have a period of time where both macosx and macos are present, and that in an emergency, we could re-default someone into macosx.

2. Grouping

I thought about placing the Python glue code in the .m files. My worry is:

1. I like the idea of eventually adopting pybind11 or nanobind rather than using the Python C API directly. I made a prototype and it does clean things up.
2. However, if the glue code goes into the .m files, and we decide to adopt pybind11/nanobind: the whole backend is now Objective-C++. I'm fine with it being used in a simple glue code layer, but I'd prefer to keep most of the backend in pure Obj-C and C since not too many people are familiar with Obj-C++.

(wait_for_stdin / flushEvents / stopWithEvent / handleSigint) are the old implementation for now because I specifically want the PR to serve as a documentation for MPLEventLoop.

3. Correctness bugs

4. Performance

5. Idiom notes

I need to go through each of these points tomorrow and check. I think there are some valid issues here and some cases where the AI is using some out-of-date information on performance characteristics ;)

6. Free-threading, build, testing

"document that everything except FigureCanvas is main-thread-only."

I'd like your opinion on #31968 at some point (no rush!). My inclination is to go ahead and add a main thread check to the BEGIN/END macros except for draw_idle(). I feel like the advantages of preventing a crash and alerting the user with a friendlier message outweight the 2-3 lines of code in the backend.

~35 new warning flags meaningfully raises the odds that a future compiler breaks downstream packagers' builds

I'm not sure if I follow the AI's logic here. None of the other backends have debug builds, and I would think that we would see future compiler breaks before downstream packagers'.

NSBezelStyleSmallSquare

Should be in 10.14, but it's a moot point - it goes away with the Toolbar PR so we don't overlap the Tahoe window corner.

That was a great read! While there were cases of the AI being pedantic, it brought up some good issues. I'll hide the details in disclosures so we can focus on the important bits below.

Fixed AI Suggestions

Ignored AI Suggestions

"FigureCanvasMac.draw never runs"

The AI said that FigureCanvasMac.draw never runs. I'm not seeing this in my own tests.

We have the following class hierarchy:

# backend_bases.py
class FigureCanvasBase:
    def draw(self):
        print("FigureCanvasBase.draw")


# backend_agg.py
class FigureCanvasAgg(FigureCanvasBase):
    def draw(self):
        print("FigureCanvasAgg.draw")
        super().draw()


# _macos.m 
class _macos_FigureCanvas:
    def draw(self):
        print("_macos_FigureCanvas.draw")


# backend_macos.py
class FigureCanvasMac(_macos_FigureCanvas, FigureCanvasBase):
    def draw(self):
        print("FigureCanvasMac.draw")
        super().draw()

class FigureCanvasMacAgg(FigureCanvasAgg, FigureCanvasMac):
    pass
    

canvas = FigureCanvasMacAgg()
canvas.draw()

FigureCanvasAgg.draw and FigureCanvasMac.draw both perform super.draw() calls. I think the AI is getting confused at the class hierarchy and all is well.

I'd appreciate if a Python expert could take a look.

_needsDrawOnNextDisplayLayer

The AI is correct in that _needsDrawOnNextDisplayLayer doesn't get set to NO after calling _handle_display_layer in Python.

However, we always set it before calling -[CALayer setNeedsDisplay] on the canvas layer; hence, it should never be "stale" (needsDisplayOnBoundsChange is NO, so the macOS UI should never call -setNeedsDisplay for us).

I'm going to investigate to be 100% sure – I'll probably add some extra comments.

flagsChanged press/release detection

In practice, flagsChanged from a keyboard event will only change one modifier flag at a time. These bubble-up from the HID system and there is no concept of event coalescing.

In theory, another app could synthesize a fake keyboard event (CGEventCreateKeyboardEvent) where two modifier keys are modified in "different directions". For example, Control being pressed while Option is simultaneously released. In this case, I believe that our previous logic would fail.

I'm inclined to keep it simple unless someone actually hits this case.

kCGImageAlphaLast may defeat the whole point

The AI is partially correct.

First, there is no 100% guarantee to get CPU-less rendering. The "fastest path" is typically obtained with the following combination:

• An IOSurface set as CALayer.contents, not a CGImageRef
• Premultiplied alpha in the first component
• Little-endian byte order
• Matched color space

We can't hit this as our buffer from Agg is non-premultiplied RGBA - we need premultiplied BGRA. Hence, there's probably going to be a conversion in any case. Our goal is to let the system make that determination and handle that conversion in the compositing server rather than in our process.

If we know that our Agg buffer is opaque, we can set kCGImageAlphaLast to get a slight speedup. I looked into this, but wasn't sure if it was worth the cost of checking various Figure properties.

In any case, we performing far less drawing and color space conversion now than we were.

Additional Notes

I'm reverting wake_on_fd_write to the legacy implementation and will address the AI's concerns in the Event Loop PR.

I'm making a note that "choose_save_file holds the GIL across a modal panel" and will investigate it as well for the Event Loop PR. I think we probably want to release the GIL for any spinup of the event loop, but I don't feel comfortable making those changes in the initial PR.

Two changes:

I decided to take the AI's suggestion of limiting #import <Python.h>. The pyObject property for each Objective-C class is now an MPLPyObjectRef rather than a PyObject *. Only "_macos.m" and "MPLUtils.m" pull in "Python.h".

I'm adding placeholder files for MPLEventLoop, MPLSubplotTool, and MPLTimer. Xcode does not handle missing files gracefully – I'm routinely having to add/remove them as I switch among the different PR branches and it's slowing me down.

I'm adding placeholder files for MPLEventLoop, MPLSubplotTool, and MPLTimer. Xcode does not handle missing files gracefully – I'm routinely having to add/remove them as I switch among the different PR branches and it's slowing me down.

Are you able to try the new Stacked Pull Request feature? That seems ideal for this kind of work here where you need to build off of this branch and then you can keep working from this without needing to try to get everything into main.
https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests

Are you able to try the new Stacked Pull Request feature? That seems ideal for this kind of work here where you need to build off of this branch and then you can keep working from this without needing to try to get everything into main. https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests

Unfortunately, "Stacked pull requests require all branches to be in the same repository. Cross-fork stacks are not supported", which makes it useless for our purposes. I was really excited for Stacked PRs and was disappointed when I saw that sentence :(

I’m not following the details of this work, but would it help if we made a feature branch in this repo for @iccir to target?

I’m not following the details of this work, but would it help if we made a feature branch in this repo for @iccir to target?

I think that was mentioned briefly at the meeting but we didn't want to change process too much. I'm willing to do whatever will help make the review process easier, however!