From 2a4ed8474889817be7266f7e675892fbb055195d Mon Sep 17 00:00:00 2001 From: Benjamin Christopher Simmonds <44439583+benibenj@users.noreply.github.com> Date: Wed, 18 Dec 2024 00:39:13 +0100 Subject: [PATCH] Ensure settings and keybindings views open at correct location (#236412) * make sure settings and keybindings views open at correct location * fix tests --- .../browser/preferences.contribution.ts | 54 ++++++++++++------- .../preferences/browser/settingsEditor2.ts | 2 +- .../preferences/browser/preferencesService.ts | 41 ++++++++------ .../preferences/common/preferences.ts | 7 ++- .../test/browser/preferencesService.test.ts | 38 ++++++------- 5 files changed, 87 insertions(+), 55 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/preferences.contribution.ts b/src/vs/workbench/contrib/preferences/browser/preferences.contribution.ts index 175e8bd518b00..2a6357e15d456 100644 --- a/src/vs/workbench/contrib/preferences/browser/preferences.contribution.ts +++ b/src/vs/workbench/contrib/preferences/browser/preferences.contribution.ts @@ -47,6 +47,9 @@ import { IUserDataProfileService, CURRENT_PROFILE_CONTEXT } from '../../../servi import { IUserDataProfilesService } from '../../../../platform/userDataProfile/common/userDataProfile.js'; import { isCodeEditor } from '../../../../editor/browser/editorBrowser.js'; import { Categories } from '../../../../platform/action/common/actionCommonCategories.js'; +import { resolveCommandsContext } from '../../../browser/parts/editor/editorCommandsContext.js'; +import { IEditorGroup, IEditorGroupsService } from '../../../services/editor/common/editorGroupsService.js'; +import { IListService } from '../../../../platform/list/browser/listService.js'; const SETTINGS_EDITOR_COMMAND_SEARCH = 'settings.action.search'; @@ -791,9 +794,10 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon ] }); } - run(accessor: ServicesAccessor, args: string | undefined) { - const query = typeof args === 'string' ? args : undefined; - return accessor.get(IPreferencesService).openGlobalKeybindingSettings(false, { query }); + run(accessor: ServicesAccessor, ...args: unknown[]) { + const query = typeof args[0] === 'string' ? args[0] : undefined; + const groupId = getEditorGroupFromArguments(accessor, args)?.id; + return accessor.get(IPreferencesService).openGlobalKeybindingSettings(false, { query, groupId }); } })); this._register(MenuRegistry.appendMenuItem(MenuId.MenubarPreferencesMenu, { @@ -834,8 +838,9 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon ] }); } - run(accessor: ServicesAccessor) { - return accessor.get(IPreferencesService).openGlobalKeybindingSettings(true); + run(accessor: ServicesAccessor, ...args: unknown[]) { + const groupId = getEditorGroupFromArguments(accessor, args)?.id; + return accessor.get(IPreferencesService).openGlobalKeybindingSettings(true, { groupId }); } })); this._register(registerAction2(class extends Action2 { @@ -852,8 +857,9 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon ] }); } - run(accessor: ServicesAccessor) { - const editorPane = accessor.get(IEditorService).activeEditorPane; + run(accessor: ServicesAccessor, ...args: unknown[]) { + const group = getEditorGroupFromArguments(accessor, args); + const editorPane = group?.activeEditorPane; if (editorPane instanceof KeybindingsEditor) { editorPane.search('@source:system'); } @@ -873,8 +879,9 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon ] }); } - run(accessor: ServicesAccessor) { - const editorPane = accessor.get(IEditorService).activeEditorPane; + run(accessor: ServicesAccessor, ...args: unknown[]) { + const group = getEditorGroupFromArguments(accessor, args); + const editorPane = group?.activeEditorPane; if (editorPane instanceof KeybindingsEditor) { editorPane.search('@source:extension'); } @@ -894,8 +901,9 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon ] }); } - run(accessor: ServicesAccessor) { - const editorPane = accessor.get(IEditorService).activeEditorPane; + run(accessor: ServicesAccessor, args: unknown[]) { + const group = getEditorGroupFromArguments(accessor, args); + const editorPane = group?.activeEditorPane; if (editorPane instanceof KeybindingsEditor) { editorPane.search('@source:user'); } @@ -1207,11 +1215,12 @@ class PreferencesActionsContribution extends Disposable implements IWorkbenchCon for (const folder of this.workspaceContextService.getWorkspace().folders) { const commandId = `_workbench.openFolderSettings.${folder.uri.toString()}`; if (!CommandsRegistry.getCommand(commandId)) { - CommandsRegistry.registerCommand(commandId, () => { + CommandsRegistry.registerCommand(commandId, (accessor: ServicesAccessor, ...args: any[]) => { + const groupId = getEditorGroupFromArguments(accessor, args)?.id; if (this.workspaceContextService.getWorkbenchState() === WorkbenchState.FOLDER) { - return this.preferencesService.openWorkspaceSettings({ jsonEditor: false }); + return this.preferencesService.openWorkspaceSettings({ jsonEditor: false, groupId }); } else { - return this.preferencesService.openFolderSettings({ folderUri: folder.uri, jsonEditor: false }); + return this.preferencesService.openFolderSettings({ folderUri: folder.uri, jsonEditor: false, groupId }); } }); MenuRegistry.appendMenuItem(MenuId.EditorTitle, { @@ -1261,9 +1270,10 @@ class SettingsEditorTitleContribution extends Disposable implements IWorkbenchCo }] }); } - run(accessor: ServicesAccessor, args: IOpenSettingsActionOptions) { - args = sanitizeOpenSettingsArgs(args); - return accessor.get(IPreferencesService).openUserSettings({ jsonEditor: false, ...args }); + run(accessor: ServicesAccessor, ...args: unknown[]) { + const sanatizedArgs = sanitizeOpenSettingsArgs(args[0]); + const groupId = getEditorGroupFromArguments(accessor, args)?.id; + return accessor.get(IPreferencesService).openUserSettings({ jsonEditor: false, ...sanatizedArgs, groupId }); } }); }; @@ -1289,8 +1299,9 @@ class SettingsEditorTitleContribution extends Disposable implements IWorkbenchCo }] }); } - run(accessor: ServicesAccessor) { - const editorPane = accessor.get(IEditorService).activeEditorPane; + run(accessor: ServicesAccessor, ...args: unknown[]) { + const group = getEditorGroupFromArguments(accessor, args); + const editorPane = group?.activeEditorPane; if (editorPane instanceof SettingsEditor2) { return editorPane.switchToSettingsFile(); } @@ -1300,6 +1311,11 @@ class SettingsEditorTitleContribution extends Disposable implements IWorkbenchCo } } +function getEditorGroupFromArguments(accessor: ServicesAccessor, args: unknown[]): IEditorGroup | undefined { + const context = resolveCommandsContext(args, accessor.get(IEditorService), accessor.get(IEditorGroupsService), accessor.get(IListService)); + return context.groupedEditors[0]?.group; +} + const workbenchContributionsRegistry = Registry.as(WorkbenchExtensions.Workbench); registerWorkbenchContribution2(PreferencesActionsContribution.ID, PreferencesActionsContribution, WorkbenchPhase.BlockStartup); registerWorkbenchContribution2(PreferencesContribution.ID, PreferencesContribution, WorkbenchPhase.BlockStartup); diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts index c1d1c302db171..8e3388a7008e2 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts @@ -789,7 +789,7 @@ export class SettingsEditor2 extends EditorPane { private async openSettingsFile(options?: ISettingsEditorOptions): Promise { const currentSettingsTarget = this.settingsTargetsWidget.settingsTarget; - const openOptions: IOpenSettingsOptions = { jsonEditor: true, ...options }; + const openOptions: IOpenSettingsOptions = { jsonEditor: true, groupId: this.group.id, ...options }; if (currentSettingsTarget === ConfigurationTarget.USER_LOCAL) { if (options?.revealSetting) { const configurationProperties = Registry.as(Extensions.Configuration).getConfigurationProperties(); diff --git a/src/vs/workbench/services/preferences/browser/preferencesService.ts b/src/vs/workbench/services/preferences/browser/preferencesService.ts index 025cc02e964a4..f0bb2cb9f25b3 100644 --- a/src/vs/workbench/services/preferences/browser/preferencesService.ts +++ b/src/vs/workbench/services/preferences/browser/preferencesService.ts @@ -29,10 +29,10 @@ import { DEFAULT_EDITOR_ASSOCIATION, IEditorPane } from '../../../common/editor. import { EditorInput } from '../../../common/editor/editorInput.js'; import { SideBySideEditorInput } from '../../../common/editor/sideBySideEditorInput.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; -import { GroupDirection, IEditorGroupsService } from '../../editor/common/editorGroupsService.js'; -import { IEditorService, SIDE_GROUP, SIDE_GROUP_TYPE } from '../../editor/common/editorService.js'; +import { GroupDirection, IEditorGroup, IEditorGroupsService } from '../../editor/common/editorGroupsService.js'; +import { IEditorService, SIDE_GROUP } from '../../editor/common/editorService.js'; import { KeybindingsEditorInput } from './keybindingsEditorInput.js'; -import { DEFAULT_SETTINGS_EDITOR_SETTING, FOLDER_SETTINGS_PATH, IKeybindingsEditorOptions, IKeybindingsEditorPane, IOpenSettingsOptions, IPreferencesEditorModel, IPreferencesService, ISetting, ISettingsEditorOptions, ISettingsGroup, SETTINGS_AUTHORITY, USE_SPLIT_JSON_SETTING, validateSettingsEditorOptions } from '../common/preferences.js'; +import { DEFAULT_SETTINGS_EDITOR_SETTING, FOLDER_SETTINGS_PATH, IKeybindingsEditorPane, IOpenKeybindingsEditorOptions, IOpenSettingsOptions, IPreferencesEditorModel, IPreferencesService, ISetting, ISettingsEditorOptions, ISettingsGroup, SETTINGS_AUTHORITY, USE_SPLIT_JSON_SETTING, validateSettingsEditorOptions } from '../common/preferences.js'; import { SettingsEditor2Input } from '../common/preferencesEditorInput.js'; import { defaultKeybindingsContents, DefaultKeybindingsEditorModel, DefaultRawSettingsEditorModel, DefaultSettings, DefaultSettingsEditorModel, Settings2EditorModel, SettingsEditorModel, WorkspaceConfigurationEditorModel } from '../common/preferencesModels.js'; import { IRemoteAgentService } from '../../remote/common/remoteAgentService.js'; @@ -48,6 +48,7 @@ import { IURLService } from '../../../../platform/url/common/url.js'; import { compareIgnoreCase } from '../../../../base/common/strings.js'; import { IExtensionService } from '../../extensions/common/extensions.js'; import { IProgressService, ProgressLocation } from '../../../../platform/progress/common/progress.js'; +import { findGroup } from '../../editor/common/editorGroupFinder.js'; const emptyEditableSettingsContent = '{\n}'; @@ -248,8 +249,8 @@ export class PreferencesService extends Disposable implements IPreferencesServic ...options, focusSearch: true }; - await this.editorService.openEditor(input, validateSettingsEditorOptions(options), options.openToSide ? SIDE_GROUP : undefined); - return this.editorGroupService.activeGroup.activeEditorPane!; + const group = await this.getEditorGroupFromOptions(options); + return (await group.openEditor(input, validateSettingsEditorOptions(options)))!; } openApplicationSettings(options: IOpenSettingsOptions = {}): Promise { @@ -312,7 +313,7 @@ export class PreferencesService extends Disposable implements IPreferencesServic return this.open(folderSettingsUri, options); } - async openGlobalKeybindingSettings(textual: boolean, options?: IKeybindingsEditorOptions): Promise { + async openGlobalKeybindingSettings(textual: boolean, options?: IOpenKeybindingsEditorOptions): Promise { options = { pinned: true, revealIfOpened: true, ...options }; if (textual) { const emptyContents = '// ' + nls.localize('emptyKeybindingsHeader', "Place your key bindings in this file to override the defaults") + '\n[\n]'; @@ -322,18 +323,18 @@ export class PreferencesService extends Disposable implements IPreferencesServic // Create as needed and open in editor await this.createIfNotExists(editableKeybindings, emptyContents); if (openDefaultKeybindings) { - const activeEditorGroup = this.editorGroupService.activeGroup; - const sideEditorGroup = this.editorGroupService.addGroup(activeEditorGroup.id, GroupDirection.RIGHT); + const sourceGroupId = options.groupId ?? this.editorGroupService.activeGroup.id; + const sideEditorGroup = this.editorGroupService.addGroup(sourceGroupId, GroupDirection.RIGHT); await Promise.all([ - this.editorService.openEditor({ resource: this.defaultKeybindingsResource, options: { pinned: true, preserveFocus: true, revealIfOpened: true, override: DEFAULT_EDITOR_ASSOCIATION.id }, label: nls.localize('defaultKeybindings', "Default Keybindings"), description: '' }), + this.editorService.openEditor({ resource: this.defaultKeybindingsResource, options: { pinned: true, preserveFocus: true, revealIfOpened: true, override: DEFAULT_EDITOR_ASSOCIATION.id }, label: nls.localize('defaultKeybindings', "Default Keybindings"), description: '' }, sourceGroupId), this.editorService.openEditor({ resource: editableKeybindings, options }, sideEditorGroup.id) ]); } else { - await this.editorService.openEditor({ resource: editableKeybindings, options }); + await this.editorService.openEditor({ resource: editableKeybindings, options }, options.groupId); } } else { - const editor = (await this.editorService.openEditor(this.instantiationService.createInstance(KeybindingsEditorInput), { ...options })) as IKeybindingsEditorPane; + const editor = (await this.editorService.openEditor(this.instantiationService.createInstance(KeybindingsEditorInput), { ...options }, options.groupId)) as IKeybindingsEditorPane; if (options.query) { editor.search(options.query); } @@ -345,8 +346,16 @@ export class PreferencesService extends Disposable implements IPreferencesServic return this.editorService.openEditor({ resource: this.defaultKeybindingsResource, label: nls.localize('defaultKeybindings', "Default Keybindings") }); } + private async getEditorGroupFromOptions(options: IOpenSettingsOptions): Promise { + let group = options?.groupId !== undefined ? this.editorGroupService.getGroup(options.groupId) ?? this.editorGroupService.activeGroup : this.editorGroupService.activeGroup; + if (options.openToSide) { + group = (await this.instantiationService.invokeFunction(findGroup, {}, SIDE_GROUP))[0]; + } + return group; + } + private async openSettingsJson(resource: URI, options: IOpenSettingsOptions): Promise { - const group = options?.openToSide ? SIDE_GROUP : undefined; + const group = await this.getEditorGroupFromOptions(options); const editor = await this.doOpenSettingsJson(resource, options, group); if (editor && options?.revealSetting) { await this.revealSetting(options.revealSetting.key, !!options.revealSetting.edit, editor, resource); @@ -354,7 +363,7 @@ export class PreferencesService extends Disposable implements IPreferencesServic return editor; } - private async doOpenSettingsJson(resource: URI, options: ISettingsEditorOptions, group?: SIDE_GROUP_TYPE): Promise { + private async doOpenSettingsJson(resource: URI, options: ISettingsEditorOptions, group: IEditorGroup): Promise { const openSplitJSON = !!this.configurationService.getValue(USE_SPLIT_JSON_SETTING); const openDefaultSettings = !!this.configurationService.getValue(DEFAULT_SETTINGS_EDITOR_SETTING); if (openSplitJSON || openDefaultSettings) { @@ -364,15 +373,15 @@ export class PreferencesService extends Disposable implements IPreferencesServic const configurationTarget = options?.target ?? ConfigurationTarget.USER; const editableSettingsEditorInput = await this.getOrCreateEditableSettingsEditorInput(configurationTarget, resource); options = { ...options, pinned: true }; - return await this.editorService.openEditor(editableSettingsEditorInput, validateSettingsEditorOptions(options), group); + return await group.openEditor(editableSettingsEditorInput, { ...validateSettingsEditorOptions(options) }); } - private async doOpenSplitJSON(resource: URI, options: ISettingsEditorOptions = {}, group?: SIDE_GROUP_TYPE): Promise { + private async doOpenSplitJSON(resource: URI, options: ISettingsEditorOptions = {}, group: IEditorGroup,): Promise { const configurationTarget = options.target ?? ConfigurationTarget.USER; await this.createSettingsIfNotExists(configurationTarget, resource); const preferencesEditorInput = this.createSplitJsonEditorInput(configurationTarget, resource); options = { ...options, pinned: true }; - return this.editorService.openEditor(preferencesEditorInput, validateSettingsEditorOptions(options), group); + return group.openEditor(preferencesEditorInput, validateSettingsEditorOptions(options)); } public createSplitJsonEditorInput(configurationTarget: ConfigurationTarget, resource: URI): EditorInput { diff --git a/src/vs/workbench/services/preferences/common/preferences.ts b/src/vs/workbench/services/preferences/common/preferences.ts index e4ae6416bbbb6..ea38554325257 100644 --- a/src/vs/workbench/services/preferences/common/preferences.ts +++ b/src/vs/workbench/services/preferences/common/preferences.ts @@ -211,6 +211,7 @@ export interface ISettingsEditorOptions extends IEditorOptions { export interface IOpenSettingsOptions extends ISettingsEditorOptions { jsonEditor?: boolean; openToSide?: boolean; + groupId?: number; } export function validateSettingsEditorOptions(options: ISettingsEditorOptions): ISettingsEditorOptions { @@ -231,6 +232,10 @@ export interface IKeybindingsEditorOptions extends IEditorOptions { query?: string; } +export interface IOpenKeybindingsEditorOptions extends IKeybindingsEditorOptions { + groupId?: number; +} + export const IPreferencesService = createDecorator('preferencesService'); export interface IPreferencesService { @@ -254,7 +259,7 @@ export interface IPreferencesService { openRemoteSettings(options?: IOpenSettingsOptions): Promise; openWorkspaceSettings(options?: IOpenSettingsOptions): Promise; openFolderSettings(options: IOpenSettingsOptions & { folderUri: IOpenSettingsOptions['folderUri'] }): Promise; - openGlobalKeybindingSettings(textual: boolean, options?: IKeybindingsEditorOptions): Promise; + openGlobalKeybindingSettings(textual: boolean, options?: IOpenKeybindingsEditorOptions): Promise; openDefaultKeybindingsFile(): Promise; openLanguageSpecificSettings(languageId: string, options?: IOpenSettingsOptions): Promise; getEditableSettingsURI(configurationTarget: ConfigurationTarget, resource?: URI): Promise; diff --git a/src/vs/workbench/services/preferences/test/browser/preferencesService.test.ts b/src/vs/workbench/services/preferences/test/browser/preferencesService.test.ts index 4f1f2493cb4f7..77c36590b8a36 100644 --- a/src/vs/workbench/services/preferences/test/browser/preferencesService.test.ts +++ b/src/vs/workbench/services/preferences/test/browser/preferencesService.test.ts @@ -10,26 +10,37 @@ import { ICommandService } from '../../../../../platform/commands/common/command import { SyncDescriptor } from '../../../../../platform/instantiation/common/descriptors.js'; import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { IURLService } from '../../../../../platform/url/common/url.js'; -import { DEFAULT_EDITOR_ASSOCIATION } from '../../../../common/editor.js'; +import { DEFAULT_EDITOR_ASSOCIATION, IEditorPane } from '../../../../common/editor.js'; import { IJSONEditingService } from '../../../configuration/common/jsonEditing.js'; import { TestJSONEditingService } from '../../../configuration/test/common/testServices.js'; import { PreferencesService } from '../../browser/preferencesService.js'; import { IPreferencesService, ISettingsEditorOptions } from '../../common/preferences.js'; import { IRemoteAgentService } from '../../../remote/common/remoteAgentService.js'; -import { TestRemoteAgentService, ITestInstantiationService, TestEditorService, workbenchInstantiationService } from '../../../../test/browser/workbenchTestServices.js'; +import { TestRemoteAgentService, ITestInstantiationService, workbenchInstantiationService, TestEditorGroupView, TestEditorGroupsService } from '../../../../test/browser/workbenchTestServices.js'; +import { IEditorGroupsService } from '../../../editor/common/editorGroupsService.js'; +import { IEditorOptions } from '../../../../../platform/editor/common/editor.js'; +import { SettingsEditor2Input } from '../../common/preferencesEditorInput.js'; suite('PreferencesService', () => { let testInstantiationService: ITestInstantiationService; let testObject: PreferencesService; - let editorService: TestEditorService2; + let lastOpenEditorOptions: IEditorOptions | undefined; const disposables = ensureNoDisposablesAreLeakedInTestSuite(); setup(() => { - editorService = disposables.add(new TestEditorService2()); - testInstantiationService = workbenchInstantiationService({ - editorService: () => editorService - }, disposables); + testInstantiationService = workbenchInstantiationService({}, disposables); + class TestOpenEditorGroupView extends TestEditorGroupView { + lastOpenEditorOptions: any; + override openEditor(_editor: SettingsEditor2Input, options?: IEditorOptions): Promise { + lastOpenEditorOptions = options; + _editor.dispose(); + return Promise.resolve(undefined!); + } + } + + const testEditorGroupService = new TestEditorGroupsService([new TestOpenEditorGroupView(0)]); + testInstantiationService.stub(IEditorGroupsService, testEditorGroupService); testInstantiationService.stub(IJSONEditingService, TestJSONEditingService); testInstantiationService.stub(IRemoteAgentService, TestRemoteAgentService); testInstantiationService.stub(ICommandService, TestCommandService); @@ -42,19 +53,10 @@ suite('PreferencesService', () => { testObject = disposables.add(instantiationService.createInstance(PreferencesService)); }); test('options are preserved when calling openEditor', async () => { - testObject.openSettings({ jsonEditor: false, query: 'test query' }); - const options = editorService.lastOpenEditorOptions as ISettingsEditorOptions; + await testObject.openSettings({ jsonEditor: false, query: 'test query' }); + const options = lastOpenEditorOptions as ISettingsEditorOptions; assert.strictEqual(options.focusSearch, true); assert.strictEqual(options.override, DEFAULT_EDITOR_ASSOCIATION.id); assert.strictEqual(options.query, 'test query'); }); }); - -class TestEditorService2 extends TestEditorService { - lastOpenEditorOptions: any; - - override async openEditor(editorInput: any, options?: any): Promise { - this.lastOpenEditorOptions = options; - return super.openEditor(editorInput, options); - } -}