Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add save as menu option #3289

Merged
merged 7 commits into from Jun 20, 2018

Conversation

Projects
None yet
5 participants
@Madhu94
Copy link
Contributor

Madhu94 commented Feb 1, 2018

Trying to close #2816

I'd like some quick, initial feedback on this.

I am checking the tests. The UI needs some work too (maybe a directory picker instead of a text box?)

Thanks.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 2, 2018

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.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Feb 2, 2018

That's how I started but I think this line stopped me.

I'll make the changes, thanks

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 2, 2018

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.

@Madhu94 Madhu94 force-pushed the Madhu94:add-save-as-menu-option branch from f97e26e to 50d6297 Feb 6, 2018

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Feb 13, 2018

@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..

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 14, 2018

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

@Madhu94 Madhu94 force-pushed the Madhu94:add-save-as-menu-option branch from 0610bd5 to 21b7710 Mar 11, 2018

@Madhu94 Madhu94 changed the title [WIP] Add save as menu option Add save as menu option Mar 11, 2018

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Mar 11, 2018

@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.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 13, 2018

I think the frontend stuff (the dialog, updating the URL, etc.) should be able to mostly use the code we already have in static/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 like copy_file: true to switch the behaviour from moving to copying.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Mar 13, 2018

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 ?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 13, 2018

Yep, so it would be called like this:

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

@Madhu94 Madhu94 force-pushed the Madhu94:add-save-as-menu-option branch from 21b7710 to 5fdf8c2 Mar 17, 2018

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Mar 17, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 19, 2018

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.

@@ -2845,6 +2845,57 @@ define([
}
};


Notebook.prototype.save_notebook_as = function() {

This comment has been minimized.

@takluyver

takluyver Mar 19, 2018

Member

I'd probably put this method into savewidget.js because of the symmetry with rename.

This comment has been minimized.

@Madhu94

Madhu94 Mar 21, 2018

Author Contributor

Will do!

Notebook.prototype.save_notebook_as = function() {
// If current notebook has some changes, save them
if (this.writable && this.dirty) {
this.save_notebook();

This comment has been minimized.

@takluyver

takluyver Mar 19, 2018

Member

I don't think I'd expect 'save as' to save over the old name as well, though it's a bit complicated by autosave. @minrk @gnestor @Carreau ?

This comment has been minimized.

@Madhu94

Madhu94 Mar 21, 2018

Author Contributor

This is to save the current notebook before navigating away. We did agree that "Save as" would open the newly created notebook (Right? Did I misunderstand?), so I thought we should try to save the current notebook before moving away. I think "make a copy" and "trust" do this too.

This comment has been minimized.

@Carreau

Carreau Apr 20, 2018

Contributor

This is to save the current notebook before navigating away. We did agree that "Save as" would open the newly created notebook (Right? Did I misunderstand?), so I thought we should try to save the current notebook before moving away. I think "make a copy" and "trust" do this too.

If you do open the new notebook, then this will likely be almost identical to "Make a Copy", Should we have "Make a copy" take a parameter which is the new name/path ?

dialog_title: i18n.msg._('Save As'),
message: i18n.msg._('Enter a notebook path relative to notebook dir'),
message_classname: 'save-message',
button_name: 'Save',

This comment has been minimized.

@takluyver

takluyver Mar 19, 2018

Member

This should probably be translatable.

return that.contents.save(nb_path, model)
.then(function(data) {
var dir_path = utils.get_body_data("baseUrl");
window.open(utils.url_path_join(dir_path, "notebooks", data.path), '_self');

This comment has been minimized.

@takluyver

takluyver Mar 19, 2018

Member

There's already code to deal with updating the address bar and other details when the notebook is renamed, based on the notebook_renamed.Notebook event. Instead of calling window.open(), we should make sure that event fires, as on this line:

that.events.trigger('notebook_renamed.Notebook', json);

We should also ensure that the session is updated - that links a kernel to a filename. See the line above the one I just linked to.

This comment has been minimized.

@Madhu94

Madhu94 Mar 21, 2018

Author Contributor

I'm not sure if triggering event will cause the new notebook to open up...I'll check and get back.

*/
SaveWidget.prototype._nb_name_dialog = function(options) {
var that = this;
var dialog_template = _.template(`<p class="<%= classname %>"><%= msg %></p>

This comment has been minimized.

@takluyver

takluyver Mar 19, 2018

Member

Neat! I assume the _.template() makes it translatable?

This comment has been minimized.

@Madhu94

Madhu94 Mar 21, 2018

Author Contributor

No, I'm sorry. Underscore templates are just plain html templates. This was just an attempt to share some of the ui code for "save as" and "rename".

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Mar 21, 2018

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 ?

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 15, 2018

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

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 19, 2018

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.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 19, 2018

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.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 20, 2018

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 as only in the current directory.

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

@@ -32,6 +32,7 @@
data-ws-url="{{ws_url | urlencode}}"
data-notebook-name="{{notebook_name | urlencode}}"
data-notebook-path="{{notebook_path | urlencode}}"
data-server-root="{{server_root}}"

This comment has been minimized.

@Carreau

Carreau Apr 20, 2018

Contributor

I would like to try that without exposing server_root it is potentially sensitive informations, and we tried out best to not expose it anywhere in the UI. The root is usually represented as the Home icon.

As far as I can tell having this information is only for display purpose, so I would just stay with the home icon.

This comment has been minimized.

@takluyver

takluyver Apr 21, 2018

Member

I actually disagree. I don't think server_root is really that sensitive, and it's probably not hard to work out from the list of files, which is already accessible. The last time we thought about this was before we enabled authentication by default; now that we've done that, I think we can be slightly more relaxed about showing this kind of information.

We've been using the home icon for lack of anything better, but I don't think it's great UI:

  • It's easy to confuse with your home directory, especially as the two are sometimes but not always the same. File managers frequently use a very similar icon for your home directory, e.g. in Gnome:
    screenshot from 2018-04-21 08-39-43
  • It obscures where the files are actually stored, if you want to go and work with them outside the notebook. At one point Anaconda's GUI launcher would create a directory like 'Notebooks' to run the server in, which is hard to discover from inside.

We should make sure it comes from the contents manager and works if it's empty, to allow for non-filesystem contents managers. But I think the benefits of showing it outweigh the disadvantages.

@Carreau

This comment has been minimized.

Copy link
Contributor

Carreau commented Apr 20, 2018

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.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 20, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 21, 2018

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 the foo folder unless I tell it otherwise.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 29, 2018

unless I tell it otherwise.

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

@Madhu94 Madhu94 force-pushed the Madhu94:add-save-as-menu-option branch from 5fdf8c2 to d3b29ed Apr 29, 2018

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 29, 2018

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.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 29, 2018

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.

};
return that.contents.save(nb_path, model)
.then(function(data) {
window.open(data.path, '_self');

This comment has been minimized.

@takluyver

takluyver Apr 29, 2018

Member

Here I think we should use the same functions that rename uses to update the session and the page:

that.session.rename_notebook(json.path);
that.events.trigger('notebook_renamed.Notebook', json);

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Apr 29, 2018

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

@takluyver
Copy link
Member

takluyver left a comment

The code and the functionality are looking good now, but I found a few minor UI issues.

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



Notebook.prototype.save_notebook_as = function() {
// If current notebook has some changes, save them, or the copied notebook won't have them.

This comment has been minimized.

@takluyver

takluyver Apr 30, 2018

Member

Is this still needed? We're sending a new copy of the notebook to the new name, so I don't think we need to save it at the old name again first.

).append($('<label />').attr('for', 'save-as-dialog')
.html('<i class="fa fa-home"></i>')
).append(
$('<input/>').attr('type','text').attr('size','25')

This comment has been minimized.

@takluyver

takluyver Apr 30, 2018

Member

This input area doesn't show up well for me. I think you're missing the form-control class that the rename dialog uses:

$('<input/>').attr('type','text').attr('size','25').addClass('form-control')

Also, I hope we can get rid of the inline styling a couple of lines below.

return false;
}
});
d.find('input[type="text"]').focus().select();

This comment has been minimized.

@takluyver

takluyver Apr 30, 2018

Member

This currently selects the path pre-filled in the input, which is not ideal. Can we make it put the cursor at the end of the input already there?

@@ -89,6 +89,9 @@
<li id="copy_notebook"
title="{% trans %}Open a copy of this notebook's contents and start a new kernel{% endtrans %}">
<a href="#">{% trans %}Make a Copy...{% endtrans %}</a></li>
<li id="save_notebook_as"
title="{% trans %}Save a copy of the notebook's contents and start a new kernel{% endtrans %}">
<a href="#">{% trans %}Save as{% endtrans %}</a></li>

This comment has been minimized.

@takluyver

takluyver Apr 30, 2018

Member

This label should end ... to indicate that it brings up a dialog rather than acting immediately.

@Madhu94 Madhu94 force-pushed the Madhu94:add-save-as-menu-option branch from 576d3ea to 84e565c May 12, 2018

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented May 13, 2018

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.

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Jun 13, 2018

Hi @takluyver @Carreau, any updates here ?

$('.save-message').html(`<span style='color:red;'>${msg}</span>`);
});
}
that.contents.get(nb_path, {type: 'notebook'}).then(function(data) {

This comment has been minimized.

@takluyver

takluyver Jun 17, 2018

Member

This should at least use content: false to avoid getting the full content of the other notebook if it does exist. It's also not great in that it assumes any error getting a model means that the file is not there.

The right way to do this is probably to add a parameter when saving, to ask the server to throw an error if the filename already exists. Maybe this way of doing it is good enough for now, though. I'd like to get some other opinions on it.

This comment has been minimized.

@Madhu94

Madhu94 Jun 17, 2018

Author Contributor

This should at least use content: false to avoid getting the full content of the other notebook if it does exist.

This is done, thank you.

It's also not great in that it assumes any error getting a model means that the file is not there.

I will try to think of a better way. Maybe we go back to something like my original implementation which used a flag to toggle between copy/save on the server..

Don't get entire contents of the notebooks, while checking
if it exists
Don't get entire contents of the notebooks, while checking
if it exists
@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Jun 18, 2018

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")
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 18, 2018

That's roughly what content: 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!

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 18, 2018

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

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Jun 18, 2018

@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

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 18, 2018

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).

Remove ES6 syntax from save_notebook_as
ES6 syntax is not available to us
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 20, 2018

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

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 20, 2018

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

@takluyver takluyver merged commit 5766341 into jupyter:master Jun 20, 2018

4 checks passed

codecov/patch 0% of diff hit (target 0%)
Details
codecov/project 73.77% (-1%) compared to dc73f9a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jun 20, 2018

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

@Madhu94

This comment has been minimized.

Copy link
Contributor Author

Madhu94 commented Jun 20, 2018

@minrk minrk added this to the 5.6 milestone Jun 20, 2018

@rachelgshaffer

This comment has been minimized.

Copy link

rachelgshaffer commented Oct 3, 2018

@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?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 22, 2018

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.