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

Add jump to cursor #14594

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mahadevaiahn
Copy link

What it does

Extend context menu of editor with jump to cursor option. Extend context menu of editor margin with jump to cursor option. The option is visible only during the debugging session and if the debug adapter supports this.

Fixes issue #14500

How to test

  • Install python debugger extension
  • Create a new .py file
  • Copy the below lines, and add a break point in line 3
    a=1
    b=2
    c=3
    d=4
  • When the debugger stops at line 3, open context menu on editor and click jump to cursor to move the instruction pointer to the desired valid line.
  • The option is also available by opening the debugger context menu (right click on editor margin, normally where the break points appear)

Follow-ups

Breaking changes

Attribution

Contributed by MVTec Software GmbH

Review checklist

Reminder for reviewers

Extend context menu of editor with jump to cursor option.
Extend context menu of editor margin with jump to cursor option.
The option is visible only during the debugging session and if the debug adapter supports this.

Fixes issue eclipse-theia#14500

Signed-off-by: Nandish Mahadevaiah <nandish.mahadevaiah@mvtec.com>
Copy link
Member

@msujew msujew 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 @mahadevaiahn for this contribution!

This already looks quite good, I just have a few suggestions to improve on this.

Comment on lines 246 to 249
export const JUMP_TO_CURSOR = Command.toDefaultLocalizedCommand({
id: 'editor.debug.action.jumpToCursor',
label: 'Debug: Jump to Cursor'
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: We usually split the label from the category. I'm not sure why this isn't done for the SHOW_HOVER command.

Suggested change
export const JUMP_TO_CURSOR = Command.toDefaultLocalizedCommand({
id: 'editor.debug.action.jumpToCursor',
label: 'Debug: Jump to Cursor'
});
export const JUMP_TO_CURSOR = Command.toDefaultLocalizedCommand({
id: 'editor.debug.action.jumpToCursor',
category: DEBUG_CATEGORY,
label: 'Jump to Cursor'
});

Copy link
Author

Choose a reason for hiding this comment

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

done

const response: DebugProtocol.GotoTargetsResponse = await this.session.sendRequest('gotoTargets', { source, line: position.lineNumber, column: position.column });

if (response && response.body.targets.length === 0) {
throw new Error('Unable to perform Jump to Cursor as there were no targets');
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: This error should likely not be thrown, but instead result in undefined and a warning in the notifications service. See the original implementation in vscode.

Copy link
Author

Choose a reason for hiding this comment

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

done

if (response && response.body.targets.length === 0) {
throw new Error('Unable to perform Jump to Cursor as there were no targets');
}
const targetId = response.body.targets[0].id;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: It would be great if you could implement a "choose the location" quick pick like it's done in vscode in case there are multiple targets.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @msujew , many thanks for your review, I could not find a debugger which supported multiple targets, as I could not test this feature, I decided not to implement the QuickInputService.pick here.

Comment on lines 70 to 71


Copy link
Member

Choose a reason for hiding this comment

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

Formatting: Please remove the added lines here, they will lead to linting errors.

Copy link
Author

Choose a reason for hiding this comment

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

done

@msujew msujew added vscode issues related to VSCode compatibility debug issues that related to debug functionality labels Dec 9, 2024
…ew changes.

Signed-off-by: nandish mahadevaiah <nandish.mahadevaiah@mvtec.com>
Signed-off-by: nandish mahadevaiah <nandish.mahadevaiah@mvtec.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

2 participants