Opened 9 years ago

Closed 9 years ago

#5920 closed enhancement (fixed)

[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 Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

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 (1)

view_as_pdf-5920-final.patch (3.8 KB) - added by nthiery 9 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 9 years ago by nthiery

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

comment:3 Changed 9 years ago by nthiery

  • Keywords dvi pdf added

comment:4 follow-up: Changed 9 years 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.

comment:5 Changed 9 years 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')

comment:6 in reply to: ↑ 4 Changed 9 years 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

comment:7 Changed 9 years 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.

comment:8 Changed 9 years ago by nthiery

  • Description modified (diff)

comment:9 Changed 9 years 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.

comment:10 follow-up: Changed 9 years 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')

comment:11 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by nthiery

Replying to was: Which work does it still need?

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years 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

comment:13 in reply to: ↑ 12 Changed 9 years 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.

comment:14 follow-up: Changed 9 years ago by mabshoff

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

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

comment:15 in reply to: ↑ 14 ; follow-ups: Changed 9 years 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 ...

comment:16 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by nthiery

Oops, please ignore the updated patch for a second

Changed 9 years ago by nthiery

comment:17 in reply to: ↑ 16 Changed 9 years ago by nthiery

Replying to nthiery:

Oops, please ignore the updated patch for a second

Finally good to go!

comment:18 Changed 9 years 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

comment:19 Changed 9 years 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.

comment:20 in reply to: ↑ 15 Changed 9 years 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

comment:21 Changed 9 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 4.0.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.