Fix for bug #735 : Images missing from XML/SVG export #1449

Merged
merged 5 commits into from May 21, 2012

Conversation

Projects
None yet
5 participants
Contributor

icmurray commented Feb 28, 2012

  • clarified error message to exported html
  • popup dialog box appears with error and possible solution

Closes #735.

Owner

minrk commented Mar 3, 2012

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.

Owner

takluyver commented Mar 10, 2012

@icmurray : Let us know if you think you'll have a chance to address Min's comments. Thanks!

Contributor

icmurray commented Mar 12, 2012

@takluyver , @minrk : Hello. Yes, I plan to make Min's improvements. Probably on Wednesday.

Contributor

icmurray commented Mar 28, 2012

@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.

Owner

minrk commented Mar 28, 2012

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.

Contributor

icmurray commented Mar 28, 2012

This will show the dialog exactly once per attempt at export, rather than once per instance of the application.

  • Re-running the export with the same, or further figures will show the dialog once more.
  • Creating a new tab (whether with a new kernel or not), and then running an export will show the dialog once more.

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.

Owner

minrk commented Mar 28, 2012

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).

IPython/frontend/qt/console/rich_ipython_widget.py
+ # 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()):
@fperez

fperez Apr 15, 2012

Owner

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.

IPython/frontend/qt/console/rich_ipython_widget.py
+ self._svg_warning_displayed = False
+
+ # Display this warning exactly once per export attempt.
+ if not getattr(self, '_svg_warning_displayed', False):
@fperez

fperez Apr 15, 2012

Owner

Same as above.

Owner

fperez commented Apr 15, 2012

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.

Contributor

icmurray commented Apr 16, 2012

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...

Owner

fperez commented Apr 16, 2012

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!

Contributor

icmurray commented Apr 23, 2012

Hi there, I've committed a much less convoluted patch; and followed the coding guide.

Let me know if there's anything outstanding.

Owner

minrk commented Apr 23, 2012

Excellent, very clean.

Looks pretty good to me.

Owner

Carreau commented May 11, 2012

@icmurray
Sorry, there is a small conflict, could you rebase it, then force push ?
Conflict might be due to the fact that QtConsole now support inlining jpg, but it shouldn't change your patch.

Thanks.

icmurray added some commits Feb 28, 2012

Improve warning about exporting SVG
 - Improved error message by instructing to regenerate the figure.
 - Only display warning message once per figure per export attempt.
Simplified the reset of the _svg_warning_displayed flag in RichIPytho…
…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.
Tidying of warning dialog strings
... and small expansion of related comments
Contributor

icmurray commented May 17, 2012

@Carreau
I've fixed the conflict, rebased and forced a push to icmurray:master.

Thanks.

Owner

takluyver commented May 21, 2012

@minrk, @Carreau: Is this ready to be merged now? It looks OK to me, and there are no conflicts since the rebase.

Owner

minrk commented May 21, 2012

looks good to me

Owner

takluyver commented May 21, 2012

Great. I'll merge it now, then. Let's see if we can get the issue count back under 300.

takluyver added a commit that referenced this pull request May 21, 2012

Merge pull request #1449 from icmurray/master
Fix for bug #735 : Images missing from XML/SVG export

@takluyver takluyver merged commit b4a3ed4 into ipython:master May 21, 2012

Owner

fperez commented May 22, 2012

Thanks, @takluyver! Sorry for the silence, drowned by the epic battle of #1732...

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1449 from icmurray/master
Fix for bug #735 : Images missing from XML/SVG export
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment