Looks like this is still failing in the same way

Even though the appveyor test is still failing, this PR does fix one bug, so it should be merged. In particular without it, tool icons do not appear when using tool-manager and the wx backend.

I closed my other PR#28008 (which I made before I saw this one) since the approach here is conceptually better and the other one still has the appveyor error.

Are we missing a call toFromSVGResource somewhere, perhaps?

This seems to be the bridge between an svg and what WX is documented as wanting for most internal tool APIs.

(Disclaimer: I've not worked much with wx at all, this is just from a quick reading of the docs)

EDIT: I now see we do call FromSVG in the_icon staticmethod... and that the Resource version of that is more limited (though it is the one that was linked to in the top paragraphs, thats why I selected it)

I will note, however, thatFromSVGs documented signatures are (int, wx.Size) and (wx.Byte, int, wx.Size)

We may wantFromSVGFile, which takes (str, wx.Size) we are passing (bytes, wx.Size) (where bytes is python bytestr.

The File variant does say it is a thin wrapper forFromSVG, but it more closely matches the signature we are providing.

What is the command to run locally to reproduce the appveyor issue? See my comment in#26710 (comment)

I am also seeing this upon rectangular selection locally:

Traceback (mostrecentcalllast):File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/cbook.py",line298,inprocessfunc(*args,**kwargs)File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_tools.py",line790,in_mouse_moveself.toolmanager.trigger_tool(File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_managers.py",line340,intrigger_tooltool.trigger(sender,canvasevent,data)# Actually trigger Tool.^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_tools.py",line343,intriggerself.draw_rubberband(*data)File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/backends/backend_wx.py",line1266,indraw_rubberbandNavigationToolbar2Wx.draw_rubberband(File"/home/kyle/src/scipy/matplotlib/lib/matplotlib/backends/backend_wx.py",line1139,indraw_rubberbandsf=1ifwx.Platform=='__WXMSW__'elseself.GetDPIScaleFactor()^^^^^^^^^^^^^^^^^^^^^^AttributeError:'types.SimpleNamespace'objecthasnoattribute'GetDPIScaleFactor'

(Only with toolmanager)

This traces to the idea of a "pseudo_toolbar" which contains only the canvas, and is ducktyped in to some functions including the rubberbanding.

The call style is odd here, but yeah it is something we do...

Okay, regardless of the addition of_icon_extension = ".svg", the_icon method is being given full paths to pngs in toolmanager, but just the filenames of svg on non-toolmanager.
(Edit: Added in the wrong spot at first, it does indeed fix icons locally)

So it is reading the png and then trying to pass that through FromSVG.

(I now also see the manipulation for dark mode to the text of the svg, which explains why File was not used, though the documented signatures are still weird to me)

This traces to the idea of a "pseudo_toolbar" which contains only the canvas, and is ducktyped in to some functions including the rubberbanding.

I know nothing of this "pseudo_toolbar", but in case it helps, this is thedocumentation for GetDPIScaleFactor. Perhaps there is another function that we can use that is not wx specific to get the same effect? How is this handled in other backends?

Aha, the test which is failing is passing a PNG icon in, and there is not any handling of such in_icon, so it is treatingthat png as an svg, which is failing.

Nice find! Should we save the file with the extension defined in the _icon_extension variable? Unfortunately,PIL Image cannot save svg, so it is not a trivial change.

We don't know what file types might be used in user tool items, so we should correctly handle .png as well.

We don't know what file types might be used in user tool items, so we should correctly handle .png as well.

Can other backends handle multiple formats? It seems that the current implementation ofToolContainerBase specifies a_icon_extension and it looks to me that each backend expect icons in that format.

With respect to fixing the current test, perchaps one possible solution would be not to use PIL to generate the icon, but to use matplotlib itself to generate a figure that we save with the appropriate_icon_extension?

Can other backends handle multiple formats? It seems that the current implementation ofToolContainerBase specifies a_icon_extension and it looks to me that each backend expect icons in that format.

Actually, that's a good question; only GTK defaults to SVG at the moment, though it doesn't fallback to anything with other extensions correctly. It doesn't error out though.

With respect to fixing the current test, perchaps one possible solution would be not to use PIL to generate the icon, but to use matplotlib itself to generate a figure that we save with the appropriate_icon_extension?

No, this test is specifically about PNG formats; it can't be replicated with SVG, but maybe it should be skipped.

On the one hand, people using PNG for their custom tools would be broken here, but on the other this toolbar is experimental. However, it's been that way for many many years, so I think we do need to support it. Fortunately, it seems relatively easy to fall back to the old code if needed.

With respect to fixing toolbar-manager, I think the suggestion of falling back to the old code when using non svg images is a good one and FWIW, I am ready to merge this PR. Thank you@QuLogic.

The separate issue of an exception when using a "pseudo_toolbar" that was mentioned by@ksunden at#28007 (comment) still remains.

I do not know about the "pseudo-toolbar" functionality. But based on the stack trace we could try/except the current code and fall back to thedisplay's GetScaleFactor for monitor 0 instead of the display in which the toolbar is being displayed. Or even better theGetDPIScaleFactor method of the main figure window, if the figure window object is available in the duck-typed version. Both of these assume the wx application has been initialized, which I guess, but I really don't know about the "pseudo-toolbar"

Looks good to me. Thank you@QuLogic and@ksunden.