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

Command to insert text #7908

Merged
merged 6 commits into from Mar 30, 2020
Merged

Conversation

dLamSlo8
Copy link
Contributor

@dLamSlo8 dLamSlo8 commented Feb 21, 2020

References

#4519

Code changes

  • Added interface function for any code editor to insert text.
  • Added individual commands for console, notebook, and full editor to be able to insert text.
  • Added a general command that will allow for inserting text in all the contexts mentioned above at once.

User-facing changes

N/A

Backwards-incompatible changes

N/A

…ral command that applies to all contexts (i.e. console, notebook, full editor)
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 21, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 24, 2020

Thanks for working on this @dLamSlo8! Do you mind adding tests for the codeeditor, notebook and console changes?

@dLamSlo8
Copy link
Contributor Author

@dLamSlo8 dLamSlo8 commented Mar 12, 2020

@blink1073 Hi, I currently have the tests for codeeditor and console, but am unable to get the notebook tests working. For these commands, I realized it requires the editor to be focused first, and when attempting to focus the currently active cell in the notebook tests using cell.editor.focus(), it fails to do so, and thus my tests can't work properly. Do you have an idea as to why that might be the case? In, for example, the console test when I focus the prompt cell, it does actually focus the cell and thus I can insert text.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Mar 13, 2020

Here's what we did in similar tests:

simulate(codeChild.editorWidget.node, 'mousedown');

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

I'm looking at this too (hoping we can get it in for 2.1). Looking at the API, it looks like the content and selection are stored in the model, though view-specific things are in the editor. I think we don't have any other methods on the widget now that explicitly change the text.

Two thoughts about the current API:

  1. Since it is replacing the selected text (as mentioned in the docs), perhaps it would be better to be called replaceSelection (which is the method name in the CodeMirror API as well: https://codemirror.net/doc/manual.html#replaceSelection)

  2. Perhaps this should be a method on the model, where the actual text content is? If we did that, we'd have to also manipulate the selection manually. Or perhaps for now, since it is immediately useful, we keep the method on the editor, and as we develop the model more for real-time collaboration, we transition such methods to the model over time?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 25, 2020

And I realize I was the one proposing these sorts of names and designs in the other issue. This is more a self-critical look at my design and architecture suggestions there than a commentary on your implementation, which seems to do a great job of implementing the thoughts there.

What do you think about the name change and/or putting the methods on the model? I'm particularly not sure if moving things to the model is the right thing to do at this point...

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Mar 25, 2020

@jasongrout will push a rename and then this will be good to go.

jasongrout added 2 commits Mar 25, 2020
The codemirror-extension plugin is for codemirror-specific things, whereas replacing a selection is now a generic editor feature, so this belongs in the file editor extension.
jasongrout added 2 commits Mar 26, 2020
…ments and execute it.

This lets us assign a keyboard shortcut that will try a number of actions and do the first that succeeds. For example, this lets us unify the various replaceSelection commands, which are enabled mutually exclusive, to have a global replace selection command in a single shortcut.

For example:

{
    command: "apputils:run-first-enabled",
    selector: "body",
    keys: ["Ctrl B”],
    args: {
        commands: [                   
            "fileeditor:replace-selection",
            "notebook:replace-selection",
            "console:replace-selection"
            ],
            args: {text: 'ABC’}}

}
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

This should now work as a keyboard shortcut:

{
    command: "notebook:replace-selection",
    selector: ".jp-Notebook",
    keys: ["Accel '"],
    args: {text: 'ABC'}
}

Rather than have a hardcoded function that provides a single command for notebooks, consoles, and editors (which seemed unnecessarily limiting?), I introduced a utility command that lets you compose commands. This will run through each command and execute the first enabled one. For example, here is how to have a single keyboard shortcut to insert text in the console, the file editor, or the notebook, whichever happens to be focused.

{
    command: "apputils:run-first-enabled",
    selector: "body",
    keys: ["Accel '"],
    args: {
        commands: [
            "console:replace-selection",
            "fileeditor:replace-selection",
            "notebook:replace-selection",
            ],
            args: {text: 'ABC'}}
}

Edit: we might be able to change that selector to .jp-Editor to be a bit more focused?
Edit: Removed the :focus in the .jp-Notebook selector

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

I think this is ready for a functional review. I see a few places the docs could be cleaned up, and I haven't looked at the test issues pointed about above.

@jasongrout jasongrout removed their request for review Mar 26, 2020
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 26, 2020

I currently have the tests for codeeditor and console, but am unable to get the notebook tests working.

@dLamSlo8 - did you push your tests to this branch? Would you mind doing so?

Copy link
Member

@saulshanabrook saulshanabrook left a comment

This works for me. Had to remove :focus modifier on notebook though for Jason's example.

@saulshanabrook saulshanabrook merged commit 25fbcf3 into jupyterlab:master Mar 30, 2020
44 of 49 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 30, 2020

This works for me. Had to remove :focus modifier on notebook though for Jason's example.

Yes, that sounds right. I copied that selector from the command-mode shortcuts, but in this case the editor is actually the one selected.

@lock lock bot added the status:resolved-locked label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:apputils pkg:codeeditor pkg:codemirror pkg:console pkg:fileeditor pkg:notebook status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants