Ticket #5920 (closed enhancement: fixed)

Opened 16 months ago

Last modified 16 months ago

[with patch, positive review] Implement view(object, viewer='pdf')

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-4.0
Component: interfaces Keywords: view, latex, dvi, pdf
Cc: sage-combinat Author(s):
Report Upstream: Reviewer(s):
Merged in: Work issues:

Description (last modified by nthiery) (diff)

This patch implements: 

sage: view(object, viewer = "pdf")

Typical use cases:

  • you prefer your pdf browser to your dvi browser
  • you want to view latex snippets which are not displayed well in dvi viewers (e.g. tikzpicture)

Potential extensions: view(object, viewer='png'), view(object, viewer='html')

Attachments

view_as_pdf-5920-final.patch Download (3.8 KB) - added by nthiery 16 months ago.

Change History

  Changed 16 months ago by nthiery

  • description modified (diff)

  Changed 16 months ago by nthiery

  • summary changed from Implements view(object, format='pdf') to [with patch, needs review] Implements view(object, format='pdf')

  Changed 16 months ago by nthiery

  • keywords view, latex, dvi, pdf added; latex view removed

follow-up: ↓ 6   Changed 16 months ago by was

It should be 

sage: view(object, viewer='pdf')

for consistency with all the 3d plotting code, which has viewer='tachyon' and viewer='jmol' options.

  Changed 16 months ago by was

  • summary changed from [with patch, needs review] Implements view(object, format='pdf') to [with patch, positive review] Implements view(object, format='pdf')

in reply to: ↑ 4   Changed 16 months ago by mabshoff

Replying to was:

It should be {{{ sage: view(object, viewer='pdf') }}} for consistency with all the 3d plotting code, which has viewer='tachyon' and viewer='jmol' options.

Hmm, how can you give this a positive review in light of this comment? I would much rather have the trivial rename in the original patch before merging it.

Cheers,

Michael

  Changed 16 months ago by nthiery

  • description modified (diff)
  • summary changed from [with patch, positive review] Implements view(object, format='pdf') to [with patch, needs review] Implements view(object, viewer='pdf')

Done in the new update patch. I switched back to needs review, though a quick glance from any of the two of you should do.

  Changed 16 months ago by nthiery

  • description modified (diff)

  Changed 16 months ago by AlexGhitza

  • summary changed from [with patch, needs review] Implements view(object, viewer='pdf') to [with patch, positive review] Implements view(object, viewer='pdf')

Looks good.

follow-up: ↓ 11   Changed 16 months ago by was

  • summary changed from [with patch, positive review] Implements view(object, viewer='pdf') to [with patch, needs work] Implements view(object, viewer='pdf')

in reply to: ↑ 10 ; follow-up: ↓ 12   Changed 16 months ago by nthiery

Replying to was: Which work does it still need?

in reply to: ↑ 11 ; follow-up: ↓ 13   Changed 16 months ago by mabshoff

  • summary changed from [with patch, needs work] Implements view(object, viewer='pdf') to [with patch, positive review] Implement view(object, viewer='pdf')

Replying to nthiery:

Replying to was: Which work does it still need?

This had a positive review by Alex since you addressed William's concern. Why did you change that?

Reinstating positive review.

Cheers,

Michaell

in reply to: ↑ 12   Changed 16 months ago by nthiery

Replying to mabshoff:

This had a positive review by Alex since you addressed William's concern. Why did you change that?

William changed that, and that's precisely what I was puzzled about.

Reinstating positive review.

Thanks.

follow-up: ↓ 15   Changed 16 months ago by mabshoff

  • summary changed from [with patch, positive review] Implement view(object, viewer='pdf') to [with patch, needs rebase] Implement view(object, viewer='pdf')
  • milestone changed from sage-3.4.2 to sage-4.0

This one needs a rebase post 3.4.2: 

sage-3.4.2.rc0/devel/sage$ hg import trac_5920_view_as_pdf-5920-nt.patch 
applying trac_5920_view_as_pdf-5920-nt.patch
patching file sage/misc/latex.py
Hunk #1 succeeded at 894 with fuzz 2 (offset 369 lines).
Hunk #4 FAILED at 575
1 out of 6 hunks FAILED -- saving rejects to file sage/misc/latex.py.rej
abort: patch failed to apply

Once the rebase has been done the positive review can be reinstated [assuming doctests pass obviously ;)].

Cheers,

Michael

in reply to: ↑ 14 ; follow-ups: ↓ 16 ↓ 20   Changed 16 months ago by nthiery

Replying to mabshoff:

This one needs a rebase post 3.4.2:

Done

Pfiew. The workflow overhead has been large on this patch ...

in reply to: ↑ 15 ; follow-up: ↓ 17   Changed 16 months ago by nthiery

Oops, please ignore the updated patch for a second

Changed 16 months ago by nthiery

in reply to: ↑ 16   Changed 16 months ago by nthiery

Replying to nthiery:

Oops, please ignore the updated patch for a second

Finally good to go!

  Changed 16 months ago by mabshoff

  • summary changed from [with patch, needs rebase] Implement view(object, viewer='pdf') to [with patch, needs review] Implement view(object, viewer='pdf')

Marked as needing review again, i.e. that it applies and passes doctests.

Cheers,

Michael

  Changed 16 months ago by AlexGhitza

  • summary changed from [with patch, needs review] Implement view(object, viewer='pdf') to [with patch, positive review] Implement view(object, viewer='pdf')

It applies cleanly to 3.4.2.rc0, passes doctests, and does what it should when I try it out.

in reply to: ↑ 15   Changed 16 months ago by mabshoff

Replying to nthiery:

Pfiew. The workflow overhead has been large on this patch ...

Yeah, given the amount of code this didn't go as smoothly as it should have :)

Cheers,

Michael

  Changed 16 months ago by mabshoff

  • status changed from new to closed
  • resolution set to fixed

Merged in Sage 4.0.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.