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

Lock on click #66169

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@YisraelV
Copy link
Contributor

YisraelV commented Jan 7, 2019

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Jan 8, 2019

@sandy081 Do you want to review this or should I?
I am fine doing this, but you changed a lot of code in the output land thus I thought you might be a better fit for the review.

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Jan 9, 2019

@isidorn Will do.

@sandy081 sandy081 assigned sandy081 and unassigned isidorn Jan 9, 2019

@sandy081 sandy081 self-requested a review Jan 9, 2019

@sandy081 sandy081 added the outline label Jan 9, 2019

@sandy081 sandy081 added this to the December/January 2019 milestone Jan 9, 2019

@@ -612,9 +601,7 @@ export class OutputService extends Disposable implements IOutputService, ITextMo
if (!preserveFocus) {
this._outputPanel.focus();
}
})
// Activate smart scroll when switching back to the output panel
.then(() => this.setPrimaryCursorToLastLine());

This comment has been minimized.

@YisraelV

YisraelV Jan 9, 2019

Author Contributor

The reason we probably called this function was to resume scrolling. Now we shouldn't call it anymore because scrolling depends only on the lock action state, and the lock state should persist between channel changes.

@YisraelV

This comment has been minimized.

Copy link
Contributor Author

YisraelV commented Jan 9, 2019

I added some comments in the code review to clarify my intention. Please let me know if it's preferred not to do it.

@sandy081 I now squashed my two commit into 1 since the second one only fixed something I forgot.

Make smart scroll and lock action work together.
Clicking inside the output panel should set the lock on just as if the
lock icon was clicked. Moving to the last line (with ctrl+end or click)
should unset the lock.

@YisraelV YisraelV force-pushed the YisraelV:lockOnClick branch from 3d5bee9 to 47014f7 Jan 12, 2019

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Jan 15, 2019

@YisraelV I liked toggling the lock button while clicking in the panel.

LGTM

@sandy081 sandy081 merged commit 0e6c4b9 into Microsoft:master Jan 15, 2019

2 checks passed

VS Code #20190112.3 succeeded
Details
license/cla All CLA requirements met.
Details
@YisraelV

This comment has been minimized.

Copy link
Contributor Author

YisraelV commented Jan 16, 2019

Thanks @sandy081. And also @isidorn and everyone else who's taken time to review my contributions. Really appreciate the experience and confidence I've gained from this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment