Skip to content

Tabbed gtk3 figuremanager #2465

Closed
wants to merge 20 commits into from

4 participants

@fariza
fariza commented Sep 26, 2013

Updated description:

Main goals of this backend

  • Be able to group figures under the same window, and under the control of the same toolbar
  • Be able to easily modify the toolbar adding and removing toolitems.

Main characterstics of the multi-figure-backends

  • The configuration of the figures can be rearranged after creation by detach and reparent methods in the manager.
  • The single-window configuration is OFF by default, the user has to specifically set matplotlib.rcParams['backend.single_window'] = True for this multi-figure functionnality to become visible
  • It is possible to choose the window under wich the figure is going to be placed from the figure instantiation with the parent keyword
  • The multi-figure-toolbar has a add_tool method, that allows the user to implement tools and integrate them to the toolbar without modifing the backend
  • It is possible to use the Navigation keybindings even if the toolbar is disabled. Following the idea of #1849
  • There is an extra toolitem in the multi-figure-toolbar that SaveAll the figures under the window, not just the current one.

The PR includes:

  • The base clases to implement
    • multi-figure-backend
    • multi-figure-toolbar
    • tools that integrate with a multi-figure-toolbar
  • GTK3 Multi-figure-Backend implementation
  • Example of use examples/user_interfaces/multifigure_backend_gtk.py

The proposed implementation of the GTK3

  • The backend display figures inside a notebook instead of separate windows.
  • It is possible to detach and destroy the figures directly from the tabs of the notebook (individual detach and destroy buttons)
  • There is a tool for line managent (The lines tool that was with the GTK3 backend was not working due to the use of deprecated libglade.)
  • There is a tool for axes management

I documented base classes following the numpydoc (did my best).

@fariza
fariza commented Sep 28, 2013

Following the idea from PR #1849 the Navigation is always present for the figure manager, even if there is no toolbar.
The key events are directed towards Navigation

@mdboom
Matplotlib Developers member
mdboom commented Oct 23, 2013

I tried to check this out to run it, but it looks like it needs a rebase -- it still contains a number of bugs from master (not in your changes) from a month ago that prevent it from running for me, and it would be good to have those fixes. Would you mind doing a rebase on the current master? They don't apply cleanly, and the resolutions should be clearer to you than to I.

First, I think this is a welcome improvement. Tabbed interfaces are really the norm these days (in most browsers etc.) and the fact that matplotlib opens many different windows seems archaic.

In the meantime, I'll comment on your goals stated above, without commenting on the code just yet.

Be able to group figures under the same window, and under the control of the same toolbar

Great.

Be able to easily modify the toolbar adding and removing toolitems.

I think this is also very welcome, but I'd prefer it if it were separate. When PRs get too big, they get very unwieldy to review and are much harder to get in. If the tabbed stuff depends on the toolbar stuff, why don't we do the toolbar stuff first...

  • The configuration of the figures can be rearranged after creation by detach and reparent methods in the manager.

reparent seems a bit obscure to me. Maybe attach (which feels nice and symmetrical to detach, to me). And maybe we could do this in one step, with a move_to method, which would move a figure into another figure manager?

  • The single-window configuration is OFF by default, the user has to specifically set matplotlib.rcParams['backend.single_window'] = True for this multi-figure functionnality to become visible

That's good, for at least one release, and then in the future, once people are used to this, we could make it the default.

  • It is possible to choose the window under wich the figure is going to be placed from the figure instantiation with the parent keyword

I like the functionality, though again the naming feels a bit off. Maybe window or in_window?

  • The multi-figure-toolbar has a add_tool method, that allows the user to implement tools and integrate them to the toolbar without modifing the backend

Sounds reasonable, though, again, I'd prefer to see this as a separate PR.

  • It is possible to use the Navigation keybindings even if the toolbar is disabled. Following the idea of #1849

Again, I think that belongs in the toolbar PR.

  • There is an extra toolitem in the multi-figure-toolbar that SaveAll the figures under the window, not just the current one.

Nice.

The proposed implementation of the GTK3

Are you willing to take on the other backends where this makes sense? It's a lot of work, but if we have good base class infrastructure, hopefully the changes should be fairly parallel in the other GUI backends.

  • There is a tool for line managent (The lines tool that was with the GTK3 backend was not working due to the use of deprecated libglade.)

I think this is also out-of-scope for this PR. It would be nice to fix that, but as a separate piece of work.

  • There is a tool for axes management

What does this do?

Anyway, I hope that doesn't seem discouraging. In my experience, it's always much easier to review and accept changes that to explicit, well-defined things. I understand some of these are related, but if we could consider them in easy steps, I think this will go better. This is almost big enough of a change (since it requires a number of things to all fall in place) that the MEP process may have made sense in hindsight.

@tacaswell
Matplotlib Developers member

As I mentioned on the mailing list, I think it is a good idea factor the pyplot dependencies out of the figure_manager classes. This looks like it is going to be split up into a bunch of PRs anyway, that re-factoring should at least be considered as part of this process.

My view of this is based on the number of SO questions I see go by which relate to embedding. It is easy enough lots of people want to do it, but not easy enough for everyone to do it smoothly.

@fariza
fariza commented Oct 23, 2013

For the moment I did only the rebase.
I want you to be able to run and see the whole shebang before removing the parts that will be included in other PR's

I tried to check this out to run it, but it looks like it needs a rebase -- it still contains a number of bugs from master (not in your changes) from a month ago that prevent it from running for me, and it would be good to have those fixes. Would you mind doing a rebase on the current master? They don't apply cleanly, and the resolutions should be clearer to you than to I.

Done, or at least I think so (my git experience is not that great)

Be able to easily modify the toolbar adding and removing toolitems.

I think this is also very welcome, but I'd prefer it if it were separate. When PRs get too big, they get very unwieldy to review and are much harder to get in. If the tabbed stuff depends on the toolbar stuff, why don't we do the toolbar stuff first...

The problem here is that some tools need to be informed if the current figure changes (selected tab change).
The way I implemented the addition of those tools was with the adding/removing mechanism.
For example for the "subplot configuration" it needs to change accordingly to the current figure.
I will see what can be done.

The configuration of the figures can be rearranged after creation by detach and reparent methods in the manager.

reparent seems a bit obscure to me. Maybe attach (which feels nice and symmetrical to detach, to me). And maybe we could do this in one step, with a move_to method, which would move a figure into another figure manager?

No problem

It is possible to choose the window under wich the figure is going to be placed from the figure instantiation with the parent keyword

I like the functionality, though again the naming feels a bit off. Maybe window or in_window?

Because of my class naming I was using parent for the "window" but I think it is OK to change it.

The multi-figure-toolbar has a add_tool method, that allows the user to implement tools and integrate them to the toolbar without modifing the backend

Sounds reasonable, though, again, I'd prefer to see this as a separate PR.

As I explained before it is going to be a little bit difficult due to the special tools. But I will try to do it.

It is possible to use the Navigation keybindings even if the toolbar is disabled. Following the idea of #1849

Again, I think that belongs in the toolbar PR.

To have the same toolbar for different figures, it is needed to separate the navigation from the toolbar.
The fact of having the keybindings working without toolbar is just a nice side effect.
If at one moment in the future, all the backends use this "multi-figure-stuff" it will be possible to merge NavigationToolbar2 and Navigation without affecting anybody.

The proposed implementation of the GTK3

Are you willing to take on the other backends where this makes sense? It's a lot of work, but if we have good base class infrastructure, hopefully the changes should be fairly parallel in the other GUI backends.

I have a draft for tk and I could do it in gtk, but I don't have experience in Qt

There is a tool for line managent (The lines tool that was with the GTK3 backend was not working due to the use of deprecated libglade.)

I think this is also out-of-scope for this PR. It would be nice to fix that, but as a separate piece of work.

Yes no problem I will remove this tool

There is a tool for axes management

What does this do?

This tool controls the legend, scale, min/max etc.. the most common things in the axes api.
I will remove it aswell and put it in a separate PR

