kivy-garden / garden.matplotlib

Matplotlib backends using kivy
MIT License
103 stars 50 forks source link

actually draw background in Agg version; update to use new matplotlib… #44

Closed janssen closed 7 years ago

janssen commented 7 years ago

… 2.0 optional parameter 'closed_only' when calling to_polygon(). This fixes the bug with matplotlib >= 1.5 not drawing lines that should be there.

matham commented 7 years ago

There seems to be many unrelated things mixed into the pr. Each seperate issue needs its own pr. Also a pr should have reproducible examples of what it's trying to fix. I'm not sure what this is fixing.

janssen commented 7 years ago

It's true. There are three things in this PR: a fix for the matplotlib 1.5+ behavior of to_polygon(), a fix for the use of undefined variable "capstyle_d", and fixes for the extension not properly handling matplotlib "textcolor" and "facecolor". I'm sorry I don't have time to tease them apart, but feel free to do that. Am I the only user of this extension?

matham commented 7 years ago

You may very well be. Or, maybe other people are using older versions so they don't run into issues. We do get new traffic it seems: https://github.com/kivy-garden/garden.matplotlib/graphs/traffic.

On Jan 30, 2017 11:46 AM, "janssen" notifications@github.com wrote:

It's true. There are three things in this PR: a fix for the matplotlib 1.5+ behavior of to_polygon(), a fix for the use of undefined variable "capstyle_d", and fixes for the extension not properly handling matplotlib "textcolor" and "facecolor". I'm sorry I don't have time to tease them apart, but feel free to do that. Am I the only user of this extension?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kivy-garden/garden.matplotlib/pull/44#issuecomment-276115975, or mute the thread https://github.com/notifications/unsubscribe-auth/ABkW_kJexG50WFroAEhDOnTlL0oOnf20ks5rXhP1gaJpZM4LP0b2 .

janssen commented 7 years ago

It also addresses #37, by only setting the Window size of it's non-zero.

janssen commented 7 years ago

I've tested this with matplotlib 2.0.0.

matham commented 7 years ago

The problem with your fix for https://github.com/kivy-garden/garden.matplotlib/pull/37 is that when I run any of the examples in the examples folder the window slowly reduces itself to a size of zero. So I think the fix from https://github.com/kivy-garden/garden.matplotlib/pull/37 is correct as that doesn't have this issue for me.

janssen commented 7 years ago

Well, yes, the fix from #37 just ignores the resize event. If you think that's correct (I don't) by all means use it. My fix (in this PR) seems to do the right thing with both test_plt.py, test_backend.py, and test_events.py. Yes, they do resize the window in a series of steps to the size of the matplotlib figure, which is smaller than the default Kivy window size. Not sure why it's done that way, but they never reach a size of zero.

janssen commented 7 years ago

Just tried it again. All the examples work just fine with #44 applied. I have to say that I don't use andnovar's FigureManagerKivy or NavigationToolbar2Kivy -- I wrote my own to match my toolkit's UI style instead of the matplotlib style. But used with the examples they are fine.

janssen commented 7 years ago

Oh, and I'm testing with matplotlib 2.0.0 on Ubuntu 16.04. Perhaps there's different behavior with 1.4.3?

janssen commented 7 years ago

I installed MPL 1.4.3 and tried it. You're absolutely right -- the examples behave differently, and with 2.0.0 they are smaller than they should be. But it's with the same fix to resize() as in PR 44. So I'm guessing the resize protocol has changed? I'll look into it. (Later) Yes, there's a callback loop. FigureCanvasKivy has a 'size' event handler, '_on_size_changed', which triggers a resize event, which calls FigureManagerKivy.resize(). Given that there's some ambiguity about what "resize" is supposed to do (see https://github.com/matplotlib/matplotlib/blob/38be7aeaaac3691560aeadafe46722dda427ef47/doc/devel/MEP/MEP27.rst), it looks like the correct behavior going forward will be to set the size of the Window, as my code does now. This means we'll have to break the cyclic dependency here. More later.

matham commented 7 years ago

Ok, I'll merge then as soon as you add a fix that works for both versions.

wjanssen commented 7 years ago

I've got to figure out what's actually calling FigureManagerBase.resize() to untangle this. I'll change the actual resize call as you suggest.

janssen commented 7 years ago

The problem is an undocumented API change in matplotlib between 1.4.3 and 2.0.0. The keyword argument 'forward' in matplotlib.figure.set_size_inches() changed default from 'False' to 'True':

    def set_size_inches(self, w, h=None, forward=True):
        """
        set_size_inches(w,h, forward=False)

        Set the figure size in inches (1in == 2.54cm)

        Usage::

             fig.set_size_inches(w,h)  # OR
             fig.set_size_inches((w,h) )

        optional kwarg *forward=True* will cause the canvas size to be
        automatically updated; e.g., you can resize the figure window
        from the shell

        ACCEPTS: a w,h tuple with w,h in inches

I'll update my patch to handle this.

janssen commented 7 years ago

Tested with 1.4.3 and 2.0.0.

matham commented 7 years ago

👍