-
Notifications
You must be signed in to change notification settings - Fork 231
Remove all resizing logic on the front-end #174
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
Conversation
MPLCanvasView.__super__.processPhosphorMessage.apply(this, arguments); | ||
|
||
switch (msg.type) { | ||
case 'resize': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this, this was causing a redraw every time the plot container was resized in JupyterLab, but this redraw was useless (because the plot size is controlled by matplotlib, not JupyterLab), and it was causing some flickering.
} | ||
} | ||
|
||
this.request_resize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, all those request_resize
were useless and were causing flickering.
Signed-off-by: martinRenou <martin.renou@gmail.com>
201e0ae
to
53ea6b5
Compare
Thanks for looking into this. When I first started working on this, it seemed like from the discussion in #117 that people wanted to be able to set the figure size either by using either the matplotlib api (e.g. The JavaScript logic was an attempt at handling the widgets API. When the canvas width is set to something like 500px, there isn't a good way of inferring what size that should be in matplotlib, and it will depend on the client. My approach was basically a hack to take the current widget and calculate what the matplotlib figure size should be off of that. I then sent out a request to matplotlib to resize the figure to that size. This works and has the "expected behavior" for both APIs, but is ugly because it requires you to know how the figure and decorations are all laid out. I am not personally experiencing any flickering, but that is likely a bug where the matplotlib resize code is being called somewhere that it shouldn't be. With your PR you will lose the ability to set the canvas size via the widgets API (e.g. |
I guess not supporting It was actually my first approach to ignore the sizing from matplotlib and to do a "responsive" plot, but that does not fit with the matplotlib way of doing things, and people were unhappy about not being able to set the size from the matplotlib API (see #117 ). Do you agree with all this? Were you using |
I do not use There is definitely no easy way to sync things across multiple clients and keep both APIs. I believe that even the decorators aren't guaranteed to be the same sizes across clients so even for fixed sized plots there would be issues. To handle multiple clients properly, you would need to have different matplotlib figure sizes for each of the clients and rework the backend to support that. |
@kboone if you don't mind looking at it that would be awesome!
I am removing lots of code you added in #147. I am basically removing all resizing logic on the JavaScript side, and matplotlib is entirely responsible for the plot sizing through
figsize
inplt.figure()
andfig.set_size_inches()
. Some of this leftover resizing logic was causing some flickering (the front-end was requesting resizes while there was no need for a resize).I wonder why you added so much logic in the resize
Note that we might put back some of this resizing logic when adding a resize handle on the figure (allowing to resize with the mouse). I am planning to work on this after this cleaning.
I think it is safe to say that it will finally close #117.