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.

Sign up for GitHub

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

Jump to bottom

Integrate Source Code into Disassembly View #132541

Merged
merged 16 commits into from
Oct 21, 2021
Merged

Integrate Source Code into Disassembly View #132541

merged 16 commits into from
Oct 21, 2021

Conversation

yuehuang010
Copy link
Contributor

@yuehuang010 yuehuang010 commented Sep 7, 2021 •
edited
Loading

Feature: Integrate Source Code into Disassembly View

  • Add Toggle Action to Show/Hide source code.
  • Go-To-Source with line number on double click.
  • Fix line-height to be more precise and default to match Monaco's default.
  • Fixed remove all bp in the BP View to also remove Instruction Breakpoints.
  • Fixed an issue when scrolling too fast to incorrectly load the wrong disassembly.

image
image

This PR fixes some of the issues in #129762

@yuehuang010 yuehuang010 changed the title Dev/yuehuang/main dap Integrate Source Code into Disassembly View Sep 7, 2021
@yuehuang010
Copy link
Contributor Author

@isidorn @roblourens

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Sep 8, 2021
@weinand
Copy link
Contributor

weinand commented Sep 8, 2021

@yuehuang010 while reviewing and verifying the PR, I noticed that I can only set a breakpoint in a disassembly line that has the yellow current location:

2021-09-08_15-34-13 (1)

This must be bug and not the expected behavior, right?

@weinand weinand assigned roblourens and unassigned weinand Sep 8, 2021
@yuehuang010
Copy link
Contributor Author

yuehuang010 commented Sep 8, 2021 •
edited
Loading

@weinand I got a repro, thanks. The click box is squashed due to "flex-end". I need to investigate an alternative. Fixed.

@weinand
Copy link
Contributor

weinand commented Sep 9, 2021

@yuehuang010 thanks, setting breakpoints works now.

@weinand
Copy link
Contributor

weinand commented Sep 9, 2021

@yuehuang010 @roblourens in order to verify the "Show/Hide source code" feature I've added "source/line" attributes to disassembly instructions in Mock Debug (published on the Marketplace): the first instruction from a new source line has a corresponding "source/line" attribute:

2021-09-09_18-10-45 (1)

I noticed a few things:

  • the "Show/Hide source code" command is not discoverable, e.g. there is no context menu or editor action. Only clicking in an instruction with source will show the source.
  • the "Show source code" does not seem to reveal/highlight the correct source line. Only the correct source editor is opened/revealed.
  • hitting an instruction breakpoint sometimes reveals the line at the top of the view port which results in back and forth jumping:
    2021-09-09_18-21-09 (1)

@yuehuang010
Copy link
Contributor Author

the "Show/Hide source code" command is not discoverable, e.g. there is no context menu or editor action. Only clicking in an > instruction with source will show the source.
A context menu is planned but out of scope for this PR. So I added other means like F1 command and Settings UI. This and copy and paste would be added once a context menu is added.

the "Show source code" does not seem to reveal/highlight the correct source line. Only the correct source editor is opened/revealed.
Did you supply add column information? It should select the span. Without column it would jump to line without selection.

@weinand

@weinand
Copy link
Contributor

weinand commented Sep 9, 2021

No, I did not provide a column, only the line. But even the line is not selected.

@yuehuang010
Copy link
Contributor Author

@weinand , just pushed two fixes. The the entire line is selected on open file and stepping should reveal/follow new address.

@yuehuang010
Copy link
Contributor Author

@roblourens Do you have any questions?

@roblourens roblourens added this to the September 2021 milestone Sep 21, 2021
roblourens
roblourens requested changes Sep 21, 2021
View reviewed changes
src/vs/editor/common/config/fontInfo.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugEditorActions.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugEditorActions.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Outdated Show resolved Hide resolved
@roblourens
Copy link
Member

Sep-21-2021 15-25-50
horizontal scrolling is not working correctly for me

@xisui-MSFT
Copy link
Contributor

Sep-21-2021 15-25-50
horizontal scrolling is not working correctly for me

@yuehuang010 Horizontal scrolling has never been supported by WorkbenchTable. Please disable it.

@xisui-MSFT xisui-MSFT mentioned this pull request Sep 24, 2021
Closed
@yuehuang010
Copy link
Contributor Author

Sep-21-2021 15-25-50
horizontal scrolling is not working correctly for me

@yuehuang010 Horizontal scrolling has never been supported by WorkbenchTable. Please disable it.

I missed reverting when testing.

@yuehuang010
Copy link
Contributor Author

@roblourens @weinand @isidorn Could you please review this?

roblourens
roblourens requested changes Oct 12, 2021
View reviewed changes
src/vs/editor/common/config/fontInfo.ts Outdated Show resolved Hide resolved
src/vs/editor/common/config/fontInfo.ts Outdated Show resolved Hide resolved
src/vs/editor/common/config/fontInfo.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debug.contribution.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugEditorActions.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/disassemblyView.ts Outdated Show resolved Hide resolved
@Trass3r Trass3r mentioned this pull request Oct 15, 2021
Closed
@Trass3r Trass3r mentioned this pull request Oct 19, 2021
Closed
@roblourens
Copy link
Member

roblourens commented Oct 20, 2021 •
edited
Loading

Just the two remaining open discussions - when those are resolved, I can merge it. Would like to do that this week :)

@yuehuang010
Copy link
Contributor Author

Just the two remaining open discussions - when those are resolved, I can merge it. Would like to do that this week :)

Pushed.

@roblourens roblourens merged commit df62644 into microsoft:main Oct 21, 2021
@PhilippHaefele PhilippHaefele mentioned this pull request Nov 21, 2021
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xisui-MSFT xisui-MSFT xisui-MSFT left review comments

@roblourens roblourens roblourens approved these changes

Assignees

@roblourens roblourens

Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
Milestone
October 2021
Development

Successfully merging this pull request may close these issues.
4 participants
@yuehuang010 @weinand @roblourens @xisui-MSFT