Opened 8 years ago
Last modified 6 weeks ago
#17942 needs_work defect
save() should download files in the notebook
Reported by: | vbraun | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | |
Component: | user interface | Keywords: | |
Cc: | gagern, kcrisman | Merged in: | |
Authors: | Volker Braun | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/vbraun/save_should_download_files (Commits, GitHub, GitLab) | Commit: | b3a066a6e206efdbf92d84c5405ffcdfae88866e |
Dependencies: | Stopgaps: |
Description (last modified by )
In a browser-based notebook an object's save()
method should automatically download the saved file and not just let it linger on the remote server.
I'm proposing to do this by returning, from save()
, a SavedFile
object instead of nothing. The SavedFile._rich_repr_
method then returns a custom rich representation which gives browser-based display backends a hook to do something with the saved file. Also, nothing happens if you call save() from within code---only if it hits the displayhook:
sage: my_plot.save() # yes, download sage: _ = my_plot.save() # no automatic download sage: for i in range(10): ....: my_plot.save() # depends sage: for i in range(10): ....: my_plot.save() # no ....: x = x + i
Change History (29)
comment:2 Changed 8 years ago by
Replying to vbraun:
In a browser-based notebook an object's
save()
method should automatically download the saved file
I'm not sure that I agree with this. You might want to save()
the object on the server to load()
it in some Sage session later.
If you want download functionality, I would prefer to use a new .download()
method for that.
comment:3 follow-up: 4 Changed 8 years ago by
Just to clarify, save() should of course still save the file on the remote file system. It just should in addition get the file to the client.
Tying it to the displayhook already gives you a means to control whether or not the file is downloaded. Also, if you really just save a temporary file on the server you are more likely to do that form within code so it wouldn't be downloaded anyway. There could be other switches, maybe foo.save(download=False)
or %download [on|off]
. But just foo.save()
should by default do something sensible. If you don't like the default you can then look into the docs. But if you only notice that save()
doesn't result in a file on your laptop then the user is going to assume that this is the way it is.
comment:4 Changed 8 years ago by
With cloud applications (let's consider sagenb as such an application), "save" usually means "save on the remote system". The notebook interface has "Save" and "Save & quit" buttons, neither of these result in a downloaded file.
comment:5 Changed 8 years ago by
From a user these are two different activities:
- checkpointing the current worksheet
- creating a media file to be used elsewhere
I agree that its confusing that both are called "save", and it all goes back to the time when Word would crash every 30 minutes so you better save your document to three different floppies. Now we still call it save and we often even have a little picture of a floppy disc under it (anyone remember these?), but its really just a pragmatic idiom for checkpointing nowadays (see also https://github.com/ipython/ipython/wiki/IPEP-15:-Autosaving-the-IPython-Notebook).
comment:6 Changed 8 years ago by
Branch: | → u/vbraun/save_should_download_files |
---|
comment:7 Changed 8 years ago by
Commit: | → 91830de68726ac8b38ed836b4bc9525beb5cf4bd |
---|---|
Description: | modified (diff) |
Apparently not so easy in IPython (https://github.com/ipython/ipython/issues/8053), I implemented a moderately hackish workaround.
comment:8 Changed 8 years ago by
Commit: | 91830de68726ac8b38ed836b4bc9525beb5cf4bd → 16cb948578b57908bf77bbb77a9af27972073d4e |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
16cb948 | Add SageNB support, automatic download in the ipython notebook
|
comment:9 Changed 8 years ago by
Authors: | → Volker Braun |
---|---|
Status: | new → needs_review |
SageNB often replaces the cell dom with the saved part even for cells that you don't currently work on. So javascript inside the cell can't really initiate the download or it would do it again all the time. So its just a download link in SageNB.
comment:10 Changed 8 years ago by
Commit: | 16cb948578b57908bf77bbb77a9af27972073d4e → e9ad72a6b3848ba30313661baabc78c172780a69 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
e9ad72a | fix doctests
|
comment:11 Changed 8 years ago by
Commit: | e9ad72a6b3848ba30313661baabc78c172780a69 → abaf94b461b9f11cdcf8af630dbc6ff63a617682 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
abaf94b | Merge branch 'develop' into #17942
|
comment:12 Changed 8 years ago by
This works now in both notebooks, imho it is the cleanest way of expressing what you want to do:
plot(f).save('/tmp/foo.png').show()
comment:13 Changed 8 years ago by
Commit: | abaf94b461b9f11cdcf8af630dbc6ff63a617682 → 18968d0d8cda2f27fb6d52b16c1888f21f22f887 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
18968d0 | Merge Sage-6.7.beta2 into #17942
|
comment:14 Changed 8 years ago by
Commit: | 18968d0d8cda2f27fb6d52b16c1888f21f22f887 → c9f1718000fd6d7d440b0c824d85f74235e834af |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
c9f1718 | fix documentation build
|
comment:15 Changed 8 years ago by
Commit: | c9f1718000fd6d7d440b0c824d85f74235e834af → ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
ed9898d | documentation fix
|
comment:16 Changed 8 years ago by
Commit: | ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc → 43a6b48bb3042852c707951966298a00eeb2a03d |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
43a6b48 | Fix doctests
|
comment:18 Changed 7 years ago by
Cc: | kcrisman added |
---|
comment:19 Changed 7 years ago by
Commit: | 43a6b48bb3042852c707951966298a00eeb2a03d → d8b4b81df437fd970e00659bc1bb982d4066578c |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
d8b4b81 | Merge Sage-6.9.beta2 into t/17942/save_should_download_files
|
comment:22 Changed 7 years ago by
Commit: | d8b4b81df437fd970e00659bc1bb982d4066578c → b3a066a6e206efdbf92d84c5405ffcdfae88866e |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
b3a066a | Merge 6.10.beta3 into Trac #17942
|
comment:23 Changed 7 years ago by
Status: | needs_work → needs_review |
---|
comment:24 follow-up: 25 Changed 7 years ago by
This looks pretty robust. Should we expect any other .save()
methods to implement this SavedFile
type? Such as the usual .sobj
saving... I assume not, but just checking.
Also, this doesn't appear to require any changes in sagenb; I ask because I'm looking at that right now, but I don't see anything that does.
comment:25 Changed 7 years ago by
Replying to kcrisman:
This looks pretty robust. Should we expect any other
.save()
methods to implement thisSavedFile
type? Such as the usual.sobj
saving... I assume not, but just checking.
I don't really have an opinion either way; We could.
Also, this doesn't appear to require any changes in sagenb; I ask because I'm looking at that right now, but I don't see anything that does.
No changes to SageNB required.
comment:26 Changed 7 years ago by
Great, I didn't think so. I guess as long as this does what we intend for now (does it make a link for non-graphics files too? just curious) then this would be a good addition.
comment:29 Changed 6 weeks ago by
Milestone: | sage-6.6 |
---|
The
SavedFile
object could itself have ashow()
method, which may or may not show the file (depending on the display backend capabilities and the saved file):