search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

Easily customize different viewers for PNG, DVI, PDF #11795

Closed 83660e46-0051-498b-a8c1-f7a7bd232b5a closed 11 years ago

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago

Currently, all viewers default to $SAGE_BROWSER if it is set, $BROWSER or some other system-specific defaults (like xdg-open, if present) otherwise.

The original proposal on this ticket was to support environment variables for what is currently

I think instead that if the user wants to change the defaults, then that should happen in their init.sage file. The current patch allows them to add lines like these:

from sage.misc.viewer import viewer
viewer.browser('open -a /Applications/Chrome.app')
viewer.png_viewer('display')
viewer.pdf_viewer('acroread')
viewer.dvi_viewer('/usr/bin/xdvi')

(or of course to use these interactively from the command line) and then the appropriate program will be used.

Apply :

CC: @kcrisman

Component: user interface

Keywords: plot browser

Author: John Palmieri

Reviewer: Nathann Cohen

Merged: sage-5.6.beta1

Issue created by migration from https://trac.sagemath.org/ticket/11795

jhpalmieri commented 13 years ago
comment:1

Oh, good, more environment variables. ;)

jhpalmieri commented 13 years ago
comment:2

On further thought, I think this is a bad idea. These variables are only used within Sage, so rather than environment variables, they should be set in .sage/init.sage. I'm attaching a patch (which needs work: no doctests and not really tested at all), which allows for this sort of behavior:

# init.sage file
from sage.misc.viewer import viewer
viewer.pdf_viewer('open -a /Applications/Adobe\ Reader.app')

Then if a is some Sage object, this opens up Adobe Reader on my Mac:

view(a, pdflatex=True)

(The attached patch also fixes a few bugs in latex.py so that this actually works.)

Opinions about using init.sage instead of environment variables?

jhpalmieri commented 12 years ago
comment:3

Okay, this has doctests and is ready for review.

jhpalmieri commented 12 years ago

Author: John Palmieri

jhpalmieri commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,8 +1,23 @@
 Currently, all viewers default to `$SAGE_BROWSER` if it is set, `$BROWSER` or some other system-specific defaults (like `xdg-open`, if present) otherwise.

-We should IMHO also support environment variables for what is currently
+The original proposal on this ticket was to support environment variables for what is currently

 * `sage.misc.viewer.DVI_VIEWER`,
 * `sage.misc.viewer.PDF_VIEWER`, and
 * `sage.misc.viewer.PNG_VIEWER`.

+I think instead that if the user wants to change the defaults, then that should happen in their init.sage file.  The current patch allows them to add lines like these:
+
+```
+from sage.misc.viewer import viewer
+viewer.browser('open -a /Applications/Chrome.app')
+viewer.png_viewer('display')
+viewer.pdf_viewer('acroread')
+viewer.dvi_viewer('/usr/bin/xdvi')
+```
+(or of course to use these interactively from the command line) and then the appropriate program will be used.
+
+---
+
+Apply [attachment: trac_11795.patch](https://github.com/sagemath/sage/files/ticket11795/trac_11795.patch.gz) to the main Sage library repository.
+
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:5

Just rebased the patch on 5.5.rc0

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:6

Attachment: trac_11795-rev.patch.gz

ellooooooooooooo John !!

Thank you very much for this patch ! I had actually begun to write a (much worse) version of it, then noticed you had already done the job ! I joined a patch adding unimportant details to yours, and there's one question keeping me from giving a positive review to your patch immediately : why isn't _viewer_prefs just a dictionary ?

This should get in quickly :-)

Nathann

jhpalmieri commented 11 years ago

Attachment: trac_11795.v2.patch.gz

jhpalmieri commented 11 years ago

Description changed:

--- 
+++ 
@@ -19,5 +19,5 @@

 ---

-Apply [attachment: trac_11795.patch](https://github.com/sagemath/sage/files/ticket11795/trac_11795.patch.gz) to the main Sage library repository.
+Apply [attachment: trac_11795.v2.patch](https://github.com/sagemath/sage-prod/files/10653663/trac_11795.v2.patch.gz) to the main Sage library repository.
jhpalmieri commented 11 years ago
comment:8

I don't know why _viewer_prefs is not just a dictionary. Maybe that class used to do more things in some prehistoric version of the patch? Anyway, I changed it so it's just a dictionary now.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:9

Well, then...

Thank you again ! :-)

Nathann

Apply trac_11795.v2.patch, trac_11795-rev.patch

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Description changed:

--- 
+++ 
@@ -19,5 +19,6 @@

 ---

-Apply [attachment: trac_11795.v2.patch](https://github.com/sagemath/sage-prod/files/10653663/trac_11795.v2.patch.gz) to the main Sage library repository.
-
+Apply :
+* [attachment: trac_11795.v2.patch](https://github.com/sagemath/sage-prod/files/10653663/trac_11795.v2.patch.gz)
+* [attachment: trac_11795-rev.patch](https://github.com/sagemath/sage-prod/files/10653662/trac_11795-rev.patch.gz)
jdemeyer commented 11 years ago

Reviewer: Nathann Cohen

greg-minshall commented 11 years ago
comment:11

hi. fwiw, i downloaded an applied these two patches on my macosx 10.8 system (using fink). i added the following to my init.sage:

from sage.misc.viewer import viewer
viewer.browser('/sw/bin/w3m')
viewer.dvi_viewer('/sw/bin/xdvi')
viewer.pdf_viewer('/sw/bin/xpdf')
viewer.png_viewer('/sw/bin/display')

and ran simple tests. everything worked just fine. (i had a separate issue with error reporting for non-existent viewers, but i don't think that's in the scope of this patch.)

i would also like the ability to get plot3d to use other than jmol. i suspect that's also out of scope for this patch. so, i'm basically happy. cheers.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:12

Helloooooooooooo !!

Well, as this one has been reviewed and fixes something already let's not disturb it. This being said, if you create a ticket to fix your problem do add me as a Cc, if I understand it the review will be fast :-)

Nathann

greg-minshall commented 11 years ago
comment:13

hello, back.

okay. i've added #13842 (give an error or warning if user specified program does not exist) and #13843 (allow configuration of 3D plot program).

i wasn't sure of who to add as CCs, etc. i'm happy to be educated in the way of the sage...

cheers.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:14

Hellooooooooooo !!!

Well, you can add as a CC whoever may feel involved by the patch. Like me, as I reviewed this, or John (who did all the smart work). This being said, it is very likely that these tickets will remain open for a while unless you write a patch yourself and want it to be reviewed :-)

Nathann

jdemeyer commented 11 years ago

Merged: sage-5.6.beta1