Opened 6 years ago
Last modified 5 years ago
#17942 needs_work defect
save() should download files in the notebook
Reported by: | vbraun | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.6 |
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) | 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 (28)
comment:1 Changed 6 years ago by
comment:2 in reply to: ↑ description Changed 6 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 6 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 in reply to: ↑ 3 Changed 6 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 6 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 6 years ago by
- Branch set to u/vbraun/save_should_download_files
comment:7 Changed 6 years ago by
- Commit set to 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 6 years ago by
- Commit changed from 91830de68726ac8b38ed836b4bc9525beb5cf4bd to 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 6 years ago by
- Status changed from new to 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 6 years ago by
- Commit changed from 16cb948578b57908bf77bbb77a9af27972073d4e to e9ad72a6b3848ba30313661baabc78c172780a69
Branch pushed to git repo; I updated commit sha1. New commits:
e9ad72a | fix doctests
|
comment:11 Changed 6 years ago by
- Commit changed from e9ad72a6b3848ba30313661baabc78c172780a69 to abaf94b461b9f11cdcf8af630dbc6ff63a617682
Branch pushed to git repo; I updated commit sha1. New commits:
abaf94b | Merge branch 'develop' into #17942
|
comment:12 Changed 6 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 6 years ago by
- Commit changed from abaf94b461b9f11cdcf8af630dbc6ff63a617682 to 18968d0d8cda2f27fb6d52b16c1888f21f22f887
Branch pushed to git repo; I updated commit sha1. New commits:
18968d0 | Merge Sage-6.7.beta2 into #17942
|
comment:14 Changed 6 years ago by
- Commit changed from 18968d0d8cda2f27fb6d52b16c1888f21f22f887 to c9f1718000fd6d7d440b0c824d85f74235e834af
Branch pushed to git repo; I updated commit sha1. New commits:
c9f1718 | fix documentation build
|
comment:15 Changed 6 years ago by
- Commit changed from c9f1718000fd6d7d440b0c824d85f74235e834af to ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc
Branch pushed to git repo; I updated commit sha1. New commits:
ed9898d | documentation fix
|
comment:16 Changed 6 years ago by
- Commit changed from ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc to 43a6b48bb3042852c707951966298a00eeb2a03d
Branch pushed to git repo; I updated commit sha1. New commits:
43a6b48 | Fix doctests
|
comment:17 Changed 6 years ago by
- Status changed from needs_review to needs_work
does not apply, needs rebase
comment:18 Changed 6 years ago by
- Cc kcrisman added
comment:19 Changed 6 years ago by
- Commit changed from 43a6b48bb3042852c707951966298a00eeb2a03d to 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 5 years ago by
- Commit changed from d8b4b81df437fd970e00659bc1bb982d4066578c to b3a066a6e206efdbf92d84c5405ffcdfae88866e
Branch pushed to git repo; I updated commit sha1. New commits:
b3a066a | Merge 6.10.beta3 into Trac #17942
|
comment:23 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:24 follow-up: ↓ 25 Changed 5 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 in reply to: ↑ 24 Changed 5 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 5 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:27 Changed 5 years ago by
It always makes a link regardless of the file type
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):