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

Mirror PythonFileEditor and Editor trackers #595

Merged
merged 6 commits into from
Jun 10, 2020

Conversation

karlaspuldaro
Copy link
Member

@karlaspuldaro karlaspuldaro commented Jun 1, 2020

Fixes #586
Fixes #520
Fixes #230
Fixes #127

Enables Create New Console for Editor from the context menu (right-click on the editor).
With the console running, auto-complete feature is accessible from the editor as below:

python-completer

Enables TOC's python generator to work by default via the Editor tracker

Enables all FileEditor feature that are ties to it's tracker, including:

  • themes
  • search

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@karlaspuldaro karlaspuldaro added component:python-editor Python editor area:front-end status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. labels Jun 1, 2020
@karlaspuldaro karlaspuldaro added this to the 1.0.0 milestone Jun 1, 2020
@karlaspuldaro karlaspuldaro marked this pull request as draft June 1, 2020 21:50
@karlaspuldaro
Copy link
Member Author

After doing some investigation, I found that our python editor never sets its own session, which represents a live connection to a session kernel, basically a manager that owns the kernel.

In our current implementation, we create a python kernel and manually runs/stops it, only reading the input once at runtime, and exposing the output when it's done, there is never a live session running (removing the shutdown command after a run is not enough) - hence the option to create a python editor console is disabled from our extension.
Code completer needs to talk to the kernel through its session, so we need to set it up and expose it from the python runner.

@karlaspuldaro
Copy link
Member Author

Instead of re-implementing the auto-complete on python file editor, the team agreed in enabling the existing 'Create console for editor'.
By manually adding create console command to python editor, as suggested, would end up with 2 commands in the context menu, one inherited from fileeditor-extension, and one from python editor.

The create console command was being disabled in python editor because the condition coming from fileeditor-extension was

const isEnabled = () => 
  tracker.currentWidget !== null && tracker.currentWidget === shell.currentWidget;

And it was found that tracker.currentWidget wasn't pointing to PythonFileEditor when it should. The solution was to add PythonFileEditor widget to an editor tracker.
This also solves the option to add a console from the main menu File --> New Console for Editor (before the changes, this option was disabled, with display New Console for 'Activity' )

@karlaspuldaro karlaspuldaro removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Jun 10, 2020
@karlaspuldaro karlaspuldaro marked this pull request as ready for review June 10, 2020 15:43
@karlaspuldaro karlaspuldaro changed the title [WIP] Add completer to python file editor Add completer to python file editor Jun 10, 2020
mirroring the editor tracker has made some code redunant
@ajbozarth ajbozarth changed the title Add completer to python file editor Mirror PythonFileEditor and FileEditor trackers Jun 10, 2020
@ajbozarth ajbozarth changed the title Mirror PythonFileEditor and FileEditor trackers Mirror PythonFileEditor and Editor trackers Jun 10, 2020
@ajbozarth
Copy link
Member

I've pushed an update removing redundant code that is no longer needed once we mirror the PythonEditor tracker with the Editor tracker. I've also updated the title and description for this PR to match the broader scope of this fix, as it now address multiple Issues.

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

Including my updates I've tested this and it addresses all the missing features mentioned. LGTM

As per the doc and previous work by @karlaspuldaro we should use
`inject` rather than add. This change requires the addition of a
missing css selector for the context menu to still work though.
@ajbozarth
Copy link
Member

copying my last commit message her for reference:

Updated to use inject

As per the doc and previous work by @karlaspuldaro we should use
`inject` rather than add. This change requires the addition of a
missing css selector for the context menu to still work though.

@karlaspuldaro
Copy link
Member Author

Using inject instead of add breaks the python editor factory upon reload or changing tabs :(

@ajbozarth
Copy link
Member

Upon further testing and research I've reverted my last commit that switched to inject. As @karlaspuldaro previously mentioned the FileEditor widget includes a check against tracker.currentWidget which is specifically not supported when using inject. I am unclear if this is a mistake on JupyterLab's part or if this is just not actually a use case for inject. I am worried that by using add instead of inject we may run into issues in the future due to two trackers tracking the state of a PythonEditor, as of now it doesn't seem to be causing any problems. So this is still LGTM

@lresende lresende merged commit ecc4e18 into elyra-ai:master Jun 10, 2020
@lresende
Copy link
Member

Applying "Jupyterlab 1.x" as this needs backport to 1.x branch. I have started the backport but needs a little more work as TOC stopped working (we might need a smaller cleanup on 1.x than in 2.x).

lresende pushed a commit to lresende/elyra that referenced this pull request Jun 11, 2020
This tracker change fixes multiple PythonEditor issues:

- Enable autocomplete to work on PythonEditor when using a console
(similar to JupyterLab text editor)

- Fix TOC integration with Python Editor

- Fix Editor theme with PythonEditor

- Enable JupyterLab search with Python Editor

Co-authored-by: Alex Bozarth <ajbozart@us.ibm.com>
akchinSTC pushed a commit that referenced this pull request Jun 11, 2020
This tracker change fixes multiple PythonEditor issues:

- Enable autocomplete to work on PythonEditor when using a console
(similar to JupyterLab text editor)

- Fix TOC integration with Python Editor

- Fix Editor theme with PythonEditor

- Enable JupyterLab search with Python Editor

Co-authored-by: Alex Bozarth <ajbozart@us.ibm.com>
@karlaspuldaro karlaspuldaro deleted the kspuldaro-completer branch January 13, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment