-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Properly handle debug console completion #10469
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync debug console with the currently selected session
It seems to me like this isn't working correctly. When clicking on another thread in the threads view (using the JS debugger), the select element gets blanked out. Only when clicking on the actual root debug session the correct option is selected:
2021-11-29.16-13-40.mp4
The issue is probably related to the changes I did in #9613. Due to these changes, some debug sessions share the same console session while others don't. This has to be reflected in the selection logic.
packages/debug/src/browser/console/debug-console-contribution.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally this seems to be working for me, though perhaps @msujew can test some of the intricacies of parent and child sessions, since he's worked on that recently.
Aside from that, a few comments on the implementation.
packages/debug/src/browser/console/debug-console-contribution.tsx
Outdated
Show resolved
Hide resolved
packages/debug/src/browser/console/debug-console-contribution.tsx
Outdated
Show resolved
Hide resolved
packages/debug/src/browser/console/debug-console-contribution.tsx
Outdated
Show resolved
Hide resolved
packages/debug/src/browser/console/debug-console-contribution.tsx
Outdated
Show resolved
Hide resolved
@colin-grant-work Thank you very much for your feedback! I changed the things you mentioned and hopefully everything is now up to par. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes look good to me, and it is now possible to get auto-completion in the debug console. From that perspective, I would consider the PR complete. However, the functionality of the debug console still seems fairly broken, as described here. This PR addresses one of the logs mentioned there: Unknown request: completions
is no longer logged, but I still see Unknown request: evaluate
when entering code in a Mocha debugging session in the Theia repo. I would guess that the cause is similar to the problem here: we're not sending the request to the right session. Would you be interested in addressing that problem as part of this PR, or would you prefer to limit this PR to the completion problem and address execution separately?
@colin-grant-work Ah, I see what you mean and I had a quick look at it. You are right that we also used the wrong session for the request for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest changes, both completion and execution work as expected. 👍
@vince-fugnitto @msujew : Any final remarks, otherwise I would merge this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using the node debugger, the completion does not to work correctly when manually triggering the completion (Ctrl+Space). I'm not completely sure what's wrong there:
2022-01-31.14-39-22.mp4
While typing the completion seems to work correctly, but when triggering it manually, an empty completion box appears (visible at the end of the video). Also, when pressing Ctrl+Space without any input, only the name of the debug session appears.
@msujew Thank you for your fast response! I had a look at this and the box with the debug session name without any input is the expected behavior if you compare it with VS Code. As far as I can see, this is actually what the debug adapter returns. As for the "empty" box, it turns out that this is a message box saying 'No suggestions.' that is just placed "badly". The VS Code suggestion widget shows suggestions above the input but messages like 'Loading' and 'No suggestions.' below. The big difference is that in VS Code the message box is actually properly rendered ABOVE the status bar at the bottom, so you can actually read the message. You can test the suggestion widget in Theia in any editor that supports suggestions. You'll see how the different things are rendered. So the problem we have is that there is not enough space at the bottom of our console views. I tried looking for a pure CSS solution but unfortunately I could not find anything satisfying. The problem is that the Do you, by any chance, have any idea how we can achieve this without restructuring the DOM? Otherwise I'd probably open up a separate issue as it is unrelated to the actual functionality of completion. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-fleck-at Ha, interesting. You're right, VSCode also exhibits the same behavior. Seems to me like the node-debugger extension does not work as expected, but that's not on us to fix ;)
I don't have too much of an issue with the weirdly placed 'No suggestions' box, it just added to my already existing confusion.
So in that regard everything else works as expected and the code looks good to me 👍
@martin-fleck-at : Could you squash, I will merge then |
- Ensure console session is disposed when debug session is destroyed -- Remove monaco completion item provider on disposal - Only trigger console session functionality for matching debug session -- Consider child sessions when searching for valid parents -- Ensure we provide completion items for our own session -- Ensure we provide evaluation for our own session - Sync debug console with the currently selected session
@msujew Thank you for your fast response. Then I'll leave it as is. |
What it does
Fixes #10468
Fixes #10648
How to test
Review checklist
Reminder for reviewers