Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

wx: Fix file extension for toolmanager-style toolbar #28007

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

Merged
QuLogic merged 3 commits into matplotlib:main from QuLogic:wx-toolbar-icons
Apr 3, 2024

Conversation

Copy link
Member

@QuLogic QuLogic commented Apr 2, 2024

PR summary

Fixes AppVeyor failure noted in
#26710 (comment)

This was also broken on non-Windows; it just didn't crash, but instead produced empty buttons.

PR checklist

@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Apr 2, 2024
@QuLogic QuLogic added this to the v3.9.0 milestone Apr 2, 2024
Copy link
Member

ksunden commented Apr 2, 2024

Looks like this is still failing in the same way

Copy link
Contributor

jmoraleda commented Apr 2, 2024
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Member

ksunden commented Apr 2, 2024
edited
Loading

Are we missing a call to FromSVGResource 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, that FromSVGs documented signatures are (int, wx.Size) and (wx.Byte, int, wx.Size)

We may want FromSVGFile, 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 for FromSVG, but it more closely matches the signature we are providing.

Copy link
Contributor

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

Copy link
Member

ksunden commented Apr 3, 2024
edited
Loading

I am also seeing this upon rectangular selection locally:

Traceback (most recent call last):
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/cbook.py", line 298, in process
 func(*args, **kwargs)
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_tools.py", line 790, in _mouse_move
 self.toolmanager.trigger_tool(
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_managers.py", line 340, in trigger_tool
 tool.trigger(sender, canvasevent, data) # Actually trigger Tool.
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/backend_tools.py", line 343, in trigger
 self.draw_rubberband(*data)
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/backends/backend_wx.py", line 1266, in draw_rubberband
 NavigationToolbar2Wx.draw_rubberband(
 File "/home/kyle/src/scipy/matplotlib/lib/matplotlib/backends/backend_wx.py", line 1139, in draw_rubberband
 sf = 1 if wx.Platform == '__WXMSW__' else self.GetDPIScaleFactor()
 ^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'types.SimpleNamespace' object has no attribute '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...

Copy link
Member

ksunden commented Apr 3, 2024
edited
Loading

(削除) 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)

Copy link
Contributor

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 the documentation 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?

Copy link
Member

ksunden commented Apr 3, 2024

def _test_toolbar_button_la_mode_icon(fig):
# test a toolbar button icon using an image in LA mode (GH issue 25174)
# create an icon in LA mode
with tempfile.TemporaryDirectory() as tempdir:
img = Image.new("LA", (26, 26))
tmp_img_path = os.path.join(tempdir, "test_la_icon.png")
img.save(tmp_img_path)
class CustomTool(ToolToggleBase):
image = tmp_img_path
description = "" # gtk3 backend does not allow None
toolmanager = fig.canvas.manager.toolmanager
toolbar = fig.canvas.manager.toolbar
toolmanager.add_tool("test", CustomTool)
toolbar.add_tool("test", "group")

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 treating that png as an svg, which is failing.

Copy link
Contributor

jmoraleda commented Apr 3, 2024
edited
Loading

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.

Copy link
Member Author

QuLogic commented Apr 3, 2024

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

Copy link
Contributor

jmoraleda commented Apr 3, 2024
edited
Loading

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 of ToolContainerBase 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?

Copy link
Member Author

QuLogic commented Apr 3, 2024

Can other backends handle multiple formats? It seems that the current implementation of ToolContainerBase 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.

This falls back to the previous dark-mode setup for any other formats.
It won't be HiDPI compatible, but that's probably not possible for
raster formats.
@rcomer rcomer mentioned this pull request Apr 3, 2024
5 tasks
Copy link
Contributor

jmoraleda commented Apr 3, 2024
edited
Loading

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.

Copy link
Contributor

jmoraleda commented Apr 3, 2024
edited
Loading

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 the display's GetScaleFactor for monitor 0 instead of the display in which the toolbar is being displayed. Or even better the GetDPIScaleFactor 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"

Copy link
Contributor

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

@QuLogic QuLogic merged commit 905da46 into matplotlib:main Apr 3, 2024
@QuLogic QuLogic deleted the wx-toolbar-icons branch April 3, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
GUI: wx Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /