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
Merged

Issues/77879 #79065

merged 3 commits into from Aug 15, 2019

Conversation

mayaswrath
Copy link
Contributor

@msftclas
Copy link

msftclas 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

isidorn 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
Copy link
Contributor

isidorn 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
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants