search

diffpy / diffpy.pdfgui

graphical user interface for real space structure refinement to PDF
Other
19 stars 27 forks source link

invalid icon paths for matplotlib 2 #15

Closed pavoljuhas closed 7 years ago

pavoljuhas commented 7 years ago

matplotlib 2 renamed or moved its standard bitmaps for icon files. PDFgui trips when loading them from old paths.

Solution: make icon loading helper compatible with both matplotlib 1 and 2.

pavoljuhas commented 7 years ago

Here is how to reproduce the problem:

# install PDFgui in development mode with matplotlib 2
conda install -c diffpy diffpy.pdfgui
conda remove diffpy.pdfgui
conda install matplotlib=2
python setup.py develop -N

# Demo use of invalid paths in extendedplotframe
python -m diffpy.pdfgui.gui.extendedplotframe

$ python.app -m diffpy.pdfgui.gui.extendedplotframe
Traceback (most recent call last):
  File "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/pjuhas/programs/github/diffpy/diffpy.pdfgui/diffpy/pdfgui/gui/extendedplotframe.py", line 437, in <module>
    app = MyApp(0)
  File "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/site-packages/wx-3.0-osx_cocoa/wx/_core.py", line 8631, in __init__
    self._BootstrapApp()
  File "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/site-packages/wx-3.0-osx_cocoa/wx/_core.py", line 8196, in _BootstrapApp
    return _core_.PyApp__BootstrapApp(*args, **kwargs)
  File "/Users/pjuhas/programs/github/diffpy/diffpy.pdfgui/diffpy/pdfgui/gui/extendedplotframe.py", line 424, in OnInit
    frame = ExtendedPlotFrame(None)
  File "/Users/pjuhas/programs/github/diffpy/diffpy.pdfgui/diffpy/pdfgui/gui/extendedplotframe.py", line 133, in __init__
    self.toolbar = ExtendedToolbar(self.canvas)
  File "/Users/pjuhas/programs/github/diffpy/diffpy.pdfgui/diffpy/pdfgui/gui/extendedplotframe.py", line 51, in __init__
    _load_bitmap('stock_save_as.xpm'),
  File "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/site-packages/matplotlib/backends/backend_wx.py", line 1434, in _load_bitmap
    raise IOError('Could not find bitmap file "%s"; dying' % bmpFilename)
IOError: Could not find bitmap file "/Users/pjuhas/arch/anaconda/envs/cmi-dev/lib/python2.7/site-packages/matplotlib/mpl-data/images/stock_save_as.xpm"; dying
pavoljuhas commented 7 years ago

@chiahaoliu - there are 2 icon path missing - "stock_save_as.xpm" and "stock_close.xpm". They were removed in https://github.com/matplotlib/matplotlib/pull/5626. The "stock_save_as.xpm" can be replaced with "filesave.png".

The "stock_close.xpm" does not seem to have an equivalent in MPL2. I suggest to drop the associated "close" icon from the toolbar, because PDFgui plot windows can be as well closed in a standard way. Can you please make sure that standard accelerator keys for closing a window (⌘W on Mac, "Ctrl-W" on Linux) work?

I tried to replace "xpm" files with "filesave.png" and the plot toolbar looks broken around the 6th icon:

pdfgui-plot-window-mpl2

The ExtendedToolbar class has code to remove the subplots icon, but somehow it stays around in MPL2. Can you investigate how to fix that?

chiahaoliu commented 7 years ago

@pavoljuhas I investigate for a bit and I suspect that this error might come from MPL 2.0 itself. Let me explain how I came to this conclusion.

  1. Inspired by the reference, I tried both DeleteTool and DeleteToolByPos methods from NavigationToolbar2WxAgg, and I found even in MPL1.5 DeleteToolByPos method leaves a broken toolbar as you posted. Luckily we always call DeleteTool in pdfgui but when it goes to MPL 2.0, both methods gives broken toolbar.

  2. I also suspected that might have to do with multiple instantiations, therefore I tried a simple wx backend gui example from MPL. With self.DeleteTool(self.wx_ids['Subplots']) into the __init__, I observed the same results; toolbar is broken in MPL2.0 while it's intact in MPL1.5

Please let me know if above tests are enough to say it's from MPL itself, if it's the case, I can report an issue to it.

Btw, I also confirmed once we remove the close icon, keyboard shortcut doesn't work :(

pavoljuhas commented 7 years ago

Indeed it seems something got broken in matplotlib wx backend. I cannot find any reference to DeleteTool or DeleteToolByPos in matplotlib issues.

@tacaswell - could you please check if there is some obvious error in our code to remove the subplots icon from a wx-embedded matplotlib toolbar? Should we report this as a matplotlib bug?

Btw, I also confirmed once we remove the close icon, keyboard shortcut doesn't work :(

@chiahaoliu - actually the close-window shortcut does not work in PDFgui plot windows at all. I think this has to be enabled for the containing wxFrame. There must be documentation or stack-exchange recipes out there to do so.

tacaswell commented 7 years ago

Overriding https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backend_bases.py#L2734 is probably a better option.

The Delete* methods come from the Wx mixin.

If you want to get the default key handling, connect https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backend_bases.py#L2430 which we do in the FigureManagerBase

_load_bitmap is not public API and I don't think the icon data files should be either, I suggest vendoring the function and data files.

How tied are you to Wx? It still does not have released python 3 compatible bindings (as for as I know) which is going to be a problem in the next year or two.

pavoljuhas commented 7 years ago

Thank you Tom, overriding toolitems in extendedplotframe.ExtendedToolbar and activating window-close key is just what we need.

@chiahaoliu - can you implement those suggestions?

As for the "export plot data" icon, we can reuse the "stock_save_as.xpm" from an earlier version of matplotlib. Just copy it to the icons folder in PDFgui project and then use the diffpy.pdfgui.gui.pdfguiglobals.iconpath function to recover its full path. It might be necessary to convert the icon to PNG format and possibly to B&W for consistency with other icons. That is the only place that calls _load_bitmap so we can completely replace that.

How tied are you to Wx? It still does not have released python 3 compatible bindings (as for as I know) which is going to be a problem in the next year or two.

Very much - all of the GUI code is wxpython and currently there are no plans to switch to some other GUI toolkit.

chiahaoliu commented 7 years ago

@pavoljuhas yup, I am on it. But please allow me some more time as it gets a bit more complicated now.

pavoljuhas commented 7 years ago

sure, no worries.

chiahaoliu commented 7 years ago

@pavoljuhas I put a PR on icon path in #19, please take a look if you have time. Also, is there a way to test pdfgui as a whole with local modifications? I tried python applications\pdfgui but it gave error due to incorrect path as below.

15:09:42: can't open file '/Users/timothyliu/Repo_github/diffpy.pdfgui/src/icons/datasetitem.png' 
....

@tacaswell is there a difference between mpl_connect and the keymap handling in the FigureManagerBase ? I am more familiar with the former and wondering if it can achieve the same result.

Thanks for both of your help!

pavoljuhas commented 7 years ago

Also, is there a way to test pdfgui as a whole with local modifications? I tried python applications\pdfgui but it gave error due to incorrect path as below.

Yes, first install it in the development mode and then run pdfgui. See my comment at the top of this issue. On Mac you may need to run it with python.app - use 

python.app /path/to/bin/pdfgui
# or
python.app -m diffpy.pdfgui.applications.pdfgui