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

Keep getting a change on workspace open #194375

Closed
jrieken opened this issue Sep 28, 2023 · 11 comments
Closed

Keep getting a change on workspace open #194375

jrieken opened this issue Sep 28, 2023 · 11 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 28, 2023

Before

 "[typescript][javascript]": {
    "editor.insertSpaces": false,
    "editor.defaultFormatter": "vscode.typescript-language-features",
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.organizeImports": "always"
    }
  },

After

{
  "[typescript][javascript]": {
    "editor.insertSpaces": false,
    "editor.defaultFormatter": "vscode.typescript-language-features",
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.organizeImports": "always"
    }
  },
// ....
  "[javascript]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": "always"
    }
  },
  "[typescript]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": "always"
    }
  },
}
Screenshot 2023-09-28 at 10 29 54
@jrieken jrieken added this to the September 2023 milestone Sep 28, 2023
@jrieken jrieken added the important Issue identified as high-priority label Sep 28, 2023
@bpasero
Copy link
Member

bpasero commented Sep 28, 2023

✋ I actually thought this was intentional and have pushed the change to vscode-docs@vnext branch.

@sandy081
Copy link
Member

@justschen There should not be migration happening in this case right?

jrieken added a commit that referenced this issue Sep 28, 2023
disable settings migration, workaround for #194375
@sandy081
Copy link
Member

I would suggest to check the new value and write only if it is changed.

@justschen
Copy link
Contributor

Sounds look. Looking into this atm.

@justschen
Copy link
Contributor

justschen commented Sep 28, 2023

Resolved the settings changing in scenarios where migration is not needed, but in scenarios where migration is needed, the above change still happens. Ends up looking like this:

{
	"files.autoSave": "onFocusChange",
    "editor.accessibilitySupport": "on",
	"workbench.colorTheme": "Solarized Light",
	"editor.detectIndentation": false,
	"editor.codeActionWidget.showInlineQuickfixes": true,
	"editor.codeActionWidget.includeNearbyQuickfixes": true,
	"[typescript][javascript]": {
		"editor.insertSpaces": false,
		"editor.defaultFormatter": "vscode.typescript-language-features",
		"editor.formatOnSave": true,
		"editor.codeActionsOnSave": {
		  "source.organizeImports": true
		}
	},
   "editor.codeActionsOnSave": {
		"source.removeUnusedImports": "explicit"
	},
	"[javascript]": {
		"editor.codeActionsOnSave": {
			"source.removeUnusedImports": "explicit",
			"source.organizeImports": "explicit"
		}
	},
	"[typescript]": {
		"editor.codeActionsOnSave": {
			"source.removeUnusedImports": "explicit",
			"source.organizeImports": "explicit"
		}
	},
}

Digging into it further, this is because registerEditorSettingMigration doesn't have the concept of anything outside of it's current scope (in this case, editor.codeActionsOnSave), seems to find the setting multiple times for each language.

@sandy081 @jrieken Not sure if there is a better way to handle the migrations but would appreciate some thoughts/insight - some of the more complex examples in https://github.com/microsoft/vscode/blob/main/src/vs/editor/browser/config/migrateOptions.ts are still a tad different from what we want to accomplish here.

@sandy081
Copy link
Member

@justschen I think you are doing the right thing here.

The scenario you are hitting here is a limitation with the IConfigurationService.updateValue api. This is true with all editor settings migrations so it is not new with your migration.

I would recommend you to go with your change ie., do migration only when value has changed.

Filed separate issue to fix this in the update method - #194427 - I would prefer to fix this in next milestone because 1. It is not a regression 2. It is not a simple fix to do and therefore risky to do in the endgame.

@sandy081
Copy link
Member

{
	"[typescript][javascript]": {
		"editor.insertSpaces": false,
		"editor.defaultFormatter": "vscode.typescript-language-features",
		"editor.formatOnSave": true,
		"editor.codeActionsOnSave": {
		  "source.organizeImports": true
		}
	},
	"[javascript]": {
		"editor.codeActionsOnSave": {
			"source.removeUnusedImports": "explicit",
			"source.organizeImports": "explicit"
		}
	},
	"[typescript]": {
		"editor.codeActionsOnSave": {
			"source.removeUnusedImports": "explicit",
			"source.organizeImports": "explicit"
		}
	},
}

To confirm, even though value is migrated to each language independently, the effective value is correct and will be the migrated value. I mean, the value of editor.codeActionsOnSave for typescript and javascript languages will be

{
        "source.removeUnusedImports": "explicit",
        "source.organizeImports": "explicit"
}

@justschen
Copy link
Contributor

Yes - original override with [typescript] [javascript] is unchanged, but the new settings written are changed properly from boolean to respective enum.

@sandy081 sandy081 removed the important Issue identified as high-priority label Sep 29, 2023
@sandy081 sandy081 removed their assignment Sep 29, 2023
@justschen
Copy link
Contributor

closing and deferring rest of issue to #194427

@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2023
@justschen
Copy link
Contributor

justschen commented Nov 30, 2023

@joyceerhl repro:

setting is this:

 "[typescript][javascript]": {
            "editor.insertSpaces": false,
            "editor.defaultFormatter": "vscode.typescript-language-features",
            "editor.formatOnSave": true,
            "editor.codeActionsOnSave": {
                "source.organizeImports": true,
                "source.fixAll": false
            }
          },

should end up like this without duplicating the settings:

 "[typescript][javascript]": {
            "editor.insertSpaces": false,
            "editor.defaultFormatter": "vscode.typescript-language-features",
            "editor.formatOnSave": true,
            "editor.codeActionsOnSave": {
                "source.organizeImports": "explicit",
                "source.fixAll": "never"
            }
          },

example with multiple:

        "[typescript][javascript]": {
            "editor.insertSpaces": false,
            "editor.defaultFormatter": "vscode.typescript-language-features",
            "editor.formatOnSave": true,
            "editor.codeActionsOnSave": {
                "source.organizeImports": false,
                "source.fixAll": "never"
            }
          },
        "[python]": {
            "editor.codeActionsOnSave": {
                "source.organizeImports": true
            }
        },
        "editor.codeActionsOnSave": {
            "source.organizeImports": true
        },

expected to turn into:

        "[typescript][javascript]": {
            "editor.insertSpaces": false,
            "editor.defaultFormatter": "vscode.typescript-language-features",
            "editor.formatOnSave": true,
            "editor.codeActionsOnSave": {
                "source.organizeImports": "never",
                "source.fixAll": "never"
            }
          },
        "[python]": {
            "editor.codeActionsOnSave": {
                "source.organizeImports": "explicit"
            }
        },
        "editor.codeActionsOnSave": {
            "source.organizeImports": "explicit"
        },

@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug verified Verification succeeded labels Dec 1, 2023
@sandy081
Copy link
Member

sandy081 commented Dec 1, 2023

Found following bug while verifying - #199736

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants