Skip to content
This repository was archived by the owner on Sep 1, 2022. It is now read-only.

Use models for file opening #104

Merged
merged 8 commits into from
Jan 28, 2016
Merged

Conversation

blink1073
Copy link
Contributor

@jasongrout, previously we were throwing away information we already had. I will make the required updates to jupyter-js-plugins.

return this.manager.save(model.path, model);
let model = AbstractFileHandler.modelProperty.get(widget);
return this.getState(widget).then(contents => {
return this.manager.save(model.path, contents);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like how this doesn't let the widget change the filepath by returning a different state. Good security catch.

@jasongrout
Copy link
Member

Comments are inline, though. Maybe the widget should verify that the format and type is something that it can handle? Before it just registered to handle an extension. Perhaps now it should register for a type/format instead?

@blink1073
Copy link
Contributor Author

Updated, version bumped.

*
* #### Notes
* Subclasses are free to use any or none of the information in
* the model.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this comment up on the abstract class too. That's what I use for the notebook, for example, so that's where I would have looked for the docs like this.

@blink1073
Copy link
Contributor Author

Updated.

@jasongrout
Copy link
Member

I think this is the wrong way to open a terminal, unless we implement terminals backed by files, and open the "file" to have a terminal. Perhaps it's wrong to have the file browser be the place to open an arbitrary widget like a terminal.

Edit: but let's put this in for now and think about it more.

@blink1073
Copy link
Contributor Author

Seems fair.

@jasongrout
Copy link
Member

This is a breaking change, so version should be bumped to 0.7.0.

@blink1073
Copy link
Contributor Author

I already bumped to 0.6 in this PR.

@jasongrout
Copy link
Member

ah, okay. Thanks.

jasongrout added a commit that referenced this pull request Jan 28, 2016
@jasongrout jasongrout merged commit 6f1986b into jupyter:master Jan 28, 2016
@blink1073 blink1073 deleted the give-models branch February 10, 2016 20:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants