Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { IWorkbenchUIElementFactory } from './workbenchUIElementFactory.js';
export class MultiDiffEditorWidget extends Disposable {
private readonly _dimension = observableValue<Dimension | undefined>(this, undefined);
private readonly _viewModel = observableValue<MultiDiffEditorViewModel | undefined>(this, undefined);
private readonly _renderSideBySide = observableValue<boolean | undefined>(this, undefined);
private readonly _diffLayoutOptions = observableValue<IDiffEditorOptions | undefined>(this, undefined);

private readonly _widgetImpl = derived(this, (reader) => {
readHotReloadableExport(DiffEditorItemTemplate, reader);
Expand All @@ -36,7 +36,7 @@ export class MultiDiffEditorWidget extends Disposable {
this._dimension,
this._viewModel,
this._workbenchUIElementFactory,
this._renderSideBySide,
this._diffLayoutOptions,
this._diffEditorOptions,
));
});
Expand Down Expand Up @@ -98,14 +98,18 @@ export class MultiDiffEditorWidget extends Disposable {
/**
* Overrides whether the embedded diffs render side by side (`true`) or inline
* (`false`) as editor-local state, independent of the
* `diffEditor.renderSideBySide` setting. When left unset the setting applies.
* `diffEditor.renderSideBySide` setting. Responsive inline fallback is disabled
* unless explicitly enabled.
*/
public setRenderSideBySide(renderSideBySide: boolean): void {
this._renderSideBySide.set(renderSideBySide, undefined);
public setRenderSideBySide(renderSideBySide: boolean, options?: { readonly useInlineViewWhenSpaceIsLimited?: boolean }): void {
this._diffLayoutOptions.set({
renderSideBySide,
useInlineViewWhenSpaceIsLimited: options?.useInlineViewWhenSpaceIsLimited ?? false,
}, undefined);
}

public toggleRenderSideBySide(): void {
this._renderSideBySide.set(!(this._renderSideBySide.get() ?? true), undefined);
this.setRenderSideBySide(!(this._diffLayoutOptions.get()?.renderSideBySide ?? true));
}

private readonly _activeControl = derived(this, (reader) => this._widgetImpl.read(reader).activeControl.read(reader));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export class MultiDiffEditorWidgetImpl extends Disposable {
private readonly _dimension: IObservable<Dimension | undefined>,
private readonly _viewModel: IObservable<MultiDiffEditorViewModel | undefined>,
private readonly _workbenchUIElementFactory: IWorkbenchUIElementFactory,
private readonly _renderSideBySide: IObservable<boolean | undefined>,
private readonly _diffLayoutOptions: IObservable<IDiffEditorOptions | undefined>,
private readonly _diffEditorOptions: IDiffEditorOptions | undefined,
@IContextKeyService private readonly _parentContextKeyService: IContextKeyService,
@IInstantiationService private readonly _parentInstantiationService: IInstantiationService,
Expand Down Expand Up @@ -108,11 +108,7 @@ export class MultiDiffEditorWidgetImpl extends Disposable {
]);
this._sizeObserver = this._register(new ObservableElementSizeObserver(this._element, undefined));
this._optionsOverride = derived(this, reader => {
const renderSideBySide = this._renderSideBySide.read(reader);
// Also pin `useInlineViewWhenSpaceIsLimited` off so the toggle deterministically
// controls inline vs. side-by-side regardless of the available width.
const options: IDiffEditorOptions = renderSideBySide === undefined ? {} : { renderSideBySide, useInlineViewWhenSpaceIsLimited: false };
return { ...this._diffEditorOptions, ...options };
return { ...this._diffEditorOptions, ...this._diffLayoutOptions.read(reader) };
});
this._objectPool = this._register(new ObjectPool<TemplateData, DiffEditorItemTemplate>((data) => {
const template = this._instantiationService.createInstance(
Expand Down Expand Up @@ -191,7 +187,7 @@ export class MultiDiffEditorWidgetImpl extends Disposable {

const ctxRenderSideBySide = this._parentContextKeyService.createKey<boolean>(EditorContextKeys.multiDiffEditorRenderSideBySide.key, true);
this._register(autorun((reader) => {
const renderSideBySide = this._renderSideBySide.read(reader);
const renderSideBySide = this._diffLayoutOptions.read(reader)?.renderSideBySide;
if (renderSideBySide !== undefined) {
ctxRenderSideBySide.set(renderSideBySide);
}
Expand Down
16 changes: 14 additions & 2 deletions src/vs/editor/test/browser/widget/multiDiffEditorWidget.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ suite('MultiDiffEditorWidget', () => {
sinon.restore();
});

test('applies document options before attaching the diff model', async () => {
test('applies document and responsive layout options before attaching the diff model', async () => {
const services = new ServiceCollection();
services.set(IAccessibilitySignalService, new class extends mock<IAccessibilitySignalService>() { }());
services.set(IActionViewItemService, new NullActionViewItemService());
Expand Down Expand Up @@ -78,19 +78,31 @@ suite('MultiDiffEditorWidget', () => {
{} satisfies IWorkbenchUIElementFactory,
undefined,
);
widget.setRenderSideBySide(true, { useInlineViewWhenSpaceIsLimited: true });
widget.layout(new Dimension(800, 600));
const viewModel = widget.createViewModel(model);
await waitForState(viewModel.items, items => items.length === 1);
widget.setViewModel(viewModel);
widget.reveal({ original: originalUri, modified: modifiedUri }, { highlight: false });

try {
const activeControl = widget.getActiveControl();
const renderSideBySideWhenNarrow = activeControl?.renderSideBySide;
widget.layout(new Dimension(1000, 600));
assert.deepStrictEqual({
configuredAccessibilitySupport: updateOptionsSpy.firstCall.args[0].accessibilitySupport,
configuredRenderSideBySide: updateOptionsSpy.firstCall.args[0].renderSideBySide,
configuredUseInlineViewWhenSpaceIsLimited: updateOptionsSpy.firstCall.args[0].useInlineViewWhenSpaceIsLimited,
renderSideBySideWhenNarrow,
renderSideBySideWhenWide: activeControl?.renderSideBySide,
optionsAppliedBeforeModel: updateOptionsSpy.calledBefore(setDiffModelSpy),
effectiveAccessibilitySupport: widget.getActiveControl()?.getModifiedEditor().getOption(EditorOption.accessibilitySupport),
effectiveAccessibilitySupport: activeControl?.getModifiedEditor().getOption(EditorOption.accessibilitySupport),
}, {
configuredAccessibilitySupport: 'off',
configuredRenderSideBySide: true,
configuredUseInlineViewWhenSpaceIsLimited: true,
renderSideBySideWhenNarrow: false,
renderSideBySideWhenWide: true,
optionsAppliedBeforeModel: true,
effectiveAccessibilitySupport: AccessibilitySupport.Disabled,
});
Expand Down
21 changes: 11 additions & 10 deletions src/vs/sessions/contrib/changes/browser/changesViewActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { Disposable } from '../../../../base/common/lifecycle.js';
import { observableFromEvent } from '../../../../base/common/observable.js';
import { isEqual } from '../../../../base/common/resources.js';
import { URI } from '../../../../base/common/uri.js';
import { EditorContextKeys } from '../../../../editor/common/editorContextKeys.js';
import { localize, localize2 } from '../../../../nls.js';
import { Action2, IAction2Options, MenuId, MenuRegistry, registerAction2 } from '../../../../platform/actions/common/actions.js';
import { ICommandService } from '../../../../platform/commands/common/commands.js';
Expand All @@ -29,6 +28,7 @@ import { ISessionsService } from '../../../services/sessions/browser/sessionsSer
import { OPEN_PULL_REQUEST_ACTION_ID } from '../../github/common/types.js';
import { ActiveSessionContextKeys, CHANGES_VIEW_ID, ChangesContextKeys, ChangesViewMode, SESSIONS_CHANGES_OPEN_SINGLE_FILE_DIFF_SETTING } from '../common/changes.js';
import { IChangesViewService } from '../common/changesViewService.js';
import { SessionsDiffRenderSideBySideContext } from '../../editor/common/diffEditorOptionsService.js';
import { CHANGES_HEADER_ACTIONS_ID } from './changesView.js';
import { SessionChangesEditor } from './sessionChangesEditor.js';

Expand Down Expand Up @@ -325,22 +325,23 @@ registerAction2(ExpandAllSessionChangesDiffsAction);

// The Agents window reuses the workbench `toggle.diff.renderSideBySide` command so a
// user's keybinding for it carries over here (issue #324765). The sessions override of
// IDiffEditorCommandsService flips the workspace `diffEditor.renderSideBySide` setting,
// which the Changes editor observes.
// IDiffEditorCommandsService updates the Changes editor's own preferred layout.

// Primary header button with state-specific titles: "Show Side by Side Diff" when
// currently inline, and (checked) "Show Inline Diff" when currently side by side.
// The action changes the preferred layout. Side by side still falls back to inline
// when the editor is narrow, so the label must not promise an immediate layout.
MenuRegistry.appendMenuItem(Menus.SessionsEditorHeaderSecondary, {
command: {
id: TOGGLE_DIFF_SIDE_BY_SIDE,
title: localize('showSideBySideDiff', "Show Side by Side Diff"),
title: localize('preferSideBySideDiff', "Prefer Side by Side Diff"),
tooltip: localize('preferSideBySideDiff.tooltip', "Uses inline layout when space is limited unless screen reader optimized mode is enabled."),
icon: Codicon.diffSidebyside,
toggled: {
condition: ContextKeyExpr.or(
ContextKeyExpr.and(singlePaneChangesEditorActive, EditorContextKeys.multiDiffEditorRenderSideBySide),
ContextKeyExpr.and(singlePaneFileDiffEditorActive, EditorContextKeys.diffEditorInlineMode.negate())
ContextKeyExpr.and(singlePaneChangesEditorActive, SessionsDiffRenderSideBySideContext),
ContextKeyExpr.and(singlePaneFileDiffEditorActive, SessionsDiffRenderSideBySideContext)
)!,
title: localize('showInlineDiff', "Show Inline Diff"),
title: localize('preferInlineDiff', "Prefer Inline Diff"),
tooltip: localize('preferInlineDiff.tooltip', "Always uses inline layout."),
},
},
group: '1_diff',
Expand All @@ -352,7 +353,7 @@ MenuRegistry.appendMenuItem(Menus.SessionsEditorHeaderSecondary, {
MenuRegistry.appendMenuItem(MenuId.CommandPalette, {
command: {
id: TOGGLE_DIFF_SIDE_BY_SIDE,
title: localize2('toggleDiffView', "Toggle Diff View"),
title: localize2('togglePreferredDiffView', "Toggle Preferred Diff View"),
category: localize2('changes', "Changes"),
},
when: singlePaneDiffEditorTitleVisible
Expand Down
15 changes: 4 additions & 11 deletions src/vs/sessions/contrib/changes/browser/sessionChangesEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { URI } from '../../../../base/common/uri.js';
import { IDiffEditor } from '../../../../editor/common/editorCommon.js';
import { ICodeEditor } from '../../../../editor/browser/editorBrowser.js';
import { ICommandService } from '../../../../platform/commands/common/commands.js';
import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js';
import { IContextKeyService } from '../../../../platform/contextkey/common/contextkey.js';
import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
import { ServiceCollection } from '../../../../platform/instantiation/common/serviceCollection.js';
Expand Down Expand Up @@ -52,6 +51,7 @@ import { CheckboxActionViewItem } from '../../../../base/browser/ui/toggle/toggl
import { defaultCheckboxStyles } from '../../../../platform/theme/browser/defaultStyles.js';
import { localize } from '../../../../nls.js';
import { getChangesEditorFileStats } from './changesEditorLabels.js';
import { IDiffEditorOptionsService } from '../../editor/common/diffEditorOptionsService.js';

const HEADER_HEIGHT = 35;

Expand Down Expand Up @@ -199,9 +199,9 @@ export class SessionChangesEditor extends AbstractEditorWithViewState<IMultiDiff
@IEditorGroupsService editorGroupService: IEditorGroupsService,
@IContextKeyService private readonly contextKeyService: IContextKeyService,
@IChangesViewService private readonly changesViewService: IChangesViewService,
@IConfigurationService private readonly configurationService: IConfigurationService,
@IAgentWorkbenchLayoutService private readonly layoutService: IAgentWorkbenchLayoutService,
@ISessionChangesService private readonly sessionChangesService: ISessionChangesService,
@IDiffEditorOptionsService private readonly diffEditorOptionsService: IDiffEditorOptionsService,
) {
super(
SessionChangesEditor.ID,
Expand Down Expand Up @@ -254,18 +254,11 @@ export class SessionChangesEditor extends AbstractEditorWithViewState<IMultiDiff
paneInstantiationService.createInstance(SessionChangesUIElementFactory, this._scopedChangesObs),
CHANGES_DIFF_EDITOR_OPTIONS,
));
this._applyRenderSideBySide();
this._register(this.configurationService.onDidChangeConfiguration(e => {
if (e.affectsConfiguration('diffEditor.renderSideBySide')) {
this._applyRenderSideBySide();
}
this._register(autorun(reader => {
this.widget?.setRenderSideBySide(this.diffEditorOptionsService.renderSideBySide.read(reader), { useInlineViewWhenSpaceIsLimited: true });
}));
}

private _applyRenderSideBySide(): void {
this.widget?.setRenderSideBySide(this.configurationService.getValue<boolean>('diffEditor.renderSideBySide') ?? true);
}

/**
* Resolves the diff editor and code editor showing the given file, mirroring
* {@link MultiDiffEditor.tryGetCodeEditor} so file-toolbar actions can operate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export class SessionsChangesAccessibilityHelp implements IAccessibleViewImplemen
content.push(localize('sessionsChanges.checks', "The Checks section lists the continuous integration checks for the session's pull request. Its header is a button: press Enter or Space to collapse or expand it{0}.", '<keybinding:sessions.action.revealCIChecks>'));
content.push(localize('sessionsChanges.viewMode', "The Changes view can show files as a tree or a flat list. Use the view's toolbar actions to switch between Tree and List modes."));
content.push(localize('sessionsChanges.operations', "When available, the toolbar also provides actions to commit, merge, sync, or create a pull request. Use Tab and Shift+Tab to move between the file list and toolbar actions."));
content.push(localize('sessionsChanges.diffView', "File diffs can be shown side by side or inline. Use the Toggle Diff View command to switch between them{0}.", '<keybinding:toggle.diff.renderSideBySide>'));
content.push(localize('sessionsChanges.diffView', "File diffs can prefer side-by-side or inline layout. Unless screen reader optimized mode is enabled, side-by-side diffs automatically use inline layout when space is limited. Use the Toggle Preferred Diff View command to switch the preference{0}.", '<keybinding:toggle.diff.renderSideBySide>'));

return new AccessibleContentProvider(
AccessibleViewProviderId.SessionsChanges,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { Context } from '../../../../../platform/contextkey/browser/contextKeySe
import { ContextKeyExpression } from '../../../../../platform/contextkey/common/contextkey.js';
import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
import { EditorContextKeys } from '../../../../../editor/common/editorContextKeys.js';
import { SessionsDiffRenderSideBySideContext } from '../../../editor/common/diffEditorOptionsService.js';
import { ActiveEditorContext, AuxiliaryBarVisibleContext, IsAuxiliaryWindowContext, IsSessionsWindowContext, IsTopRightEditorGroupContext, MainEditorAreaVisibleContext, TextCompareEditorActiveContext } from '../../../../../workbench/common/contextkeys.js';
import { Menus } from '../../../../browser/menus.js';
import { ISessionsService } from '../../../../services/sessions/browser/sessionsService.js';
Expand Down Expand Up @@ -155,7 +156,7 @@ suite('Changes View Actions', () => {
});
});

test('toggle inline view is contributed to multi-file and single-file diff editor headers with toggle state', () => {
test('preferred diff view is contributed to multi-file and single-file diff editor headers with toggle state', () => {
const item = MenuRegistry.getMenuItems(Menus.SessionsEditorHeaderSecondary)
.filter(isIMenuItem)
.find(item => item.command.id === 'toggle.diff.renderSideBySide');
Expand All @@ -177,9 +178,10 @@ suite('Changes View Actions', () => {
group: item.group,
order: item.order,
icon: ThemeIcon.isThemeIcon(item.command.icon) ? item.command.icon.id : undefined,
tooltip: typeof item.command.tooltip === 'string' ? item.command.tooltip : item.command.tooltip?.value,
toggledTitle: toggledInfo?.title,
toggledOnMultiDiffSideBySide: toggledInfo?.condition.serialize().includes(EditorContextKeys.multiDiffEditorRenderSideBySide.key),
toggledOnSingleDiffSideBySide: toggledInfo?.condition.serialize().includes(EditorContextKeys.diffEditorInlineMode.key),
toggledTooltip: toggledInfo?.tooltip,
toggledOnSharedPreference: toggledInfo?.condition.serialize().includes(SessionsDiffRenderSideBySideContext.key),
hasSessionsWindowGate: when.includes(IsSessionsWindowContext.key),
hasActiveEditorGate: when.includes(ActiveEditorContext.key) && when.includes(SessionChangesEditor.ID),
hasTextCompareEditorGate: when.includes(TextCompareEditorActiveContext.key),
Expand All @@ -188,13 +190,14 @@ suite('Changes View Actions', () => {
matchesNonTextDiffContext: item.when?.evaluate(nonTextDiffContext) ?? false,
}, {
id: 'toggle.diff.renderSideBySide',
title: 'Show Side by Side Diff',
title: 'Prefer Side by Side Diff',
group: '1_diff',
order: 20,
icon: Codicon.diffSidebyside.id,
toggledTitle: 'Show Inline Diff',
toggledOnMultiDiffSideBySide: true,
toggledOnSingleDiffSideBySide: true,
tooltip: 'Uses inline layout when space is limited unless screen reader optimized mode is enabled.',
toggledTitle: 'Prefer Inline Diff',
toggledTooltip: 'Always uses inline layout.',
toggledOnSharedPreference: true,
hasSessionsWindowGate: true,
hasActiveEditorGate: true,
hasTextCompareEditorGate: true,
Expand All @@ -204,7 +207,7 @@ suite('Changes View Actions', () => {
});
});

test('toggle inline view is contributed to the command palette (Changes category)', () => {
test('preferred diff view is contributed to the command palette (Changes category)', () => {
const item = MenuRegistry.getMenuItems(MenuId.CommandPalette)
.filter(isIMenuItem)
.find(item => item.command.id === 'toggle.diff.renderSideBySide' && item.command.category !== undefined && (typeof item.command.category === 'string' ? item.command.category : item.command.category.value) === 'Changes');
Expand All @@ -222,7 +225,7 @@ suite('Changes View Actions', () => {
hasEditorAreaVisibleGate: when.includes(MainEditorAreaVisibleContext.key),
}, {
id: 'toggle.diff.renderSideBySide',
title: 'Toggle Diff View',
title: 'Toggle Preferred Diff View',
category: 'Changes',
hasSessionsWindowGate: true,
hasActiveEditorGate: true,
Expand Down
Loading
Loading