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

⚡️Open node's script at specific line #160

Merged
merged 8 commits into from
May 27, 2022

Conversation

AdrySky
Copy link
Contributor

@AdrySky AdrySky commented May 25, 2022

Description

This PR will enable to open node's script and go to its specific line.

Ways to open script are via context menu and component library.

Pull Request Type

  • Xircuits Core (Jupyterlab Related changes)
  • Xircuits Canvas (Custom RD Related changes)
  • Xircuits Component Library
  • Testing Automation
  • Documentation
  • Others (Please Specify)

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

  1. Drag a node to canvas.
    1. Open context menu
    2. Choose Open Script
    3. It'll go to that node name
  2. (Additional) Ctrl+click or double-click any node on component library(sidebar).
    1. It'll open its script and go to that node name

Tested on?

  • Windows
  • Linux Ubuntu
  • Centos
  • Mac
  • Others (State here -> xxx )

Notes

Need to update JupyterLab to at least 3.4.0v.
Notice a few bugs with this latest JupyterLab update. Probably need to check on that or fix the version.

@AdrySky AdrySky added the enhancement New feature or request label May 25, 2022
@AdrySky AdrySky requested a review from MFA-X-AI May 25, 2022 08:50
@AdrySky AdrySky self-assigned this May 25, 2022
@mansouralawi
Copy link
Member

mansouralawi commented May 25, 2022

Great job, Thank you for the PR.

It works as expected.

node

@mansouralawi
Copy link
Member

I have noticed if a comment inside the component's script matches the component name it will detect both

bug

Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

As @mansouralawi has mentioned, this has to do with the way the go to line is implemented, where the search will break if users mention that component name in a comment or such.

There are 2 ways of remedying this - one is to search using the @ decorator tag + class name, but I think the best way of implementing it is using the feature right after the search command, the go to line feature: https://github.com/jupyterlab/jupyterlab/pull/12204/files

This might require additional information embedded inside the component information though, but I think it's fine to add.

Awesome feature, looking forward to having this one implemented!

@AdrySky
Copy link
Contributor Author

AdrySky commented May 26, 2022

Thanks @mansouralawi & @MFA-X-AI for the review. I didn't consider class name on the docstring. Then, this search function doesn't bode well in this situation. Luckily, @MFA-X-AI found a go-to-line command(which I didn't know it exist) which I just added in the latest commit. Hope you guys can review it again. Thanks.
Edit: One things to note is that the line number data is only available with the latest added node. Old node on canvas don't have that data so just re-added node into canvas should works.

@AdrySky AdrySky requested review from mansouralawi and MFA-X-AI May 26, 2022 03:50
Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

Looks good from my side, opening nodes will properly jumps to that specific line number.
One thing that I wish could've been supported by Jupyterlab is the view control itself so that when jumping to that component class definition it wouldn't be at the bottom but instead the top, but I don't think that's possible from the codemirror text editor plugin itself.

Nonetheless this is definitely a lifesaver feature for a big component libraries, so great work and thanks!

Copy link
Member

@mansouralawi mansouralawi left a comment

Choose a reason for hiding this comment

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

Thank you for the changes.

  • Opening the script by double-click or ctrl+click any node on component library(sidebar) works as expected.

side_bar

issue:

  • Opening the script form the context menu --> open script doesn't go to the specific component.

right_click

@MFA-X-AI
Copy link
Member

MFA-X-AI commented May 26, 2022

@mansouralawi

issue:

  • Opening the script form the context menu --> open script doesn't go to the specific component.

There was an extra edit by @AdrySky, old nodes on the canvas don't have that line number data. Reloading those components will correctly reload the information.

Since reloading .xircuits changes a lot of lines, I think we can do the reloading of all examples before the 1.5 update, just in case add more node related changes. Better yet, maybe a refresh all nodes feature. 😄

@mansouralawi
Copy link
Member

There was an extra edit by @AdrySky, old nodes on the canvas don't have that line number data. Reloading those components will correctly reload the information.

Reloading the node fixed the issue. the new feature behaves as expected now.

@MFA-X-AI MFA-X-AI self-requested a review May 27, 2022 02:27
Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

Just checked the newest commit, I'm very impressed how well this works out. Great thinking!

    newWidget.context.ready.then(() => {
        // Go to end of node's line first before go to its class
        app.commands.execute('codemirror:go-to-line', {
            line: nodeLineNo[0].end_lineno
        }).then(() => {
            app.commands.execute('codemirror:go-to-line', {
                line: nodeLineNo[0].lineno
            })

Here's a gif of how it works now.
open-script
Since it's working well, will merge. Will also add the update previous workflows to the todo list.

@MFA-X-AI MFA-X-AI merged commit 1cb631e into master May 27, 2022
@MFA-X-AI MFA-X-AI deleted the adry/open-specific-line-script branch May 27, 2022 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants