Opened 5 years ago

Last modified 3 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 vbraun)

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

http://boxen.math.washington.edu/home/vbraun/www/ipython-download-link.png

Change History (28)

comment:1 Changed 5 years ago by vbraun

The SavedFile object could itself have a show() method, which may or may not show the file (depending on the display backend capabilities and the saved file):

sage: my_plot.save(**complicated_options).show()
The Foo Notebook cannot display $complicated_graphics_format
Last edited 5 years ago by vbraun (previous) (diff)

comment:2 in reply to: ↑ description Changed 5 years ago by jdemeyer

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: Changed 5 years ago by vbraun

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.

Last edited 5 years ago by vbraun (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 5 years ago by jdemeyer

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 5 years ago by vbraun

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 5 years ago by vbraun

  • Branch set to u/vbraun/save_should_download_files

comment:7 Changed 5 years ago by vbraun

  • 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 5 years ago by git

  • Commit changed from 91830de68726ac8b38ed836b4bc9525beb5cf4bd to 16cb948578b57908bf77bbb77a9af27972073d4e

Branch pushed to git repo; I updated commit sha1. New commits:

16cb948Add SageNB support, automatic download in the ipython notebook

comment:9 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • 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 5 years ago by git

  • Commit changed from 16cb948578b57908bf77bbb77a9af27972073d4e to e9ad72a6b3848ba30313661baabc78c172780a69

Branch pushed to git repo; I updated commit sha1. New commits:

e9ad72afix doctests

comment:11 Changed 5 years ago by git

  • Commit changed from e9ad72a6b3848ba30313661baabc78c172780a69 to abaf94b461b9f11cdcf8af630dbc6ff63a617682

Branch pushed to git repo; I updated commit sha1. New commits:

abaf94bMerge branch 'develop' into #17942

comment:12 Changed 5 years ago by vbraun

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 5 years ago by git

  • Commit changed from abaf94b461b9f11cdcf8af630dbc6ff63a617682 to 18968d0d8cda2f27fb6d52b16c1888f21f22f887

Branch pushed to git repo; I updated commit sha1. New commits:

18968d0Merge Sage-6.7.beta2 into #17942

comment:14 Changed 5 years ago by git

  • Commit changed from 18968d0d8cda2f27fb6d52b16c1888f21f22f887 to c9f1718000fd6d7d440b0c824d85f74235e834af

Branch pushed to git repo; I updated commit sha1. New commits:

c9f1718fix documentation build

comment:15 Changed 5 years ago by git

  • Commit changed from c9f1718000fd6d7d440b0c824d85f74235e834af to ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc

Branch pushed to git repo; I updated commit sha1. New commits:

ed9898ddocumentation fix

comment:16 Changed 5 years ago by git

  • Commit changed from ed9898ddff71c85e34bd51ccd4cf671e40a3f5dc to 43a6b48bb3042852c707951966298a00eeb2a03d

Branch pushed to git repo; I updated commit sha1. New commits:

43a6b48Fix doctests

comment:17 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply, needs rebase

comment:18 Changed 4 years ago by kcrisman

  • Cc kcrisman added

comment:19 Changed 4 years ago by git

  • Commit changed from 43a6b48bb3042852c707951966298a00eeb2a03d to d8b4b81df437fd970e00659bc1bb982d4066578c

Branch pushed to git repo; I updated commit sha1. New commits:

d8b4b81Merge Sage-6.9.beta2 into t/17942/save_should_download_files

comment:20 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

Fixed

comment:21 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:22 Changed 4 years ago by git

  • Commit changed from d8b4b81df437fd970e00659bc1bb982d4066578c to b3a066a6e206efdbf92d84c5405ffcdfae88866e

Branch pushed to git repo; I updated commit sha1. New commits:

b3a066aMerge 6.10.beta3 into Trac #17942

comment:23 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:24 follow-up: Changed 4 years ago by kcrisman

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 4 years ago by vbraun

Replying to kcrisman:

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.

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 4 years ago by kcrisman

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 4 years ago by vbraun

It always makes a link regardless of the file type

comment:28 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

Note: See TracTickets for help on using tickets.