Introduce build figure

[saroad2]

The Problem

At the moment, Chart does not allow to redraw a figure manually. However, it does allow doing so automatically on resize, using a protected method.

My solution

I introduced a new method called Chart.build_figure that creates a figure with the right screen size. This figure now can be used as a parameter to Chart.draw.

Why not simply use Chart.draw?

This reason is that Chart.draw excepts a figure parameter, but that figure might not have the size of the chart. Chart._resize does not except a figure, but it creates a figure that is suitable for the chart size.

PR Checklist:

• [X] All new features have been tested
• [X] All new features have been documented
• [X] I have read the CONTRIBUTING.md file
• [X] I will abide by the code of conduct

[saroad2]

So - the use case makes sense, but the final shape of the API doesn't. Is there any use case where someone wouldn't invoke self.chart.draw(self.chart.build_figure())? Is there any reason to not expose redraw() which is an implied draw(build_figure)?

Your'e right. I can't seem to find any use case when someone will run Chart.draw without using Chart.build_figure. However, I cannot remove the Chart.draw since it is an implementation of the superclass pyplot.FigureCanvasBase.

Your original PR of this looked very close to this (renaming _resize()) as redraw())'; is it possible the problem is that we need to retain a specific _resize() handler, and expose an additional redraw()?

When I was trying to implement Chart.redraw in the following way, it caused an infinite loop:

def redraw(self):
    self.draw(self.build_figure())
    super(Chart, self).redraw()

I wasn't able to figure out why.

[freakboy3742]

I wasn't able to figure out why.

That's because redraw() is already a method on canvas(), and adding a drawing object causes a redraw, and the renderer adds lots of drawing objects. The same is true of any operation to learn the canvas.

This indicates we've got a locking condition on canvas - if you're in the middle of a redraw, you can't do a redraw (or, something about the redraw needs to be suspended.

[saroad2]

This indicates we've got a locking condition on canvas - if you're in the middle of a redraw, you can't do a redraw (or, something about the redraw needs to be suspended.

So the solution is to add a "redraw indicator" in toga.Canvas such that it will not redraw again if a redraw is in progress? Or the same, but doing so in the toga.Chart class?

[freakboy3742]

I'm not sure it's as simple as that. We have a problem here where redraw() now has 2 interpretations - it's used by Canvas as an internal "something has changed on the canvas" API; this PR is adding a "I wish to make a change to the canvas" interpretation. In the former use case, redraw() is the canvas's reaction to a draw(); in the new use case, redraw() causes a draw. This is the root source of the recursion.

It might be possible to fix this with a flag to short-circuit the redraw in the latter case, but honestly, that just seems fragile. I suspect the actual fix is to either:

1. Rework the internal structure of Canvas to use composition rather than inheritance. The problem we're facing here is that the APIs of our superclasses is colliding with the API we want to expose to end users; one way to avoid this is to change the internal structure so that end users don't inherit the raw API of the base classes.
2. Come up with a name other than redraw(). The only name that is coming to mind is update(), which I don't love but it would work.

The quickest fix is (2); but that means we end up with an API that has draw(), redraw() and update()... and the first two of those are APIs that shouldn't be touched by end users. That's not ideal from an API design point of view.

[saroad2]

I went ahead and implemented 2 since it is a faster solution.

I will open a new issue linked to this issue in toga, so we could tackle it there. Once this is solved, we could add another fix here to rename Chart.update to Chart.redraw.

[freakboy3742]

I went ahead and implemented 2 since it is a faster solution.

"Faster" isn't necessarily a good thing. We have to live with the consequences of "quickly" introducing (and potentially deprecating) an API.

The more I think about this, the more I'm convinced the fundamental design of this widget is wrong.

[saroad2]

Okay, I'm closing this PR. We'll keep the discussion in beeware/toga-chart#21 and then decide what's the best way to go.