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

Create new view of cell in cell context menu #3159

Merged
merged 13 commits into from Oct 27, 2017

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 26, 2017

This adds a "Create new view of cell" context menu entry to make a new cell widget tied to the same model.

It exposes a createCell command on the notebook object - thoughts on if this should be exposed as a public method? It seems like we took pains to make cell creation private, but it just means we need to duplicate that code when we do want the notebook to create a new cell and hand it to us. But perhaps this is an esoteric enough case that it doesn't warrant the API addition to notebook.

Fixes #2073

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 26, 2017

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

We had previously considered adding a .clone() function to a cell widget that we could also use in cell tools.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

Actually, we should probably have different logic for that that is shared with creating a drag image for a cell. Either way, I think Cell.clone() makes sense.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 26, 2017

I also thought about doing the clone at the cell level, but the notebook is the one that has the factory for creating cell widgets from models, and it may modify the cell widget after it is created (for example, it adds a class to code cells currently).

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 26, 2017

the notebook is the one that has the factory for creating cell widgets from models

Though a counterpoint is that the notebook cell factory may make assumptions about the cell being part of the notebook, rather than in a different tab.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 26, 2017

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 26, 2017

@blink1073 - comments now?

@jasongrout jasongrout changed the title WIP create new view of cell context menu WIP create new view of cell in cell context menu Oct 26, 2017
@@ -312,6 +312,17 @@ class Cell extends Widget {
}

/**
* Clone the cell, using the same model.
*/
clone() {
Copy link
Member

@blink1073 blink1073 Oct 27, 2017

Choose a reason for hiding this comment

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

Missing a return type.

Copy link
Contributor Author

@jasongrout jasongrout Oct 27, 2017

Choose a reason for hiding this comment

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

I couldn't figure out to make the type this, so I added explicit types, which meant I needed to add an explicit clone method to RawCell to get the typing right.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

I like it!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

With jupyter-widgets/ipywidgets#1779, we can verify a linked widget:

@maartenbreddels
Copy link
Contributor

@maartenbreddels maartenbreddels commented Oct 27, 2017

This will make many people really happy :) At least me! 👍

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

With jupyter-widgets/ipywidgets#1779, we can verify a linked widget:

Nice! Thanks for that PR.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

One glitch I noticed with this - the CodeMirror refresh() does not seem to be taking effect at the very start.

  1. Create a new view of a cell
  2. click on the new Cell tab

What I see is the codemirror not auto-sized for the code. As soon as I click inside the codemirror, or move the tab, codemirror auto-fits to the code size (presumably because of another refresh() call).

I traced it a bit yesterday - the codemirror refresh is called in onShow, but perhaps the codemirror is not ready to be refreshed at that point?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

When the widget is shown it is not yet properly sized. This change defers the refresh to allow the CSS rules to be applied:

diff --git a/packages/codeeditor/src/widget.ts b/packages/codeeditor/src/widget.ts
index 8fca1c930..7743e6b3a 100644
--- a/packages/codeeditor/src/widget.ts
+++ b/packages/codeeditor/src/widget.ts
@@ -76,7 +76,7 @@ class CodeEditorWrapper extends Widget {
   protected onAfterAttach(msg: Message): void {
     super.onAfterAttach(msg);
     if (this.isVisible) {
-      this.editor.refresh();
+      this.update();
     }
   }

@@ -84,7 +84,7 @@ class CodeEditorWrapper extends Widget {
    * A message handler invoked on an `'after-show'` message.
    */
   protected onAfterShow(msg: Message): void {
-    this.editor.refresh();
+    this.update();
   }

   /**
@@ -98,6 +98,10 @@ class CodeEditorWrapper extends Widget {
     }
   }

+  protected onUpdateRequest(msg: Message): void {
+      this.editor.refresh();
+  }
+
   /**
    * Handle a change in model selections.
    */

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

@blink1073 - that works great! I committed and pushed to this PR.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

I think this is good now.

@jasongrout jasongrout changed the title WIP create new view of cell in cell context menu Create new view of cell in cell context menu Oct 27, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

I submitted a PR against yours.

Add docstring and update tests
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

Thanks. I think you could have just pushed to my branch as well.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Still one more test to fix, upcoming.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 27, 2017

feel free to just push to my branch.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Yep, should be good to go now.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 27, 2017

Thanks!

@blink1073 blink1073 merged commit 01c9a6f into jupyterlab:master Oct 27, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Nov 8, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants