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

Adopt StorageTarget #109967

Closed
bpasero opened this issue Nov 4, 2020 · 7 comments
Closed

Adopt StorageTarget #109967

bpasero opened this issue Nov 4, 2020 · 7 comments
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders workbench-state UI state across restarts
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Nov 4, 2020

If you own a call to IStorageService#store, please adopt IStorageService#store2 where a new argument is required:

  • StorageTarget.USER: sync it across machines
  • StorageTarget.MACHINE: do NOT sync it

As a rule of thumb, StorageTarget.USER should be used in almost all cases unless the state is very specific to the machine (e.g. file paths are used, or window dimensions and sizes).

Note: even though we do not sync workspace UI state yet, it would still be good to adopt this new method so that we are ready in the future.

Here is a list of areas:
StorageService

Memento (look for legacygetMemento)

Fyi I will go over a few not mentioned here that are straight forward to adopt.

@alexr00
Copy link
Member

alexr00 commented Nov 4, 2020

@bpasero how does this interact with StorageScope.WORKSPACE?

@RMacfarlane RMacfarlane added the debt Code quality issues label Nov 4, 2020
@bpasero
Copy link
Member Author

bpasero commented Nov 4, 2020

@alexr00 I tried to explain this above:

Note: even though we do not sync workspace UI state yet, it would still be good to adopt this new method so that we are ready in the future.

Today we don't sync workspace UI state, but we may in the future. Only put StorageTarget.USER for StorageScope.WORKSPACE if the data does not contain machine specific things (like file paths).

jrieken added a commit that referenced this issue Nov 4, 2020
@jrieken jrieken removed their assignment Nov 4, 2020
@alexr00
Copy link
Member

alexr00 commented Nov 4, 2020

I should have read more carefully, thank you.

@bpasero bpasero added this to the November 2020 milestone Nov 4, 2020
isidorn added a commit that referenced this issue Nov 4, 2020
@isidorn isidorn removed their assignment Nov 4, 2020
Tyriar added a commit that referenced this issue Nov 4, 2020
@Tyriar Tyriar removed their assignment Nov 4, 2020
@sandy081 sandy081 removed their assignment Nov 4, 2020
sandy081 added a commit that referenced this issue Nov 4, 2020
@vscodebot vscodebot bot added the workbench-state UI state across restarts label Nov 5, 2020
@bpasero
Copy link
Member Author

bpasero commented Nov 5, 2020

Thanks for a lot of changes already. I forgot to list the references to memento object that also needs adoption. The new list is:

StorageService

Memento (look for legacygetMemento)

alexr00 added a commit that referenced this issue Nov 5, 2020
@alexr00 alexr00 removed their assignment Nov 5, 2020
aeschli added a commit that referenced this issue Nov 5, 2020
@aeschli aeschli removed their assignment Nov 5, 2020
meganrogge added a commit that referenced this issue Nov 5, 2020
@meganrogge meganrogge removed their assignment Nov 5, 2020
JacksonKearl pushed a commit that referenced this issue Nov 6, 2020
@JacksonKearl JacksonKearl removed their assignment Nov 6, 2020
sbatten added a commit that referenced this issue Nov 9, 2020
@sbatten sbatten removed their assignment Nov 9, 2020
mjbvz added a commit that referenced this issue Nov 9, 2020
@mjbvz mjbvz removed their assignment Nov 10, 2020
@bpasero
Copy link
Member Author

bpasero commented Nov 11, 2020

Thanks, only a few left. Ping @rebornix & @RMacfarlane

@RMacfarlane RMacfarlane removed their assignment Nov 11, 2020
bpasero added a commit that referenced this issue Nov 13, 2020
@bpasero
Copy link
Member Author

bpasero commented Nov 13, 2020

Down to only these:

image

rebornix added a commit that referenced this issue Nov 17, 2020
@rebornix
Copy link
Member

@bpasero I added ScopeTarget in notebook world, the legacygetMemto can be removed now.

@rebornix rebornix removed their assignment Nov 17, 2020
meganrogge pushed a commit that referenced this issue Nov 18, 2020
meganrogge pushed a commit that referenced this issue Nov 18, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders workbench-state UI state across restarts
Projects
None yet
Development

No branches or pull requests

15 participants