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

Fixes for workspaceState sync #182234

Merged
merged 6 commits into from May 22, 2023
Merged

Fixes for workspaceState sync #182234

merged 6 commits into from May 22, 2023

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented May 11, 2023 •
edited

For #179898

@joyceerhl joyceerhl self-assigned this May 11, 2023
@bpasero bpasero self-requested a review May 14, 2023 16:47
bpasero
bpasero reviewed May 14, 2023
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Once workspace state becomes part of settings sync, do we not have to make sure we set it during this.storageService.onWillSaveState?

I think when the user triggers the "Continue on" flow, you will have to request all components to persist their state because otherwise it is not guaranteed to be up to date?

bpasero
bpasero requested changes May 14, 2023
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Once workspace state becomes part of settings sync, do we not have to make sure we set it during this.storageService.onWillSaveState?

I think when the user triggers the "Continue on" flow, you will have to request all components to persist their state because otherwise it is not guaranteed to be up to date?

@joyceerhl joyceerhl marked this pull request as ready for review May 21, 2023 21:52
@joyceerhl joyceerhl requested a review from bpasero May 21, 2023 21:52
@VSCodeTriageBot VSCodeTriageBot added this to the May 2023 milestone May 21, 2023
bpasero
bpasero previously requested changes May 22, 2023
View reviewed changes
src/vs/workbench/contrib/scm/browser/scmViewPane.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/scm/browser/scmViewPane.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/scm/browser/scmViewPane.ts Outdated Show resolved Hide resolved
@bpasero bpasero requested review from lszomoru and bpasero May 22, 2023 04:53
@bpasero
Copy link
Member

bpasero commented May 22, 2023

I am yielding for git component owners, no objections from my end.

@bpasero bpasero removed their request for review May 22, 2023 04:59
@joyceerhl joyceerhl removed the request for review from lszomoru May 22, 2023 15:15
@joyceerhl joyceerhl changed the title Transfer SCM view state in Continue On Fixes for workspaceState sync May 22, 2023
@joyceerhl
Copy link
Contributor Author

fyi, I moved the SCM-specific changes into https://github.com/microsoft/vscode/pull/183127/files as I discovered a few necessary fixes while implementing this PR.

@joyceerhl joyceerhl enabled auto-merge (squash) May 22, 2023 16:13
@joyceerhl joyceerhl dismissed bpasero’s stale review May 22, 2023 16:20

Addressed comments

@joyceerhl joyceerhl merged commit 770d321 into main May 22, 2023
6 checks passed
@joyceerhl joyceerhl deleted the dev/joyceerhl/roasted-crane branch May 22, 2023 16:32
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bpasero bpasero bpasero left review comments

@Tyriar Tyriar Tyriar approved these changes

Assignees

@joyceerhl joyceerhl

Labels
None yet
Projects
None yet
Milestone
May 2023
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@joyceerhl @bpasero @Tyriar @VSCodeTriageBot