Skip to content

Implement FigureManager.resize for macosx backend #16992

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
merged 5 commits into from
May 5, 2020

Conversation

dopplershift
Copy link
Contributor

@dopplershift dopplershift commented Apr 1, 2020

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Fixes #15131. In addition to implementing the missing method there are also a few clean-ups here for the macosx backend. This also depends on fixing handling of Figure.dpi changes in #16971 or #16972.

Instead of manually adding tracking rectangles and updating for resize
(older API), use a "newer" API (added OSX 10.5) for tracking areas. With
the right flags, these areas are automatically updated for resize.
Just set some resizing flags on the view. This allows us to
avoid manually resizing the view.
We can just request these events from the tracking area rather than
manually enabling/disabling mouse move/checking inside.
Rename _device_scale to _dpi_ratio, as expected by
Figure.set_size_inches(). Also, make sure the value of _dpi_ratio on
instances of FigureCanvasMac are updated *before* setting figure DPI so
that the proper value is available for downstream code. This is part of
the fix needed to make set_size_inches work on macosx.
This allows Figure.set_size_inches to work.
@dopplershift
Copy link
Contributor Author

Tagging 3.3 since the underlying issue is tagged 3.3. Would be nice to get the issue closed.

@dopplershift dopplershift added this to the v3.3.0 milestone Apr 13, 2020
@dopplershift dopplershift added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Apr 13, 2020
@QuLogic QuLogic requested a review from jklymak April 14, 2020 03:34
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This works and the code seems fine.

I have no idea what the Rect stuff is supposed to be doing, and seems unrelated. OTOH I couldn't see that it broke anything. Some explanation would be helpful.... OK< sorry this was in the commit message.

@dstansby
Copy link
Member

Just given this a spin and it seems to work well. The only thing I've noticed is if I resize the window to have zero height, I get a bunch of messages like:

  File "/Users/dstansby/github/matplotlib/lib/matplotlib/figure.py", line 879, in set_size_inches
    raise ValueError(f'figure size must be positive finite not {size}')
ValueError: figure size must be positive finite not [ 5.65 -0.34]
Traceback (most recent call last):
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 84, in resize
    forward=False)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/figure.py", line 879, in set_size_inches
    raise ValueError(f'figure size must be positive finite not {size}')
ValueError: figure size must be positive finite not [ 5.6  -0.19]

@timhoffm
Copy link
Member

@dstansby We currently expect figures to have a finite height and width. However that may change, see #17093 (comment).

@QuLogic
Copy link
Member

QuLogic commented Apr 20, 2020

In other backends (WebAgg, specifically), there are minimum sizes on the window so you can't do that.

@dstansby
Copy link
Member

Yeah, I'm guessing the backend should enforce >0 figure height and width.

@dopplershift
Copy link
Contributor Author

So is that a requested change on this PR?

@timhoffm
Copy link
Member

I think it would be good to enforce a finite size for this PR, so that we can get it still into 3.3. There are considerations elsewhere to drop that limitations, but that needs additional changes in the code.

@tacaswell
Copy link
Member

How does this interact with

#17084 and
#17263 ?

@dopplershift
Copy link
Contributor Author

@tacaswell Should be no interaction between this and either of those other two.

@timhoffm To be clear, this PR does not make the figure size must be positive finite messages newly possible. I can get those today with master just fine:

Traceback (most recent call last):
  File "/Users/rmay/repos/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 80, in resize
    self.figure.set_size_inches(width * self._device_scale,
  File "/Users/rmay/repos/matplotlib/lib/matplotlib/figure.py", line 879, in set_size_inches
    raise ValueError(f'figure size must be positive finite not {size}')
ValueError: figure size must be positive finite not [ 0.73 -0.24]
Traceback (most recent call last):
  File "/Users/rmay/repos/matplotlib/lib/matplotlib/backends/backend_macosx.py", line 80, in resize
    self.figure.set_size_inches(width * self._device_scale,
  File "/Users/rmay/repos/matplotlib/lib/matplotlib/figure.py", line 879, in set_size_inches
    raise ValueError(f'figure size must be positive finite not {size}')
ValueError: figure size must be positive finite not [ 0.73 -0.35]

(just grab the upper right corner and drag down and to the left past the lower left corner.)

I can do it if strictly necessary, but it's not a new issue introduced by this PR.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I can't judge the Objective C changes, but in a simple test, the code works for me and solves the original problem of the mismatch between window size and figure size.

@QuLogic
Copy link
Member

QuLogic commented May 5, 2020

I'm going to merge then; finite size requirements can be a separate new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI: MacOSX OS: Apple Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set_size_inches doesn't resize window on macosx backend
7 participants