Conversation
replace _load_bitmap with public api by overriding
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 25.67% 25.67% -0.01%
==========================================
Files 90 90
Lines 11921 11924 +3
==========================================
Hits 3061 3061
- Misses 8860 8863 +3
Continue to review full report at Codecov.
|
|
|
||
| def __init__(self, canvas): | ||
| # override class var before init | ||
| NavToolbar.toolitems = CUSTOMIZED_TOOLITEMS |
There was a problem hiding this comment.
Not good - this modifies the base class. Please leave it alone and just define tooltimes above as an attribute of the ExtendedToolbar class:
toolitems = tuple(ti for ti in NavToolbar.toolitems if ti[0] != 'Subplots')| #('Subplots', 'Configure subplots', 'subplots', 'configure_subplots'), | ||
| (None, None, None, None), | ||
| #('Save', 'Save the figure', 'filesave', 'save_figure') | ||
| ) |
There was a problem hiding this comment.
It is better to filter the standard matplotlib toolitems instead of copying them. That way the code would be OK and up to date should matplotlib add some icon or rename handler method. See the next comment.
| (None, None, None, None), | ||
| #('Save', 'Save the figure', 'filesave', 'save_figure') | ||
| ('Export plot data', 'Export plot data to file', | ||
| 'filesave', 'save_figure') |
There was a problem hiding this comment.
This seems to just change the tooltip of the standard MPL "Save the figure" icon to "Export plot data", but the method name is the same - this still just saves the figure.
Actually, we need 2 save icons:
- Save the figure - keep the standard MPL icon here
- Export plot data - should be a new icon with its own event which eventually needs to call
ExtendedPlotFrame.savePlotData. See the MPL sources for meaning oftooltipsentries and specification of their handler functions. Given the existing PDFgui code, the handler function should perhaps post theDATA_SAVE_IDwx event.
| def __init__(self, canvas): | ||
| # override class var to exclude subplots | ||
| self.toolitems = tuple(el for el in NavToolbar.toolitems\ | ||
| if el[0] != 'Subplots') |
There was a problem hiding this comment.
Please define toolitems as class attribute above the __init__ method. Just drop the self. and move whole statement to line 37.
Also, do not use backslash continuation for expressions in parentheses. The parentheses already imply line-continuation. Make sure the 2nd line is aligned as per PEP8 (if under the el above).
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
getting closer, but not ready yet.
| 'Print', 'print graph') | ||
| self.AddSimpleTool(DATA_SAVE_ID, | ||
| _load_bitmap('stock_save_as.xpm'), | ||
| _load_bitmap('filesave.png'), |
There was a problem hiding this comment.
This produces two same-image icons with different effects and still uses non-API MPL function _load_bitmap.
Please grab the "stock_save_as.xpm" from matplotlib repo prior to matplotlib/matplotlib#5626, convert it to PNG and add it to the /icons directory in PDFgui repo. Then use the diffpy.pdfgui.gui.pdfguiglobals.iconpath function instead of _load_bitmap; you might also need wx.Bitmap call to load the PNG image.
Please test the toolbar look with both matplotlib 1 and matplotlib 2. If either looks weird, we may need 2 icon files - a colorful version for MPL-1 and a black-and-white for MPL-2.
There was a problem hiding this comment.
yeah, I just finished this modification, but I encountered something weird. iconpath actually return a path as:
/repo/location/diffpy.pdfgui/src/icons/*
but the icons/ is actually at the same level as src/ in this repo.
So by running python -m diffpy.pdfgui.gui.extendedplotframe, I would have following error:
12:50:42: can't open file '/home/timothy/Documents/Repo/diffpy.pdfgui/src/icons/stock_save_as_mpl2.png' (error 2: No such file or directory)
12:50:42: Failed to load image from file "/home/timothy/Documents/Repo/diffpy.pdfgui/src/icons/stock_save_as_mpl2.png".
that seems to be the same error when I run python.app -m diffpy.pdfgui.applications.pdfgui as mentioned in #15.
Does it come from my local configuration?
Also, since we might have to prepare two files, is matplotlib.__version__ a good approach to check?
Thanks for that comments, they are all very helpful!
There was a problem hiding this comment.
but the icons/ is actually at the same level as src/ in this repo.
Ugh, I see the same problem. Seems to happen only in development-mode installation.
I will fix it later today. In the meantime, create symbolic links src/icons --> ../icons, src/doc --> ../doc in your working directory, but make sure they are not added to git.
There was a problem hiding this comment.
thanks, symlink solves the problem and here is the result of testing whole pdfgui with the grayscale icon in MPL2.0
and the same grayscale icon in MPL1.5.3
Also remove the trailing toolbar separator.
Use the standard `wx.Frame.Close` method to post `wx.EVT_CLOSE`, which is already bound to `ExtendedPlotFrame.onClose`.
- use `matplotlib.rcParamsDefault` instead of `rcParams` to get the same shortcuts regardless of matplotlibrc. - rename `ExtendedPlotFrame.quit_keys` --> `close_keys`, which is more descriptive as for the keys function. - rename method `closeShortcut` --> `mplKeyPress`. Make it easier to add more keyboard shortcuts if needed in the future.
Switch to size and style consistent with other matplotlib 2 icons. The old 16x16 icon looks too small.
|
@chiahaoliu - please check my updates and let me know if it is good to go. The main change is the old icon looked to small - it was 16x16 whereas current matplotlib uses 24x24.
|
|
NOTE: remove unused |
more suitable icon file was added in previous commit 4b24ea2
|
@pavoljuhas Thanks for the cleaning! It covers lots of aspects I didn't think of. I pushed commit of removing unused icon file, please let me know further modification is needed. |
2 participants
This is a PR to fix invalid icon path of pdfgui when it works with matplotlib 2.0.
It addresses half of fix discussed at issue #15 by overriding
toolitemsclass variable ofNavigationToolbar2WxAggclass so that we can avoid touching private method. Here is how the toolbar looks like now:@pavoljuhas since I was trying to avoid non-public API, the position of saving icon and printing icon are exchanged. The functionalities remain the same, please let me know if you are okay with that.
attn: @tacaswell