Anyway, I hope that doesn't seem discouraging. In my experience, it's always much easier to review and accept changes that to explicit, well-defined things. I understand some of these are related, but if we could consider them in easy steps, I think this will go better. This is almost big enough of a change (since it requires a number of things to all fall in place) that the MEP process may have made sense in hindsight.

I never thought of a MEP....
Keep in mind that I did not touch any base class, so it can live in parallel with other backends.

Please check it out and let me know what you think.

@fariza
fariza commented Oct 23, 2013

As I mentioned on the mailing list, I think it is a good idea factor the pyplot dependencies out of the figure_manager classes. This looks like it is going to be split up into a bunch of PRs anyway, that re-factoring should at least be considered as part of this process.

My view of this is based on the number of SO questions I see go by which relate to embedding. It is easy enough lots of people want to do it, but not easy enough for everyone to do it smoothly.

From the mailing list conversation I agree with you, it is important to have this independent.
Please correct me if I'm wrong, but the only pylab dependency that I see is the use of Gfc.
Is this correct?

@fariza
fariza commented Oct 23, 2013

I don't understand the errors that I'm getting, I don't think they are related.
backend_gtk3cairo.py passes cleanly pep8

@tacaswell
Matplotlib Developers member

I think that the import _pyhelper and the Gcf are the only dependencies so ripping it out shouldn't be too hard (create figure_manager_base which has everything but Gcf and than make figure_manager sub-class it to add the Gcf back in).

There should be some thought put into providing (for each toolkit) a widget for a) the just the Figure b) the Figure + toolbar c) a stand alone window. I might have time to work on this in the new year, but if you are ripping this part now, and don't mind, you might as well do this as I think this kind of re-factoring will make it easier to do the multi-tab work on all of the backends.

@tacaswell
Matplotlib Developers member

The problem is that they pass ;) Remove backend_gtkcairo.py from the exclude list in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_coding_standards.py#L109 The exclude list is there so that files could start being tested with out having to overhaul the entire library in one shot. Once a file is clean, this error makes sure it won't stay on the exclude list and hence be able to regress.

@fariza
fariza commented Oct 24, 2013

In line with the idea of different PRs wouldn't it be better to leave the removing of pyplot dependencies for latter?

@fariza
fariza commented Oct 28, 2013

After reviewing @mdboom comments, I will start with the toolbar PR.

But before start taking apart my baby I just to make sure that this is something that you consider acceptable (because I don't want to do this more times than needed).

For this first new PR
I will take most of what I already have and clean it up to remove the multi-figure stuff.

At the end I will have three new base clases.

  • NavigationBase: that subclass NavigationToolbar2 (to be merged when all the backends use the new toolbar) and holds the navigation state and as a natural side effect it gives access to the keybindings without need of the real toolbar.
  • ToolbarBase that implements
    • add_tool
    • remove_tool
    • move_tool
  • ToolBase Base class that is going to be used for tools that can be added to the toolbar

For the moment I will implement this only in Gtk3, and if someday we have everything accepted (that this PR proposes), I will implement in Tk and Gtk

Does this seems like a good plan?
Does it have good chances of success?

I will leave this PR open as the big picture reference.

@tacaswell
Matplotlib Developers member

I think the widget refactor/pyplot de-coupling should also be done first.

But, yes sounds like a good plan. Try to make each PR as small as possible so that people can understand them in a reasonable amount of time.

@fariza
fariza commented Oct 28, 2013

Here is the first small PR separating navigation and toolbar. #2557

@tacaswell My problem is that I am not 100% aware of what the decoupling implies, so I can not say that I will do it on my own. Do you have a draft or description of what has to be done?

@tacaswell
Matplotlib Developers member

As a side note, this issue exists: #2194

@tacaswell
Matplotlib Developers member

@fariza Where did we leave this?

@fariza
fariza commented Feb 16, 2015

@tacaswell I am working on a replacement that uses the MEP22 stuff, more aligned with MEP23 https://github.com/matplotlib/matplotlib/wiki/Mep23
Maybe I should just close this one and wait until MEP22 is merged

@tacaswell
Matplotlib Developers member

That sounds reasonable to me.

Really sorry about the epic odyssey getting these things review and merged as been.

@tacaswell tacaswell closed this Feb 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.