Fix for bug #735 : Images missing from XML/SVG export #1449
Conversation
|
Can we have this warning only once? It can get annoying if you have quite a few figures you are trying to save. Also, we should add "and regenerate the figures" onto the end of the message, as just changing the config will not make the same export work. |
|
@icmurray : Let us know if you think you'll have a chance to address Min's comments. Thanks! |
|
@takluyver , @minrk : Hello. Yes, I plan to make Min's improvements. Probably on Wednesday. |
|
@takluyver , @minrk : I've made the suggested improvements. Sorry for the delay, I hadn't realised you wouldn't be notified of new commits being added to the pull request. |
|
Thanks for the ping. It looks like this only shows the dialog once per instance of the application. Does it make sense to reset the flag after the save, so you can tell that each subsequent save didn't work properly? With tabs, restarts, etc. there could be several completely unrelated documents exported from a given application instance, and it makes sense that each one should probably be notified that save didn't work properly. |
|
This will show the dialog exactly once per attempt at export, rather than once per instance of the application.
However, I hadn't known about restarting the kernel, and I'm glad you raised it, as in this situation, the first failed export will fail without a dialog. I'll address this. I remember wanting to do as you've suggested and reset a flag upon the completion of a save. But there was a reason, which I can't remember now, that I didn't: it was probably that I wanted to keep the change contained to that one function, or some notion I had of neatness! Probably related to the fact that a failed SVG export still needs to return a string of XML, and couldn't raise an exception. Anyway, I'll take another look... Thanks for the feedback. |
|
Ah, reading a second time I understand better. I think a simple and conservative choice would be to just clear the flag at the beginning of each save attempt, and continue from there. This should result in exactly one warning on every failed save, regardless of whether the specific SVG failed to export in previous attempts. This seems fine to me, because users should not be trying to export SVGs to PNG, and it's right to shout at them every time they try (as long as try==per-save, not try==per-image). |
| # we've attempted to export this svg previously. If so, then | ||
| # reset the flag that determines whether the warning has been | ||
| # displayed or not. | ||
| if match.group("name") in getattr(self, '_svgs_processed', set()): |
There was a problem hiding this comment.
This shouldn't use getattr() b/c the attribute itself should always be present. Our policy is that all attributes that an object is meant to have through its lifetime should be declared only in one place, the class specification. This makes it much easier to reason about the structure of the object, without having to go through every single method looking for who adds what.
|
I think with a bit of a rework as indicated in my comments, so the required attributes are declared and documented according to our policy of doing so in the class spec, this will be in good shape soon. @icmurray, thanks for the contribution! Looking forward to having it finished up soon, and sorry for the delay in reviewing, we've been a bit swamped. |
|
Hi there, sorry for the delay in finishing this up. I've been pretty swamped too. I should get the chance some point this week... |
|
Totally understood, no worries. We're trying to clear the backlog so all this can go into 0.13, but we're not quite at release freeze point yet, so if you need a few days it's no problem at all. Thanks again for pitching in! |
|
Hi there, I've committed a much less convoluted patch; and followed the coding guide. Let me know if there's anything outstanding. |
|
Excellent, very clean. Looks pretty good to me. |
|
@icmurray Thanks. |
- Improved error message by instructing to regenerate the figure. - Only display warning message once per figure per export attempt.
…nWidget By overriding export_html(), and reseting the flag prior to the html export. This avoids repeated messages for a single document save, but repeats the message for subsequent saves, including in new tabs and across kernel restarts.
... and small expansion of related comments
|
@Carreau Thanks. |
|
looks good to me |
|
Great. I'll merge it now, then. Let's see if we can get the issue count back under 300. |
Fix for bug #735 : Images missing from XML/SVG export
|
Thanks, @takluyver! Sorry for the silence, drowned by the epic battle of #1732... |
Fix for bug ipython#735 : Images missing from XML/SVG export
Closes #735.