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

Issues/77879 #79065

Merged
merged 3 commits into from Aug 15, 2019

Conversation

@mayaswrath
Copy link
Contributor

commented Aug 13, 2019

@msftclas

This comment has been minimized.

Copy link

commented Aug 13, 2019

CLA assistant check
All CLA requirements met.

@isidorn isidorn self-assigned this Aug 14, 2019

@isidorn isidorn added this to the August 2019 milestone Aug 14, 2019

if (debugService.getConfigurationManager().canSetBreakpointsIn(editor.getModel())) {
return debugService.addBreakpoints(modelUri, [{ lineNumber: position.lineNumber }], 'debugEditorActions.toggleBreakpointAction');
}
const lineNumbers = new Set(editor._getCursors().getAll()

This comment has been minimized.

Copy link
@isidorn

isidorn Aug 14, 2019

Contributor

Please use editor.getSelections() instead of editor._getCursors.
That is the proper API (I just double checked)

This comment has been minimized.

Copy link
@mayaswrath

mayaswrath Aug 14, 2019

Author Contributor

When I did this I thought the Selection object was for highlighted text only, realized later on that was not the case. I made this change. In using the Selection object I am only considering the line the cursor is on. I could have done all lines that are selected, since the original did not do this, I did not either.

.filter(cs => cs.modelState.position)
.map(cs => cs.modelState.position.lineNumber));

const promises = Array<Promise<any>>();

This comment has been minimized.

Copy link
@isidorn

isidorn Aug 14, 2019

Contributor

There is no need for this promises array.
You can just say return Promise.all(lineNumbers.map(..

This comment has been minimized.

Copy link
@mayaswrath

mayaswrath Aug 14, 2019

Author Contributor

Made this change. Only reason I was doing that before is because there is the possibly to remove multiple breakpoints from a line, resulting in multiple promises for one iteration. I just wrapped that with a Promise.all so they resolve correctly.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Thanks for your PR. It looks good and it works fine.
However I have left some comments in the code so we improve this. Once you tackle that let me know and I can merge this. Thanks a lot!

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Looks great, thanks a lot for your contribution! Merging in 👏

@isidorn isidorn merged commit 6f13755 into microsoft:master Aug 15, 2019

1 of 2 checks passed

VS Code #20190814.52 failed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.