Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made deleteCells public so we could use it for the delete action and renamed moveCellUp/moveCellDown to be plural since it supports multi-select.

Original file line number Diff line number Diff line change
Expand Up @@ -219,25 +219,28 @@ export interface IPositronNotebookInstance extends IPositronNotebookEditor {
/**
* Removes a cell from the notebook.
*
* @param cell Optional cell to delete. If not provided, deletes the currently selected cell
* @param cell Optional cell to delete. If not provided, deletes the currently active cell
*/
deleteCell(cell?: IPositronNotebookCell): void;

/**
* Moves a cell up by one position.
* Supports multi-cell selection - moves all selected cells as a group.
* Removes multiple cells from the notebook.
*
* @param cell The cell to move up
* @param cells Optional array of cells to delete. If not provided, uses current selection.
*/
moveCellUp(cell: IPositronNotebookCell): void;
deleteCells(cells?: IPositronNotebookCell[]): void;

/**
* Moves a cell down by one position.
* Moves the selected cell(s) up by one position.
* Supports multi-cell selection - moves all selected cells as a group.
*/
moveCellsUp(): void;

/**
* Moves the selected cell(s) down by one position.
* Supports multi-cell selection - moves all selected cells as a group.
*
* @param cell The cell to move down
*/
moveCellDown(cell: IPositronNotebookCell): void;
moveCellsDown(): void;

/**
* Moves cells to a specific index.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@
}
}

/**
* Override the global keyoard focus outline style for notebook containers.
* This prevents focus outline from showing when a user clicks on the notebook.
* This prevents empty notebooks from showing an outline when they are focused.
*/
.monaco-workbench .positron-notebook-container[tabindex="-1"]:focus {
outline: none;
}

.positron-notebook-cells-container {
padding-block: 1.5rem;
padding-inline: 1rem;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ export class PositronNotebookEditor extends AbstractEditorWithViewState<INoteboo

this._logService.debug(this._identifier, 'createEditor');
this._parentDiv = DOM.$('.positron-notebook-container');
// Make container focusable so it can maintain focus when empty
this._parentDiv.tabIndex = -1;
parent.appendChild(this._parentDiv);
this._parentDiv.style.display = 'relative';
}
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file changes the "paste" and "move cell" functions to use the active/selected cell for the operation.

The delete cell function has been updated to use the active/selected cells for the operation if a set of cells aren't provided.

cc @nstrayer will these changes effect the assistant tool calls you added? Do I need to rewrite some of these to support an optional array of cells?

Original file line number Diff line number Diff line change
Expand Up @@ -836,12 +836,13 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
this.deleteCells([cell]);
}


