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

Utilize ObservableMap in models and rendermime #1709

Merged
merged 98 commits into from Feb 20, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 13, 2017

Fixes #1139. Fixes #1697.

  • Renderers now take live objects that have observable .data and .metadata.
  • The output model logic is moved to rendermime/outputmodel because an "OutputWidget" is just a rendermime output.
  • Uses the ObservableMap to make an ObservableJSON subclass which powers the ObservableJSONWidget (was the Metadata.Editor).
  • Cleans up the OutputAreaModel/OutputAreaWidget interfaces so that the execution takes place on the widget, and is the only thing that has the concept of the stdin handling.
  • Converts the notebook and cell metadata to ObservableJSON.
@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 14, 2017

Cool, I would love to help review this probably won't happen until Friday-ish.

/**
* Convert a mime bundle to mime data.
*/
function convertBundle(bundle: nbformat.IMimeBundle): JSONObject {
Copy link
Member

@vidartf vidartf Feb 14, 2017

This function was exposed in #1469. Is this functionality now exposed somewhere else?

Copy link
Member Author

@blink1073 blink1073 Feb 14, 2017

Re-exposed in 91d54fc.

Copy link
Member

@vidartf vidartf Feb 14, 2017

Looking at my dependence on this function again, it might simply be enough to expose getData(), as this now calls convertBundle() before returning. My dependencies all called them in sequence.

Copy link
Member Author

@blink1073 blink1073 Feb 14, 2017

Yeah, that was how we were using it internally as well.

@vidartf
Copy link
Member

@vidartf vidartf commented Feb 14, 2017

It is also slightly confusing that there is both nbformat.IMimeBundle and RenderMime.IMimeBundle, c.f. jupyter/nbformat#68. According to that issue, nbformat.IMimeBundle should probably be renamed (attachments will also be of this type?), but I don't have a constructive suggestion for what to call it.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Feb 14, 2017

Perhaps call the one in rendermime IObservableMimeData?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Feb 14, 2017

Or IMimeModel?

@blink1073 blink1073 changed the title Clean up RenderMime interface Utilize ObservableMap in models and rendermime Feb 14, 2017
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Feb 14, 2017

model :allthethings:

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Feb 17, 2017

This is done. I would have written a shorter letter, but I did not have the time. - Blaise Pascal

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 17, 2017

I have discovered a truly marvelous code review, which this margin is too narrow to contain.

afshin
afshin approved these changes Feb 20, 2017
*/
namespace Private {
/**
* The default strict equality item cmp.
Copy link
Member

@afshin afshin Feb 20, 2017

Not a big deal, but maybe say comparator instead of cmp.

* @param index - The optional order index.
*
* ####Notes
* Negative indices count from the end, so -1 refers to the penultimate index.
Copy link
Member

@afshin afshin Feb 20, 2017

penultimate is second-to-last. Is this correct?

@@ -0,0 +1,202 @@
// Copyright (c) Jupyter Development Team.
Copy link
Member

@afshin afshin Feb 20, 2017

This whole file is gone now in master. What's the best way to keep the new pattern in master? Since there's no rebase required for this PR to go in, I'm not sure.

@afshin afshin merged commit ebdb6ee into jupyterlab:master Feb 20, 2017
2 checks passed
@afshin
Copy link
Member

@afshin afshin commented Feb 20, 2017

In. Sane.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.