Movatterモバイル変換

[0]ホーム
URL:

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

Accept LA icons for the toolbar#25174

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tacaswell merged 4 commits intomatplotlib:mainfromfgoudreault:fix-25164
Feb 9, 2023

Conversation

@fgoudreault
Copy link
Contributor

@fgoudreaultfgoudreault commentedFeb 7, 2023•
edited
Loading

PR Summary

Fixes#25164 by converting a PIL icon image to RGBA when loading it. Also created unittest to check the bug in the future.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (andpytest passes)
  • [ N/A ] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • [ N/A ] New plotting related features are documented with examples.

Release Notes

  • [ N/A ] New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • [ N/A ] API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • [ N/A ] Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@jklymakjklymak changed the titleFix issue #25164Accept LA icons for the toolbarFeb 7, 2023
Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good. not sure, but should this be intest_backend_tools.py?

@tacaswelltacaswell added this to thev3.7.0 milestoneFeb 7, 2023
@jklymakjklymak marked this pull request as draftFebruary 8, 2023 14:57
@fgoudreault
Copy link
ContributorAuthor

Looks good. not sure, but should this be intest_backend_tools.py?

I put the test intest_backend_tk.py as this was a bug related only toTkAgg AFAIK. The actual bug fix is in_backends/backend_tk.py.

@jklymak
Copy link
Member

Looks good. not sure, but should this be intest_backend_tools.py?

I put the test intest_backend_tk.py as this was a bug related only toTkAgg AFAIK. The actual bug fix is in_backends/backend_tk.py.

Thats probably fine, but is the same bug present in other backends?

@fgoudreault
Copy link
ContributorAuthor

Looks good. not sure, but should this be intest_backend_tools.py?

I put the test intest_backend_tk.py as this was a bug related only toTkAgg AFAIK. The actual bug fix is in_backends/backend_tk.py.

Thats probably fine, but is the same bug present in other backends?

As the bug stems from thebackend_tk.py file It would seem the bug would only affect this backend. Additionally, I just tested withqtagg and PyQt6 and there is no bug on themain branch.

@jklymak
Copy link
Member

Thats great - however, does the test apply to the other backends? If so, perhaps better to put somewhere that all the backends get tested to increase our test coverage? BTW, I haven't checked all this, so feel free to tell me the test is definitely only useful for Tk and I'm happy to approve - we can always generalize the test later if you are not correct.

@fgoudreault
Copy link
ContributorAuthor

Indeed, this test could be applied to any backend! You are right that it would be more useful if I can parametrize the test for any backend. Although I am not familiar with the whole set of matplotlib unittests. Do you know if there are tests that parametrize over backends so that I can generalize the one I introduce here? Thanks

@fgoudreault
Copy link
ContributorAuthor

Indeed, this test could be applied to any backend! You are right that it would be more useful if I can parametrize the test for any backend. Although I am not familiar with the whole set of matplotlib unittests. Do you know if there are tests that parametrize over backends so that I can generalize the one I introduce here? Thanks

I used the parametrization defined intest_backends_interactive.py in order to get environments with different backends and run them in a subprocess helper function. The parametrization now gives ~13 tests for different configuration. I tested withpyqt6,pyside6 andpycairo installed in my environment and it seems to work. I couldn't make it work withpyqt5 and otherpysides as I had trouble installing these packages.

@fgoudreault
Copy link
ContributorAuthor

Oh it seems there is a very similar bug in the wx backend seen on appveyor. I could reproduce it locally and fixed it. I'll push the fix.

@anntzer
Copy link
Contributor

Sorry for the conflicting informations, but I think this test should rather go into test_backends_interactive.py::_test_interactive_impl. This is because interactive tests are very, very slow to start up (as they need to be run into their own subprocess) so it is better to try and minimize the number of tests and do more in a single test. Probably we should better document this at the top of test_backends_interactive.py...

jklymak reacted with thumbs up emoji

@fgoudreault
Copy link
ContributorAuthor

Sorry for the conflicting informations, but I think this test should rather go into test_backends_interactive.py::_test_interactive_impl. This is because interactive tests are very, very slow to start up (as they need to be run into their own subprocess) so it is better to try and minimize the number of tests and do more in a single test. Probably we should better document this at the top of test_backends_interactive.py...

Sure I'll move it there

jklymak reacted with thumbs up emoji

@fgoudreaultfgoudreault marked this pull request as ready for reviewFebruary 9, 2023 17:21
Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not an expert on how the tests are structures (obviously 🐑), but this looks good to me. Perhaps@anntzer will have a minute for a second review.

fgoudreault reacted with hooray emoji
Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Modulo sorting out the extra try/except added in the test.

@tacaswelltacaswell merged commit6f753cd intomatplotlib:mainFeb 9, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestFeb 9, 2023
@tacaswell
Copy link
Member

Thank you for your work on this@fgoudreault and congratulations on your first merged Matplotlib PR 🎉 I hope we hear from you again!

fgoudreault reacted with hooray emoji

@fgoudreault
Copy link
ContributorAuthor

Thank you for your work on this@fgoudreault and congratulations on your first merged Matplotlib PR tada I hope we hear from you again!

The pleasure is mine! Thanks to all for your time!

tacaswell added a commit that referenced this pull requestFeb 9, 2023
…174-on-v3.7.xBackport PR#25174 on branch v3.7.x (Accept LA icons for the toolbar)
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@tacaswelltacaswelltacaswell approved these changes

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@jklymakjklymakjklymak approved these changes

Assignees

No one assigned

Projects

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

[Bug]: LA image mode not working anymore for custom toolbar buttons

5 participants

@fgoudreault@jklymak@anntzer@tacaswell@melissawm
[8]ページ先頭
©2009-2025 Movatter.jp