I would expect save-as to work without needing any changes to the REST API or the server. It should update the path for the notebook file in the frontend, and then do a regular save (i.e. a PUT request to the URL of the new file).

This is in line with how 'save as' works in most applications: it changes the file path you're working on, so if you open A and then 'save as' B, subsequent saves will write B until you use 'save as' again.

Some applications have a separate 'save a copy' action, which lets you write file B but keep working on A. I don't think that's a priority to add to the notebook - not many other apps have it.

That's how I started but I thinkthis line stopped me.

I'll make the changes, thanks

Yup, I'd just send the whole contents again (for now, every save does that). The notebook may have changed since it was last saved, so copying the file we saved before may not be right.

@takluyver Don't you think that if "Save As" behaves almost like "Save", the user could accidentally overwrite a notebook, if they are not careful?

I guess "save as" should do an additional check and warn the user that the file exists..

Yes, ideally if the user tries to 'save as' over an existing file, they should be warned about that.

@takluyver Now the user is asked to confirm if they want to overwrite, if a notebook exists with the same name. Let me know if these changes are okay.

I think the frontend stuff (the dialog, updating the URL, etc.) should be able to mostly use the code we already have instatic/notebook/js/savewidget.js to rename a notebook. On this side, I would do what you started out doing on the server side: have an option likecopy_file: true to switch the behaviour from moving to copying.

On this side, I would do what you started out doing on the server side: have an option like copy_file: true to switch the behaviour from moving to copying.

Did you mean for reusing the dialog, I should use a flag ?

Yep, so it would be called like this:

notebook.save_widget.rename_notebook({notebook:notebook,copy_file:true});

From a user perspective, I think the starting point should be the directory the notebook is currently saved in. The REST API should always work with paths from the notebook root, though.

From a user perspective, I think the starting point should be the directory the notebook is currently saved in. The REST API should always work with paths from the notebook root, though.

I.e. "save as" will be restricted to the current dir + all subdirectories therein ?

@takluyver I'm sorry, this PR "fell through the cracks". I will try to wind this up in the next few days

I'm really, really sorry, but I'm not able to understand why the above changes are requested, hence a lot of delay. I'll come out and place my concerns here -
I don't get how save as and rename are symmetrical.
Rename

• patches an existing notebook
• Updates the url and
• changes the kernel session

Save As

• creates a new notebook
• checks that a notebook of the same name does not exist
• Opens the newly created notebook (hence no need to change the session in the kernel ??)
  The only shared code I could find is that we need a very similar dialog box - we could use underscore templates here.

Please let me know why you think "save as" and "rename" should be implemented the same.

Thanks for your patience.

Let me try to put those as I see them:

Rename

• Pick a new name that doesn't already exist
• Ask the server to move the notebook file to the new name
• Update the session so that our kernel is associated with the new name
• Update the frontend (URL, notebook name in the page, possibly other things)

Save as

• Pick a new name that doesn't already exist
• Ask the server to copy the notebook file to the new name
• Update the session so that our kernel is associated with the new name
• Update the frontend (URL, notebook name in the page, possibly other things)

Does that make it clearer?

I think maybe the disconnect is that you're talking about opening the new notebook after 'save as' - are you thinking it would open a second tab, so you would have both notebooks open? It's an interesting idea, but what I describe is the way lots of familiar programs do 'save as' - you have just one editor open afterwards, and it's associated with the new file.

I see. The disconnect was because I implemented "save as" to work with any path relative to the notebook's root_dir, and I don't think that can be implemented with a rename.

I guess that isn't relevant now as we later agreed to implement save asonly in the current directory.

Thanks a lot for the expanation, I will implement "Save As" as rename + save.

I agree with your plan, and think that with this approach we have something really close to what "Make a copy" does, and it would be nice to get both functionality closer. I think that "Save as..." may be more familiar than "Make a copy", and we could try to see if "Make a copy" could be removed in the longer term.

I implemented "save as" to work with any path relative to the notebook's root_dir, and I don't think that can be implemented with a rename.

Ah, my bad. Rename and move are the same operation in the REST API, and I thought we'd made the GUI so that rename could move it to a different folder, but I was wrong about that.

I think it would be easier to decide what to do if we fix on how "Save As" should behave - save a new notebook in the current directory or anywhere relative to root_dir ?

I think it should be able to save anywhere under the server root directory, but in terms of the UI, it should work relative to the directory the notebook is already in. So if I start the notebook server at/home/takluyver and open a notebook/home/takluyver/foo/ThisNotebook.ipynb, then 'Save as' should assume that I want to stay in thefoo folder unless I tell it otherwise.

unless I tell it otherwise.

So, if the user typesbar/copynotebook.ipynb insidefoo, we save under/home/takluyver/foo/bar/copynotebook.ipynb?

I've changed it back to the original implementation which behaved a little like "Make a copy". Even deciding to make it relative to current dir should be as easy as appending$('body').attr('data-notebook-path') to the notebook path, let me know.

So, if the user types bar/copynotebook.ipynb inside foo, we save under /home/takluyver/foo/bar/copynotebook.ipynb?

Yes... but if possible, could we make it so that when the dialog appears,foo/ is already filled in, and the cursor is at the end? So you could type a name to save in the same folder, or backspace to take it to another folder.

I think I got the behavior you wanted now. (Although the save as method has become a callback+promise hell...)

I'd also like to have a basic Selenium test for this - see this folder:https://github.com/jupyter/notebook/tree/master/notebook/tests/selenium

I added one. Also took care of the css issues.

Hi@takluyver@Carreau, any updates here ?

What do you think of adding a HEAD HTTP method to the contents handler? This would simply call contents_manager.file_exists or contents_manager.dir_exists. (I'm not sure if things like this are "done")

It might be better than adding a flag while saving, I don't like how that looks -

if exists:    if model.get('overwrite', True):        yield gen.maybe_future(self._save(model.get("model"), path))     else:         raise web.HTTPError(400, "File with that name exists")

That's roughly whatcontent: false already does, but with the metadata in the JSON model rather than HTTP headers.

@minrk@Carreau@mpacer I'd like to get another pair of eyes on this PR if possible. Thanks!

@Madhu94 It seems to be failing the tests, if you could take a look.

@Madhu94 It seems to be failing the tests, if you could take a look.

".CodeMirror-code" still did not exist" => I thought this was some transient Travis bug, will check

I think that error can mean it's failing to load the notebook page - it waits for a codemirror block to exist to check that the notebook is loading correctly. The fact that it's doing that on all frontend tests suggests that there's a real problem somewhere (though it might be that something else changed at the same time).

I tracked down the test failure to some ES6 syntax in the new javascript. Removing that should let the tests pass.

Indeed it does :-). Thanks Min, and thanks@Madhu94 for your patience with this. Merging now.

Heads up@mpacer that this is another change for 5.6.

@Madhu94@takluyver quick question... I am curious about the implementation here. The tooltip indicates that it starts a new session but seems the intent was to associate the new name with the same kernel session. I wanted to double check on the actual behavior: does it keep the same kernel or start a new one?

@rachelgshaffer it should be keeping the same session and changing the filename associated with it. The 'start a new kernel' language in the tooltip is misleading copy-pasta, I think. Do you want to make a PR to fix that?