/**
* Deletes multiple cells from the notebook.
* @param cellsToDelete Array of cells to delete
* @param cells Array of cells to delete
*/
deleteCells(cellsToDelete: IPositronNotebookCell[]): void {
deleteCells(cells?: IPositronNotebookCell[]): void {
const cellsToDelete = cells || getSelectedCells(this.selectionStateMachine.state.get());

this._assertTextModel();

if (cellsToDelete.length === 0) {
Expand Down Expand Up @@ -905,18 +906,17 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
}

/**
* Moves a cell up by one position.
* Moves the selected cell(s) up by one position.
* Supports multi-cell selection - moves all selected cells as a group.
* @param cell The cell to move up
*/
moveCellUp(cell: IPositronNotebookCell): void {
moveCellsUp(): void {
this._assertTextModel();

if (cell.index <= 0) {
const cellsToMove = getSelectedCells(this.selectionStateMachine.state.get());
if (cellsToMove.length === 0) {
return;
}

const cellsToMove = getSelectedCells(this.selectionStateMachine.state.get());
const firstIndex = Math.min(...cellsToMove.map(c => c.index));
const lastIndex = Math.max(...cellsToMove.map(c => c.index));
const length = lastIndex - firstIndex + 1;
Expand Down Expand Up @@ -956,19 +956,18 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
}

/**
* Moves a cell down by one position.
* Moves the selected cell(s) down by one position.
* Supports multi-cell selection - moves all selected cells as a group.
* @param cell The cell to move down
*/
moveCellDown(cell: IPositronNotebookCell): void {
moveCellsDown(): void {
this._assertTextModel();

const cells = this.cells.get();
if (cell.index >= cells.length - 1) {
const cellsToMove = getSelectedCells(this.selectionStateMachine.state.get());
if (cellsToMove.length === 0) {
return;
}

const cellsToMove = getSelectedCells(this.selectionStateMachine.state.get());
const cells = this.cells.get();
const firstIndex = Math.min(...cellsToMove.map(c => c.index));
const lastIndex = Math.max(...cellsToMove.map(c => c.index));
const length = lastIndex - firstIndex + 1;
Expand Down Expand Up @@ -1210,6 +1209,10 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
this._assertTextModel();
const modelCells = this.textModel.cells;

// Track if we're transitioning from empty to non-empty or vice versa
const wasEmpty = this.cells.get().length === 0;
const willBeEmpty = modelCells.length === 0;

const cellModelToCellMap = new Map(
this.cells.get().map(cell => [cell.model, cell])
);
Expand Down Expand Up @@ -1251,6 +1254,12 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
cellModelToCellMap.forEach(cell => cell.dispose());

this.cells.set(cells, undefined);

// Handle focus management for empty/non-empty transitions
if (!wasEmpty && willBeEmpty && this._container) {
// Transitioned to empty: focus the container
this._container.focus();
}
}

/**
Expand Down Expand Up @@ -1460,7 +1469,7 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot

/**
* Pastes cells from the clipboard at the specified index.
* @param index The position to paste cells at. If not provided, pastes after the last selected cell
* @param index The position to paste cells at. If not provided, pastes after active cell or at end of notebook.
*/
pasteCells(index?: number): void {
if (!this.canPaste()) {
Expand Down Expand Up @@ -1517,13 +1526,12 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
}

/**
* Pastes cells from the clipboard above the first selected cell.
* Pastes cells from the clipboard above the active cell.
*/
pasteCellsAbove(): void {
const selection = getSelectedCells(this.selectionStateMachine.state.get());
if (selection.length > 0) {
const firstSelectedIndex = selection[0].index;
this.pasteCells(firstSelectedIndex);
const activeCell = getActiveCell(this.selectionStateMachine.state.get());
if (activeCell) {
this.pasteCells(activeCell.index);
} else {
this.pasteCells(0);
}
Expand Down Expand Up @@ -1564,10 +1572,10 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot

// Helper method to get insertion index
private getInsertionIndex(): number {
const selections = getSelectedCells(this.selectionStateMachine.state.get());
if (selections.length > 0) {
const lastSelectedIndex = selections[selections.length - 1].index;
return lastSelectedIndex + 1;
// Use the active cell position for determining insertion, or at the end if no active cell
const activeCell = getActiveCell(this.selectionStateMachine.state.get());
if (activeCell) {
return activeCell.index + 1;
}
return this.cells.get().length;
}
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change in this file allows undo/redo to work when a notebook file has no cells

Original file line number Diff line number Diff line change
Expand Up @@ -37,25 +37,29 @@ class PositronNotebookUndoRedoContribution extends Disposable {
return false;
}

// Determine if the notebook is empty. This is important because when there are no cells,
// neither the container or cell editor will have focus, but we still want undo/redo to work.
// This enables undoing cell operations (cut and delete) that result in an empty notebook.
const emptyNotebook = instance.cells.get().length === 0;

// Use the notebook-specific scoped context key service instead of the global one
const scopedContextKeyService = instance.contextManager.getScopedContextKeyService();
if (!scopedContextKeyService) {
// Fallback to global context service if scoped service is not available
// This shouldn't happen in normal operation, but provides a safety net
const containerFocused = this.contextKeyService.getContextKeyValue<boolean>(POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED.key) ?? false;
const cellEditorFocused = this.contextKeyService.getContextKeyValue<boolean>(POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED.key) ?? false;
// Handle undo/redo if either the container is focused OR a cell editor is focused
// This matches VS Code's behavior where notebook undo works regardless of specific focus location
return containerFocused || cellEditorFocused;
// Handle undo/redo if the container is focused OR a cell editor is focused OR the notebook is empty
return containerFocused || cellEditorFocused || emptyNotebook;
}

// Read context keys from the scoped context service that actually has these keys bound
const containerFocused = scopedContextKeyService.getContextKeyValue<boolean>(POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED.key) ?? false;
const cellEditorFocused = scopedContextKeyService.getContextKeyValue<boolean>(POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED.key) ?? false;

// Handle undo/redo if either the container is focused OR a cell editor is focused
// Handle undo/redo if the container is focused OR a cell editor is focused OR the notebook is empty
// This allows undo to work even when typing in a cell (common after adding a new cell)
return containerFocused || cellEditorFocused;
return containerFocused || cellEditorFocused || emptyNotebook;
}

private handleUndo(): boolean | Promise<void> {
Expand Down
Loading
Loading