From de7c95a4354faf58b6aac76114e34e08853eb128 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 18 Sep 2024 16:38:47 -0300 Subject: [PATCH 01/17] Implement the logic to go to last focused editor --- assets/js/hooks/cell.js | 9 ++ assets/js/hooks/cell_editor/live_editor.js | 12 +++ assets/js/hooks/session.js | 29 ++++++ assets/js/lib/history.js | 103 +++++++++++++++++++++ 4 files changed, 153 insertions(+) create mode 100644 assets/js/lib/history.js diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index 5f2a440f4e32..0faef2cbea22 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -211,6 +211,15 @@ const Cell = { // gives it focus if (!this.isFocused || !this.insertMode) { this.currentEditor().blur(); + } else if (this.insertMode) { + const lineNumber = this.currentEditor().getLineNumberAtCursor(); + + if (lineNumber !== null) + globalPubsub.broadcast("history", { + type: "navigation", + cellId: this.props.cellId, + line: lineNumber.toString(), + }); } }, 0); }); diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 12230755f2ab..dd1050d22aee 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -166,6 +166,18 @@ export default class LiveEditor { return node.parentElement; } + /** + * Returns line number from the current main cursor position. + */ + getLineNumberAtCursor() { + if (!this.isMounted()) { + return null; + } + + const pos = this.view.state.selection.main.head; + return this.view.state.doc.lineAt(pos).number; + } + /** * Focuses the editor. * diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index 6a70db304af4..5a9247608dcd 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -18,6 +18,7 @@ import { leaveChannel } from "./js_view/channel"; import { isDirectlyEditable, isEvaluable } from "../lib/notebook"; import { settingsStore } from "../lib/settings"; import { LiveStore } from "../lib/live_store"; +import History from "../lib/history"; /** * A hook managing the whole session. @@ -81,6 +82,7 @@ const Session = { this.viewOptions = null; this.keyBuffer = new KeyBuffer(); this.lastLocationReportByClientId = {}; + this.history = new History(); this.followedClientId = null; this.store = LiveStore.create("session"); @@ -161,6 +163,7 @@ const Session = { globalPubsub.subscribe("jump_to_editor", ({ line, file }) => this.jumpToLine(file, line), ), + globalPubsub.subscribe("history", this.handleHistoryEvent.bind(this)), ]; this.initializeDragAndDrop(); @@ -278,6 +281,7 @@ const Session = { this.subscriptions.forEach((subscription) => subscription.destroy()); this.store.destroy(); + this.history.destroy(); }, getProps() { @@ -1227,6 +1231,8 @@ const Session = { }, handleCellDeleted(cellId, siblingCellId) { + this.history.removeAllFromCell(cellId); + if (this.focusedId === cellId) { if (this.view) { const visibleSiblingId = this.ensureVisibleFocusableEl(siblingCellId); @@ -1324,6 +1330,14 @@ const Session = { } }, + handleHistoryEvent(payload) { + if (payload.type === "back") { + this.goBackNavigationHistory(); + } else if (payload.type === "navigation") { + this.saveNavigationHistory(payload); + } + }, + repositionJSViews() { globalPubsub.broadcast("js_views", { type: "reposition" }); }, @@ -1447,7 +1461,22 @@ const Session = { jumpToLine(file, line) { const [_filename, cellId] = file.split("#cell:"); + this.doJumpToLine(cellId, line); + }, + + saveNavigationHistory({ cellId, line }) { + if (cellId === null || line === null) return; + this.history.saveCell(cellId, line); + }, + + goBackNavigationHistory() { + if (this.history.canGoBack()) { + const { cellId, line } = this.history.goBack(); + this.doJumpToLine(cellId, line); + } + }, + doJumpToLine(cellId, line) { this.setFocusedEl(cellId, { scroll: false }); this.setInsertMode(true); diff --git a/assets/js/lib/history.js b/assets/js/lib/history.js new file mode 100644 index 000000000000..87636a0161c0 --- /dev/null +++ b/assets/js/lib/history.js @@ -0,0 +1,103 @@ +/** + * Allows for recording a sequence of focused cells with the focused line + * and navigate inside this stack. + */ +export default class History { + constructor() { + this.entries = []; + this.index = -1; + } + + /** + * Adds a new cell to the stack. + * + * If the stack length is greater than the stack limit, + * it will remove the oldest entries. + */ + saveCell(cellId, line) { + if (this.isTheSameLastEntry(cellId, line)) return; + + if (this.entries[this.index + 1] !== undefined) + this.entries = this.entries.slice(0, this.index + 1); + + this.entries.push({ cellId, line }); + this.index++; + + if (this.entries.length > 20) { + this.entries.shift(); + this.index--; + } + } + + /** + * Immediately clears the stack and reset the current index. + */ + destroy() { + this.entries = []; + this.index = -1; + } + + /** + * Removes all matching cells with given id from the stack. + */ + removeAllFromCell(cellId) { + // We need to make sure the last entry from history + // doesn't belong to the given cell id that we need + // to remove from the entries list. + let currentEntryIndex = this.index; + let currentEntry = this.entries[currentEntryIndex]; + + while (currentEntry.cellId === cellId) { + currentEntryIndex--; + currentEntry = this.entries[currentEntryIndex]; + } + + this.entries = this.entries.filter((entry) => entry.cellId !== cellId); + this.index = this.entries.lastIndexOf(currentEntry); + } + + /** + * Checks if the current stack is available to navigate back. + */ + canGoBack() { + return this.canGetFromHistory(-1); + } + + /** + * Navigates back in the current stack. + * + * If the navigation succeeds, it will return the entry from current index. + * Otherwise, returns null; + */ + goBack() { + return this.getFromHistory(-1); + } + + /** @private **/ + getFromHistory(direction) { + if (!this.canGetFromHistory(direction)) return null; + + this.index = Math.max(0, this.index + direction); + return this.entries[this.index]; + } + + /** @private **/ + canGetFromHistory(direction) { + if (this.index === -1) return false; + if (this.entries.length === 0) return false; + + const index = Math.max(0, this.index + direction); + return this.entries[index] !== undefined; + } + + /** @private **/ + isTheSameLastEntry(cellId, line) { + const lastEntry = this.entries[this.index]; + + return ( + lastEntry !== undefined && + cellId === lastEntry.cellId && + line === lastEntry.line + ); + } +} From d7a7b0944e827f164bccdfa310b98cbbf3cb1fa5 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 18 Sep 2024 16:24:56 -0300 Subject: [PATCH 02/17] Add keyboard shortcut to go to last focused editor --- assets/js/hooks/cell_editor/live_editor.js | 10 +++++++++- .../live/session_live/shortcuts_component.ex | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index dd1050d22aee..0fd4a184962f 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -401,11 +401,19 @@ export default class LiveEditor { // We dispatch escape event, but only if it is not consumed by any // registered handler in the editor, such as closing autocompletion // or escaping Vim insert mode + const cmd = isMacOS() ? event.metaKey : event.ctrlKey; + const alt = event.altKey; + const key = event.key; - if (event.key === "Escape") { + if (key === "Escape") { this.container.dispatchEvent( new CustomEvent("lb:editor_escape", { bubbles: true }), ); + } else if ( + key === "-" && + ((isMacOS() && cmd) || (!isMacOS() && cmd && alt)) + ) { + globalPubsub.broadcast("history", { type: "back" }); } return false; diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index dbe00eedecaf..c86efcfa2b2a 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -113,6 +113,12 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do seq_mac: ["⌘", "k"], press_all: true, desc: "Toggle keyboard control in cell output" + }, + %{ + seq: ["ctrl", "alt", "-"], + seq_mac: ["⌥", "-"], + press_all: true, + desc: "Go back" } ], universal: [ From 6a57f70a2029debd1e3221c8e09a557952f3d461 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 18 Sep 2024 18:16:51 -0300 Subject: [PATCH 03/17] Add tests --- assets/test/lib/history.test.js | 94 +++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 assets/test/lib/history.test.js diff --git a/assets/test/lib/history.test.js b/assets/test/lib/history.test.js new file mode 100644 index 000000000000..9b899567d5d5 --- /dev/null +++ b/assets/test/lib/history.test.js @@ -0,0 +1,94 @@ +import History from "../../js/lib/history"; + +test("goBack returns when there's at least one cell", () => { + const history = new History(); + const expectedEntry = { cellId: "123", line: "1" }; + + expect(history.canGoBack()).toBe(false); + + history.entries = [expectedEntry]; + history.index = 0; + + expect(history.index).toBe(0); + expect(history.canGoBack()).toBe(true); + expect(history.goBack()).toStrictEqual(expectedEntry); +}); + +describe("saveCell", () => { + test("does not add duplicated cells", () => { + const history = new History(); + const entry = { cellId: "123", line: "1" }; + + expect(history.index).toBe(-1); + + history.entries = [entry]; + history.index = 0; + + history.saveCell(entry.cellId, entry.line); + expect(history.index).toBe(0); + + // It will only add a new cell if the line isn't the same + // as the last entry from the stack + history.saveCell(entry.cellId, "2"); + expect(history.index).toBe(1); + }); + + test("removes oldest entries and keep it with a maximum of 20 entries", () => { + const history = new History(); + + for (let i = 0; i <= 19; i++) history.saveCell("123", (i + 1).toString()); + + expect(history.index).toBe(19); + + history.saveCell("231", "1"); + + expect(history.index).toBe(19); + expect(history.entries[0]).toStrictEqual({ cellId: "123", line: "2" }); + }); + + test("rewrites the subsequent cells if go back and saves a new cell", () => { + const history = new History(); + expect(history.canGoBack()).toBe(false); + + history.entries = [ + { cellId: "123", line: "1" }, + { cellId: "456", line: "1" }, + { cellId: "789", line: "2" }, + ]; + history.index = 2; + + expect(history.canGoBack()).toBe(true); + + // Go back to cell id 456 + history.goBack(); + expect(history.index).toBe(1); + + // Go back to cell id 123 + history.goBack(); + expect(history.index).toBe(0); + + // Removes the subsequent cells from stack + // and adds the cell id 999 to the stack + history.saveCell("999", "1"); + expect(history.index).toBe(1); + }); +}); + +test("removeAllFromCell removes the cells with given id", () => { + const history = new History(); + const cellId = "123456789"; + + history.entries = [ + { cellId: "123", line: "1" }, + { cellId: "456", line: "1" }, + { cellId: cellId, line: "1" }, + { cellId: "1234", line: "1" }, + { cellId: cellId, line: "1" }, + { cellId: "8901", line: "1" }, + { cellId: cellId, line: "1" }, + ]; + history.index = 6; + + history.removeAllFromCell(cellId); + expect(history.index).toBe(3); +}); From e70cd0e48ba88c94d1d07236aab7803a5fc23f4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 19 Sep 2024 16:46:36 +0200 Subject: [PATCH 04/17] macOS fixes --- assets/js/hooks/cell_editor/live_editor.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 0fd4a184962f..6562c6897e5c 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -401,7 +401,7 @@ export default class LiveEditor { // We dispatch escape event, but only if it is not consumed by any // registered handler in the editor, such as closing autocompletion // or escaping Vim insert mode - const cmd = isMacOS() ? event.metaKey : event.ctrlKey; + const ctrl = event.ctrlKey; const alt = event.altKey; const key = event.key; @@ -409,10 +409,8 @@ export default class LiveEditor { this.container.dispatchEvent( new CustomEvent("lb:editor_escape", { bubbles: true }), ); - } else if ( - key === "-" && - ((isMacOS() && cmd) || (!isMacOS() && cmd && alt)) - ) { + // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code + } else if (event.code === "Minus" && ctrl && alt) { globalPubsub.broadcast("history", { type: "back" }); } From 4882312e54208506218f635f3a0074cf0014533c Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Thu, 19 Sep 2024 11:51:36 -0300 Subject: [PATCH 05/17] Update sequence mac shortcut --- lib/livebook_web/live/session_live/shortcuts_component.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index c86efcfa2b2a..c6b0dafc0d62 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -116,7 +116,7 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do }, %{ seq: ["ctrl", "alt", "-"], - seq_mac: ["⌥", "-"], + seq_mac: ["⌃", "⌥", "-"], press_all: true, desc: "Go back" } From 91443d89a8c273f40c020f42501121c763e6b85c Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Thu, 19 Sep 2024 12:54:28 -0300 Subject: [PATCH 06/17] npm run format --- assets/js/hooks/cell_editor/live_editor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 6562c6897e5c..73f22c6965b2 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -409,7 +409,7 @@ export default class LiveEditor { this.container.dispatchEvent( new CustomEvent("lb:editor_escape", { bubbles: true }), ); - // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code + // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code } else if (event.code === "Minus" && ctrl && alt) { globalPubsub.broadcast("history", { type: "back" }); } From ad94f5989dbc3643f4cccfc335f84a8fa7045cbe Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Thu, 19 Sep 2024 14:14:58 -0300 Subject: [PATCH 07/17] Allow go back outside of the editor --- assets/js/hooks/session.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index 5a9247608dcd..f1a41a24a408 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -308,6 +308,7 @@ const Session = { } const cmd = isMacOS() ? event.metaKey : event.ctrlKey; + const ctrl = event.ctrlKey; const alt = event.altKey; const shift = event.shiftKey; const key = event.key; @@ -320,7 +321,11 @@ const Session = { event.target.closest(`[data-el-outputs-container]`) ) ) { - if (cmd && shift && !alt && key === "Enter") { + if (event.code === "Minus" && ctrl && alt) { + cancelEvent(event); + this.goBackNavigationHistory(); + return; + } else if (cmd && shift && !alt && key === "Enter") { cancelEvent(event); this.queueFullCellsEvaluation(true); return; From 65ccbfaf6f41840aa1c9c2084f9fc49223096d76 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Thu, 19 Sep 2024 15:21:10 -0300 Subject: [PATCH 08/17] Apply review comments --- .../live/session_live/shortcuts_component.ex | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index c6b0dafc0d62..f11f72cea3ad 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -113,12 +113,6 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do seq_mac: ["⌘", "k"], press_all: true, desc: "Toggle keyboard control in cell output" - }, - %{ - seq: ["ctrl", "alt", "-"], - seq_mac: ["⌃", "⌥", "-"], - press_all: true, - desc: "Go back" } ], universal: [ @@ -149,6 +143,12 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do press_all: true, desc: "Save notebook", basic: true + }, + %{ + seq: ["ctrl", "alt", "-"], + seq_mac: ["⌃", "⌥", "-"], + press_all: true, + desc: "Go back (cursor)" } ] } From 56de1d3f9a3b12a6f10042272f8435c84c720e5b Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Thu, 19 Sep 2024 15:52:26 -0300 Subject: [PATCH 09/17] Update lib/livebook_web/live/session_live/shortcuts_component.ex --- lib/livebook_web/live/session_live/shortcuts_component.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index f11f72cea3ad..1444b70190d1 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -148,7 +148,7 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do seq: ["ctrl", "alt", "-"], seq_mac: ["⌃", "⌥", "-"], press_all: true, - desc: "Go back (cursor)" + desc: "Go back to last editor" } ] } From 9297065203184887e4657d4d4fc2783c87ece2f4 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Mon, 23 Sep 2024 12:52:52 -0300 Subject: [PATCH 10/17] Apply review comments --- assets/js/hooks/cell.js | 37 ++++--- assets/js/hooks/cell_editor/live_editor.js | 28 +++--- assets/js/hooks/session.js | 55 +++++++---- .../session/cursor_history.js} | 50 ++++++---- .../test/hooks/session/cursor_history.test.js | 99 +++++++++++++++++++ assets/test/lib/history.test.js | 94 ------------------ .../live/session_live/shortcuts_component.ex | 8 +- 7 files changed, 213 insertions(+), 158 deletions(-) rename assets/js/{lib/history.js => hooks/session/cursor_history.js} (70%) create mode 100644 assets/test/hooks/session/cursor_history.test.js delete mode 100644 assets/test/lib/history.test.js diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index 0faef2cbea22..0fd6b481f8eb 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -156,7 +156,13 @@ const Cell = { if (event.type === "dispatch_queue_evaluation") { this.handleDispatchQueueEvaluation(event.dispatch); } else if (event.type === "jump_to_line") { - this.handleJumpToLine(event.line); + if (this.isFocused) { + this.currentEditor().moveCursorToLine(event.line); + } + } else if (event.type === "jump_to_line_and_column") { + if (this.isFocused) { + this.currentEditor().moveCursorToLine(event.line, event.column); + } } }, @@ -173,12 +179,6 @@ const Cell = { } }, - handleJumpToLine(line) { - if (this.isFocused) { - this.currentEditor().moveCursorToLine(line); - } - }, - handleCellEditorCreated(tag, liveEditor) { this.liveEditors[tag] = liveEditor; @@ -198,6 +198,8 @@ const Cell = { if (this.isFocused && this.insertMode) { this.currentEditor().focus(); } + + this.sendCursorHistory(); }, 0); }); @@ -212,14 +214,7 @@ const Cell = { if (!this.isFocused || !this.insertMode) { this.currentEditor().blur(); } else if (this.insertMode) { - const lineNumber = this.currentEditor().getLineNumberAtCursor(); - - if (lineNumber !== null) - globalPubsub.broadcast("history", { - type: "navigation", - cellId: this.props.cellId, - line: lineNumber.toString(), - }); + this.sendCursorHistory(); } }, 0); }); @@ -379,6 +374,18 @@ const Cell = { }); }); }, + + sendCursorHistory() { + const cursor = this.currentEditor().getCurrentCursorPosition(); + if (cursor === null) return; + + globalPubsub.broadcast("history", { + type: "navigation", + cellId: this.props.cellId, + line: cursor.line.toString(), + column: cursor.column.toString(), + }); + }, }; export default Cell; diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 73f22c6965b2..1924532b950f 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -167,15 +167,22 @@ export default class LiveEditor { } /** - * Returns line number from the current main cursor position. + * Returns the current main cursor position. */ - getLineNumberAtCursor() { + getCurrentCursorPosition() { if (!this.isMounted()) { return null; } const pos = this.view.state.selection.main.head; - return this.view.state.doc.lineAt(pos).number; + const line = this.view.state.doc.lineAt(pos); + const lineLength = line.to - line.from; + const column = pos - line.from; + + if (column < 1 || column > lineLength) + return { line: line.number, column: 1 }; + + return { line: line.number, column }; } /** @@ -195,11 +202,13 @@ export default class LiveEditor { /** * Updates editor selection such that cursor points to the given line. */ - moveCursorToLine(lineNumber) { + moveCursorToLine(lineNumber, column = "1") { const line = this.view.state.doc.line(lineNumber); + const columnNumber = parseInt(column); + const position = line.from + columnNumber; this.view.dispatch({ - selection: EditorSelection.single(line.from), + selection: EditorSelection.single(position), }); } @@ -401,17 +410,10 @@ export default class LiveEditor { // We dispatch escape event, but only if it is not consumed by any // registered handler in the editor, such as closing autocompletion // or escaping Vim insert mode - const ctrl = event.ctrlKey; - const alt = event.altKey; - const key = event.key; - - if (key === "Escape") { + if (event.key === "Escape") { this.container.dispatchEvent( new CustomEvent("lb:editor_escape", { bubbles: true }), ); - // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code - } else if (event.code === "Minus" && ctrl && alt) { - globalPubsub.broadcast("history", { type: "back" }); } return false; diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index f1a41a24a408..4bbce920a2ff 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -18,7 +18,7 @@ import { leaveChannel } from "./js_view/channel"; import { isDirectlyEditable, isEvaluable } from "../lib/notebook"; import { settingsStore } from "../lib/settings"; import { LiveStore } from "../lib/live_store"; -import History from "../lib/history"; +import CursorHistory from "./session/cursor_history"; /** * A hook managing the whole session. @@ -82,7 +82,7 @@ const Session = { this.viewOptions = null; this.keyBuffer = new KeyBuffer(); this.lastLocationReportByClientId = {}; - this.history = new History(); + this.cursorHistory = new CursorHistory(); this.followedClientId = null; this.store = LiveStore.create("session"); @@ -281,7 +281,6 @@ const Session = { this.subscriptions.forEach((subscription) => subscription.destroy()); this.store.destroy(); - this.history.destroy(); }, getProps() { @@ -321,10 +320,15 @@ const Session = { event.target.closest(`[data-el-outputs-container]`) ) ) { + // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code if (event.code === "Minus" && ctrl && alt) { cancelEvent(event); this.goBackNavigationHistory(); return; + } else if (key === "=" && ctrl && alt) { + cancelEvent(event); + this.goForwardNavigationHistory(); + return; } else if (cmd && shift && !alt && key === "Enter") { cancelEvent(event); this.queueFullCellsEvaluation(true); @@ -1236,7 +1240,7 @@ const Session = { }, handleCellDeleted(cellId, siblingCellId) { - this.history.removeAllFromCell(cellId); + this.cursorHistory.removeAllFromCell(cellId); if (this.focusedId === cellId) { if (this.view) { @@ -1336,9 +1340,7 @@ const Session = { }, handleHistoryEvent(payload) { - if (payload.type === "back") { - this.goBackNavigationHistory(); - } else if (payload.type === "navigation") { + if (payload.type === "navigation") { this.saveNavigationHistory(payload); } }, @@ -1466,26 +1468,43 @@ const Session = { jumpToLine(file, line) { const [_filename, cellId] = file.split("#cell:"); - this.doJumpToLine(cellId, line); + this.setFocusedEl(cellId, { scroll: false }); + this.setInsertMode(true); + + globalPubsub.broadcast(`cells:${cellId}`, { type: "jump_to_line", line }); }, - saveNavigationHistory({ cellId, line }) { - if (cellId === null || line === null) return; - this.history.saveCell(cellId, line); + saveNavigationHistory({ cellId, line, column }) { + if (cellId === null || line === null || column === null) return; + this.cursorHistory.push(cellId, line, column); }, goBackNavigationHistory() { - if (this.history.canGoBack()) { - const { cellId, line } = this.history.goBack(); - this.doJumpToLine(cellId, line); + if (this.cursorHistory.canGoBack()) { + const { cellId, line, column } = this.cursorHistory.goBack(); + this.setFocusedEl(cellId, { scroll: false }); + this.setInsertMode(true); + + globalPubsub.broadcast(`cells:${cellId}`, { + type: "jump_to_line_and_column", + line, + column, + }); } }, - doJumpToLine(cellId, line) { - this.setFocusedEl(cellId, { scroll: false }); - this.setInsertMode(true); + goForwardNavigationHistory() { + if (this.cursorHistory.canGoForward()) { + const { cellId, line, column } = this.cursorHistory.goForward(); + this.setFocusedEl(cellId, { scroll: false }); + this.setInsertMode(true); - globalPubsub.broadcast(`cells:${cellId}`, { type: "jump_to_line", line }); + globalPubsub.broadcast(`cells:${cellId}`, { + type: "jump_to_line_and_column", + line, + column, + }); + } }, }; diff --git a/assets/js/lib/history.js b/assets/js/hooks/session/cursor_history.js similarity index 70% rename from assets/js/lib/history.js rename to assets/js/hooks/session/cursor_history.js index 87636a0161c0..cfacf14b4767 100644 --- a/assets/js/lib/history.js +++ b/assets/js/hooks/session/cursor_history.js @@ -2,7 +2,7 @@ * Allows for recording a sequence of focused cells with the focused line * and navigate inside this stack. */ -export default class History { +export default class CursorHistory { constructor() { this.entries = []; this.index = -1; @@ -14,14 +14,19 @@ export default class History { * If the stack length is greater than the stack limit, * it will remove the oldest entries. */ - saveCell(cellId, line) { - if (this.isTheSameLastEntry(cellId, line)) return; - - if (this.entries[this.index + 1] !== undefined) - this.entries = this.entries.slice(0, this.index + 1); - - this.entries.push({ cellId, line }); - this.index++; + push(cellId, line, column) { + const entry = { cellId, line, column }; + + if (this.isTheSameCell(cellId)) { + this.entries[this.index] = entry; + } else { + if (this.entries[this.index + 1] !== undefined) { + this.entries = this.entries.slice(0, this.index + 1); + } + + this.entries.push(entry); + this.index++; + } if (this.entries.length > 20) { this.entries.shift(); @@ -73,6 +78,23 @@ export default class History { return this.getFromHistory(-1); } + /** + * Checks if the current stack is available to navigate forward. + */ + canGoForward() { + return this.canGetFromHistory(1); + } + + /** + * Navigates forward in the current stack. + * + * If the navigation succeeds, it will return the entry from current index. + * Otherwise, returns null; + */ + goForward() { + return this.getFromHistory(1); + } + /** @private **/ getFromHistory(direction) { if (!this.canGetFromHistory(direction)) return null; @@ -83,7 +105,6 @@ export default class History { /** @private **/ canGetFromHistory(direction) { - if (this.index === -1) return false; if (this.entries.length === 0) return false; const index = Math.max(0, this.index + direction); @@ -91,13 +112,8 @@ export default class History { } /** @private **/ - isTheSameLastEntry(cellId, line) { + isTheSameCell(cellId) { const lastEntry = this.entries[this.index]; - - return ( - lastEntry !== undefined && - cellId === lastEntry.cellId && - line === lastEntry.line - ); + return lastEntry !== undefined && cellId === lastEntry.cellId; } } diff --git a/assets/test/hooks/session/cursor_history.test.js b/assets/test/hooks/session/cursor_history.test.js new file mode 100644 index 000000000000..2e2352d9a93a --- /dev/null +++ b/assets/test/hooks/session/cursor_history.test.js @@ -0,0 +1,99 @@ +import CursorHistory from "../../../js/hooks/session/cursor_history"; + +test("goBack returns when there's at least one cell", () => { + const cursorHistory = new CursorHistory(); + const expectedEntry = { cellId: "123", line: "1", column: "1" }; + + expect(cursorHistory.canGoBack()).toBe(false); + + cursorHistory.entries = [expectedEntry]; + cursorHistory.index = 0; + + expect(cursorHistory.index).toBe(0); + expect(cursorHistory.canGoBack()).toBe(true); + expect(cursorHistory.goBack()).toStrictEqual(expectedEntry); +}); + +describe("push", () => { + test("does not add duplicated cells", () => { + const cursorHistory = new CursorHistory(); + const entry = { cellId: "123", line: "1", column: "1" }; + + expect(cursorHistory.index).toBe(-1); + + cursorHistory.entries = [entry]; + cursorHistory.index = 0; + + cursorHistory.push(entry.cellId, entry.line, "2"); + expect(cursorHistory.index).toBe(0); + + cursorHistory.push(entry.cellId, "2", entry.column); + expect(cursorHistory.index).toBe(0); + }); + + test("removes oldest entries and keep it with a maximum of 20 entries", () => { + const cursorHistory = new CursorHistory(); + + for (let i = 0; i <= 19; i++) { + const value = (i + 1).toString(); + cursorHistory.push(`123${value}`, value, "1"); + } + + expect(cursorHistory.index).toBe(19); + + cursorHistory.push("231", "1", "1"); + + expect(cursorHistory.index).toBe(19); + expect(cursorHistory.entries[0]).toStrictEqual({ + cellId: "1232", + column: "1", + line: "2", + }); + }); + + test("rewrites the subsequent cells if go back and saves a new cell", () => { + const cursorHistory = new CursorHistory(); + expect(cursorHistory.canGoBack()).toBe(false); + + cursorHistory.entries = [ + { cellId: "123", line: "1", column: "1" }, + { cellId: "456", line: "1", column: "1" }, + { cellId: "789", line: "2", column: "1" }, + ]; + cursorHistory.index = 2; + + expect(cursorHistory.canGoBack()).toBe(true); + + // Go back to cell id 456 + cursorHistory.goBack(); + expect(cursorHistory.index).toBe(1); + + // Go back to cell id 123 + cursorHistory.goBack(); + expect(cursorHistory.index).toBe(0); + + // Removes the subsequent cells from stack + // and adds the cell id 999 to the stack + cursorHistory.push("999", "1", "1"); + expect(cursorHistory.index).toBe(1); + }); +}); + +test("removeAllFromCell removes the cells with given id", () => { + const cursorHistory = new CursorHistory(); + const cellId = "123456789"; + + cursorHistory.entries = [ + { cellId: "123", line: "1", column: "1" }, + { cellId: "456", line: "1", column: "1" }, + { cellId: cellId, line: "1", column: "1" }, + { cellId: "1234", line: "1", column: "1" }, + { cellId: cellId, line: "1", column: "1" }, + { cellId: "8901", line: "1", column: "1" }, + { cellId: cellId, line: "1", column: "1" }, + ]; + cursorHistory.index = 6; + + cursorHistory.removeAllFromCell(cellId); + expect(cursorHistory.index).toBe(3); +}); diff --git a/assets/test/lib/history.test.js b/assets/test/lib/history.test.js deleted file mode 100644 index 9b899567d5d5..000000000000 --- a/assets/test/lib/history.test.js +++ /dev/null @@ -1,94 +0,0 @@ -import History from "../../js/lib/history"; - -test("goBack returns when there's at least one cell", () => { - const history = new History(); - const expectedEntry = { cellId: "123", line: "1" }; - - expect(history.canGoBack()).toBe(false); - - history.entries = [expectedEntry]; - history.index = 0; - - expect(history.index).toBe(0); - expect(history.canGoBack()).toBe(true); - expect(history.goBack()).toStrictEqual(expectedEntry); -}); - -describe("saveCell", () => { - test("does not add duplicated cells", () => { - const history = new History(); - const entry = { cellId: "123", line: "1" }; - - expect(history.index).toBe(-1); - - history.entries = [entry]; - history.index = 0; - - history.saveCell(entry.cellId, entry.line); - expect(history.index).toBe(0); - - // It will only add a new cell if the line isn't the same - // as the last entry from the stack - history.saveCell(entry.cellId, "2"); - expect(history.index).toBe(1); - }); - - test("removes oldest entries and keep it with a maximum of 20 entries", () => { - const history = new History(); - - for (let i = 0; i <= 19; i++) history.saveCell("123", (i + 1).toString()); - - expect(history.index).toBe(19); - - history.saveCell("231", "1"); - - expect(history.index).toBe(19); - expect(history.entries[0]).toStrictEqual({ cellId: "123", line: "2" }); - }); - - test("rewrites the subsequent cells if go back and saves a new cell", () => { - const history = new History(); - expect(history.canGoBack()).toBe(false); - - history.entries = [ - { cellId: "123", line: "1" }, - { cellId: "456", line: "1" }, - { cellId: "789", line: "2" }, - ]; - history.index = 2; - - expect(history.canGoBack()).toBe(true); - - // Go back to cell id 456 - history.goBack(); - expect(history.index).toBe(1); - - // Go back to cell id 123 - history.goBack(); - expect(history.index).toBe(0); - - // Removes the subsequent cells from stack - // and adds the cell id 999 to the stack - history.saveCell("999", "1"); - expect(history.index).toBe(1); - }); -}); - -test("removeAllFromCell removes the cells with given id", () => { - const history = new History(); - const cellId = "123456789"; - - history.entries = [ - { cellId: "123", line: "1" }, - { cellId: "456", line: "1" }, - { cellId: cellId, line: "1" }, - { cellId: "1234", line: "1" }, - { cellId: cellId, line: "1" }, - { cellId: "8901", line: "1" }, - { cellId: cellId, line: "1" }, - ]; - history.index = 6; - - history.removeAllFromCell(cellId); - expect(history.index).toBe(3); -}); diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index 1444b70190d1..67d89857c3f9 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -148,7 +148,13 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do seq: ["ctrl", "alt", "-"], seq_mac: ["⌃", "⌥", "-"], press_all: true, - desc: "Go back to last editor" + desc: "Go back to previous editor" + }, + %{ + seq: ["ctrl", "alt", "="], + seq_mac: ["⌃", "⌥", "="], + press_all: true, + desc: "Go forward to next editor" } ] } From 15a2f895ad986a346dc65b81a73e24fe84fd9513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 24 Sep 2024 09:57:49 +0200 Subject: [PATCH 11/17] Focus previous/next -> Focus above/below --- lib/livebook_web/live/session_live/shortcuts_component.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/livebook_web/live/session_live/shortcuts_component.ex b/lib/livebook_web/live/session_live/shortcuts_component.ex index 67d89857c3f9..fac322bd009e 100644 --- a/lib/livebook_web/live/session_live/shortcuts_component.ex +++ b/lib/livebook_web/live/session_live/shortcuts_component.ex @@ -82,8 +82,8 @@ defmodule LivebookWeb.SessionLive.ShortcutsComponent do ], navigation_mode: [ %{seq: ["?"], desc: "Open this help modal", basic: true}, - %{seq: ["j"], desc: "Focus next cell", basic: true}, - %{seq: ["k"], desc: "Focus previous cell", basic: true}, + %{seq: ["j"], desc: "Focus cell below", basic: true}, + %{seq: ["k"], desc: "Focus cell above", basic: true}, %{seq: ["J"], desc: "Move cell down"}, %{seq: ["K"], desc: "Move cell up"}, %{seq: ["i"], desc: "Switch to insert mode", basic: true}, From ef8a71a67c91e1c0c0eba24c772c13152326bd3d Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Tue, 24 Sep 2024 10:33:03 -0300 Subject: [PATCH 12/17] Remove unused function --- assets/js/hooks/session/cursor_history.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/assets/js/hooks/session/cursor_history.js b/assets/js/hooks/session/cursor_history.js index cfacf14b4767..3accb06d4727 100644 --- a/assets/js/hooks/session/cursor_history.js +++ b/assets/js/hooks/session/cursor_history.js @@ -34,14 +34,6 @@ export default class CursorHistory { } } - /** - * Immediately clears the stack and reset the current index. - */ - destroy() { - this.entries = []; - this.index = -1; - } - /** * Removes all matching cells with given id from the stack. */ From ff655cd60a6626eebe0b962fd69ffe573f524b40 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Tue, 24 Sep 2024 14:29:36 -0300 Subject: [PATCH 13/17] Apply review comments --- assets/js/hooks/cell.js | 21 ++--- assets/js/hooks/cell_editor/live_editor.js | 45 ++++++---- assets/js/hooks/session.js | 31 +++---- assets/js/hooks/session/cursor_history.js | 32 ++++--- .../test/hooks/session/cursor_history.test.js | 88 +++++++++---------- 5 files changed, 118 insertions(+), 99 deletions(-) diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index 0fd6b481f8eb..db6b15511b74 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -157,11 +157,7 @@ const Cell = { this.handleDispatchQueueEvaluation(event.dispatch); } else if (event.type === "jump_to_line") { if (this.isFocused) { - this.currentEditor().moveCursorToLine(event.line); - } - } else if (event.type === "jump_to_line_and_column") { - if (this.isFocused) { - this.currentEditor().moveCursorToLine(event.line, event.column); + this.currentEditor().moveCursorToLine(event.line, event.offset || 0); } } }, @@ -198,8 +194,6 @@ const Cell = { if (this.isFocused && this.insertMode) { this.currentEditor().focus(); } - - this.sendCursorHistory(); }, 0); }); @@ -213,7 +207,15 @@ const Cell = { // gives it focus if (!this.isFocused || !this.insertMode) { this.currentEditor().blur(); - } else if (this.insertMode) { + } + }, 0); + }); + + liveEditor.onViewUpdate((viewUpdate) => { + // We defer the check to happen after all focus/click events have + // been processed, in case the state changes as a result + setTimeout(() => { + if (this.isFocused && viewUpdate.selectionSet) { this.sendCursorHistory(); } }, 0); @@ -380,10 +382,9 @@ const Cell = { if (cursor === null) return; globalPubsub.broadcast("history", { + ...cursor, type: "navigation", cellId: this.props.cellId, - line: cursor.line.toString(), - column: cursor.column.toString(), }); }, }; diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 1924532b950f..a21fb0c42886 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -99,6 +99,14 @@ export default class LiveEditor { */ onFocus = this._onFocus.event; + /** @private */ + _onViewUpdate = new Emitter(); + + /** + * Registers a callback called whenever the editor updates the view. + */ + onViewUpdate = this._onViewUpdate.event; + constructor( container, connection, @@ -176,13 +184,9 @@ export default class LiveEditor { const pos = this.view.state.selection.main.head; const line = this.view.state.doc.lineAt(pos); - const lineLength = line.to - line.from; - const column = pos - line.from; - - if (column < 1 || column > lineLength) - return { line: line.number, column: 1 }; + const offset = pos - line.from; - return { line: line.number, column }; + return { line: line.number, offset }; } /** @@ -202,10 +206,9 @@ export default class LiveEditor { /** * Updates editor selection such that cursor points to the given line. */ - moveCursorToLine(lineNumber, column = "1") { + moveCursorToLine(lineNumber, offset) { const line = this.view.state.doc.line(lineNumber); - const columnNumber = parseInt(column); - const position = line.from + columnNumber; + const position = line.from + offset; this.view.dispatch({ selection: EditorSelection.single(position), @@ -329,6 +332,10 @@ export default class LiveEditor { { key: "Alt-Enter", run: insertBlankLineAndCloseHints }, ]; + const selectionChangeListener = EditorView.updateListener.of((viewUpdate) => + this.handleViewUpdate(viewUpdate), + ); + this.view = new EditorView({ parent: this.container, doc: this.source, @@ -372,13 +379,13 @@ export default class LiveEditor { }), this.intellisense ? [ - autocompletion({ override: [this.completionSource.bind(this)] }), - hoverDetails(this.docsHoverTooltipSource.bind(this)), - signature(this.signatureSource.bind(this), { - activateOnTyping: settings.editor_auto_signature, - }), - formatter(this.formatterSource.bind(this)), - ] + autocompletion({ override: [this.completionSource.bind(this)] }), + hoverDetails(this.docsHoverTooltipSource.bind(this)), + signature(this.signatureSource.bind(this), { + activateOnTyping: settings.editor_auto_signature, + }), + formatter(this.formatterSource.bind(this)), + ] : [], settings.editor_mode === "vim" ? [vim()] : [], settings.editor_mode === "emacs" ? [emacs()] : [], @@ -390,6 +397,7 @@ export default class LiveEditor { focus: this.handleEditorFocus.bind(this), }), EditorView.clickAddsSelectionRange.of((event) => event.altKey), + selectionChangeListener, ], }); } @@ -435,6 +443,11 @@ export default class LiveEditor { return false; } + /** @private */ + handleViewUpdate(viewUpdate) { + this._onViewUpdate.dispatch(viewUpdate); + } + /** @private */ completionSource(context) { const settings = settingsStore.get(); diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index 4bbce920a2ff..3b01cb605e1f 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -323,11 +323,11 @@ const Session = { // On macOS, ctrl+alt+- becomes an em-dash, so we check for the code if (event.code === "Minus" && ctrl && alt) { cancelEvent(event); - this.goBackNavigationHistory(); + this.cursorHistoryGoBack(); return; } else if (key === "=" && ctrl && alt) { cancelEvent(event); - this.goForwardNavigationHistory(); + this.cursorHistoryGoForward(); return; } else if (cmd && shift && !alt && key === "Enter") { cancelEvent(event); @@ -1339,9 +1339,9 @@ const Session = { } }, - handleHistoryEvent(payload) { - if (payload.type === "navigation") { - this.saveNavigationHistory(payload); + handleHistoryEvent(event) { + if (event.type === "navigation") { + this.cursorHistory.push(event.cellId, event.line, event.offset); } }, @@ -1474,35 +1474,30 @@ const Session = { globalPubsub.broadcast(`cells:${cellId}`, { type: "jump_to_line", line }); }, - saveNavigationHistory({ cellId, line, column }) { - if (cellId === null || line === null || column === null) return; - this.cursorHistory.push(cellId, line, column); - }, - - goBackNavigationHistory() { + cursorHistoryGoBack() { if (this.cursorHistory.canGoBack()) { - const { cellId, line, column } = this.cursorHistory.goBack(); + const { cellId, line, offset } = this.cursorHistory.goBack(); this.setFocusedEl(cellId, { scroll: false }); this.setInsertMode(true); globalPubsub.broadcast(`cells:${cellId}`, { - type: "jump_to_line_and_column", + type: "jump_to_line", line, - column, + offset, }); } }, - goForwardNavigationHistory() { + cursorHistoryGoForward() { if (this.cursorHistory.canGoForward()) { - const { cellId, line, column } = this.cursorHistory.goForward(); + const { cellId, line, offset } = this.cursorHistory.goForward(); this.setFocusedEl(cellId, { scroll: false }); this.setInsertMode(true); globalPubsub.broadcast(`cells:${cellId}`, { - type: "jump_to_line_and_column", + type: "jump_to_line", line, - column, + offset, }); } }, diff --git a/assets/js/hooks/session/cursor_history.js b/assets/js/hooks/session/cursor_history.js index 3accb06d4727..8a9f65a74627 100644 --- a/assets/js/hooks/session/cursor_history.js +++ b/assets/js/hooks/session/cursor_history.js @@ -3,10 +3,11 @@ * and navigate inside this stack. */ export default class CursorHistory { - constructor() { - this.entries = []; - this.index = -1; - } + /** @private */ + entries = []; + + /** @private */ + index = -1; /** * Adds a new cell to the stack. @@ -14,10 +15,10 @@ export default class CursorHistory { * If the stack length is greater than the stack limit, * it will remove the oldest entries. */ - push(cellId, line, column) { - const entry = { cellId, line, column }; + push(cellId, line, offset) { + const entry = { cellId, line, offset }; - if (this.isTheSameCell(cellId)) { + if (this.isSameCell(cellId)) { this.entries[this.index] = entry; } else { if (this.entries[this.index + 1] !== undefined) { @@ -87,6 +88,17 @@ export default class CursorHistory { return this.getFromHistory(1); } + /** + * Gets the entry in the current stack. + * + * If the stack have at least one entry, it will return the entry from current index. + * Otherwise, returns null; + */ + getCurrent() { + if (this.entries.length <= 0) return null; + return this.entries[this.index]; + } + /** @private **/ getFromHistory(direction) { if (!this.canGetFromHistory(direction)) return null; @@ -99,12 +111,12 @@ export default class CursorHistory { canGetFromHistory(direction) { if (this.entries.length === 0) return false; - const index = Math.max(0, this.index + direction); - return this.entries[index] !== undefined; + const index = this.index + direction; + return 0 <= index && index < this.entries.length; } /** @private **/ - isTheSameCell(cellId) { + isSameCell(cellId) { const lastEntry = this.entries[this.index]; return lastEntry !== undefined && cellId === lastEntry.cellId; } diff --git a/assets/test/hooks/session/cursor_history.test.js b/assets/test/hooks/session/cursor_history.test.js index 2e2352d9a93a..040f4f2c2d29 100644 --- a/assets/test/hooks/session/cursor_history.test.js +++ b/assets/test/hooks/session/cursor_history.test.js @@ -2,52 +2,51 @@ import CursorHistory from "../../../js/hooks/session/cursor_history"; test("goBack returns when there's at least one cell", () => { const cursorHistory = new CursorHistory(); - const expectedEntry = { cellId: "123", line: "1", column: "1" }; + const entry = { cellId: Math.random().toString(36), line: 1, offset: 1 }; expect(cursorHistory.canGoBack()).toBe(false); + expect(cursorHistory.getCurrent()).toStrictEqual(null); - cursorHistory.entries = [expectedEntry]; - cursorHistory.index = 0; + cursorHistory.push(entry.cellId, entry.line, entry.offset); - expect(cursorHistory.index).toBe(0); - expect(cursorHistory.canGoBack()).toBe(true); - expect(cursorHistory.goBack()).toStrictEqual(expectedEntry); + expect(cursorHistory.canGoBack()).toBe(false); + expect(cursorHistory.getCurrent()).toStrictEqual(entry); }); describe("push", () => { test("does not add duplicated cells", () => { const cursorHistory = new CursorHistory(); - const entry = { cellId: "123", line: "1", column: "1" }; - - expect(cursorHistory.index).toBe(-1); + const entry = { cellId: Math.random().toString(36), line: 1, offset: 1 }; - cursorHistory.entries = [entry]; - cursorHistory.index = 0; + expect(cursorHistory.canGoBack()).toBe(false); - cursorHistory.push(entry.cellId, entry.line, "2"); - expect(cursorHistory.index).toBe(0); + cursorHistory.push(entry.cellId, entry.line, entry.offset); + cursorHistory.push(entry.cellId, entry.line, 2); + expect(cursorHistory.canGoBack()).toBe(false); - cursorHistory.push(entry.cellId, "2", entry.column); - expect(cursorHistory.index).toBe(0); + cursorHistory.push(entry.cellId, 2, entry.offset); + expect(cursorHistory.canGoBack()).toBe(false); }); test("removes oldest entries and keep it with a maximum of 20 entries", () => { const cursorHistory = new CursorHistory(); for (let i = 0; i <= 19; i++) { - const value = (i + 1).toString(); - cursorHistory.push(`123${value}`, value, "1"); + const value = i + 1; + cursorHistory.push(`123${value}`, value, 1); } - expect(cursorHistory.index).toBe(19); + cursorHistory.push("231", 1, 1); - cursorHistory.push("231", "1", "1"); + // Navigates to the bottom of the stack + for (let i = 0; i <= 18; i++) { + cursorHistory.goBack(); + } - expect(cursorHistory.index).toBe(19); - expect(cursorHistory.entries[0]).toStrictEqual({ + expect(cursorHistory.getCurrent()).toStrictEqual({ cellId: "1232", - column: "1", - line: "2", + offset: 1, + line: 2, }); }); @@ -55,27 +54,24 @@ describe("push", () => { const cursorHistory = new CursorHistory(); expect(cursorHistory.canGoBack()).toBe(false); - cursorHistory.entries = [ - { cellId: "123", line: "1", column: "1" }, - { cellId: "456", line: "1", column: "1" }, - { cellId: "789", line: "2", column: "1" }, - ]; - cursorHistory.index = 2; + cursorHistory.push(Math.random().toString(36), 1, 1); + cursorHistory.push(Math.random().toString(36), 1, 1); + cursorHistory.push(Math.random().toString(36), 2, 1); expect(cursorHistory.canGoBack()).toBe(true); // Go back to cell id 456 cursorHistory.goBack(); - expect(cursorHistory.index).toBe(1); + expect(cursorHistory.canGoBack()).toBe(true); // Go back to cell id 123 cursorHistory.goBack(); - expect(cursorHistory.index).toBe(0); + expect(cursorHistory.canGoBack()).toBe(false); // Removes the subsequent cells from stack - // and adds the cell id 999 to the stack - cursorHistory.push("999", "1", "1"); - expect(cursorHistory.index).toBe(1); + // and adds this cell to the stack + cursorHistory.push(Math.random().toString(36), 1, 1); + expect(cursorHistory.canGoForward()).toBe(false); }); }); @@ -83,17 +79,19 @@ test("removeAllFromCell removes the cells with given id", () => { const cursorHistory = new CursorHistory(); const cellId = "123456789"; - cursorHistory.entries = [ - { cellId: "123", line: "1", column: "1" }, - { cellId: "456", line: "1", column: "1" }, - { cellId: cellId, line: "1", column: "1" }, - { cellId: "1234", line: "1", column: "1" }, - { cellId: cellId, line: "1", column: "1" }, - { cellId: "8901", line: "1", column: "1" }, - { cellId: cellId, line: "1", column: "1" }, - ]; - cursorHistory.index = 6; + cursorHistory.push("123", 1, 1); + cursorHistory.push("456", 1, 1); + cursorHistory.push(cellId, 1, 1); + cursorHistory.push("1234", 1, 1); + cursorHistory.push(cellId, 1, 1); + cursorHistory.push("8901", 1, 1); + cursorHistory.push(cellId, 1, 1); cursorHistory.removeAllFromCell(cellId); - expect(cursorHistory.index).toBe(3); + expect(cursorHistory.canGoForward()).toBe(false); + expect(cursorHistory.getCurrent()).toStrictEqual({ + cellId: "8901", + line: 1, + offset: 1, + }); }); From cbcdd797e37cc873d70d4d44f47386e123509630 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Tue, 24 Sep 2024 14:30:45 -0300 Subject: [PATCH 14/17] npm run format --- assets/js/hooks/cell_editor/live_editor.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index a21fb0c42886..92a7eab511cb 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -379,13 +379,13 @@ export default class LiveEditor { }), this.intellisense ? [ - autocompletion({ override: [this.completionSource.bind(this)] }), - hoverDetails(this.docsHoverTooltipSource.bind(this)), - signature(this.signatureSource.bind(this), { - activateOnTyping: settings.editor_auto_signature, - }), - formatter(this.formatterSource.bind(this)), - ] + autocompletion({ override: [this.completionSource.bind(this)] }), + hoverDetails(this.docsHoverTooltipSource.bind(this)), + signature(this.signatureSource.bind(this), { + activateOnTyping: settings.editor_auto_signature, + }), + formatter(this.formatterSource.bind(this)), + ] : [], settings.editor_mode === "vim" ? [vim()] : [], settings.editor_mode === "emacs" ? [emacs()] : [], From dafadcd7b2aa69b53657846592ae5dff1ccedc28 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 25 Sep 2024 12:33:03 -0300 Subject: [PATCH 15/17] Apply review comments --- assets/js/hooks/cell.js | 4 +-- assets/js/hooks/cell_editor/live_editor.js | 28 ++++++++--------- assets/js/hooks/session/cursor_history.js | 31 +++++++++++++------ .../test/hooks/session/cursor_history.test.js | 25 +++++++-------- 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index db6b15511b74..872306f82278 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -211,11 +211,11 @@ const Cell = { }, 0); }); - liveEditor.onViewUpdate((viewUpdate) => { + liveEditor.onSelectionChange((update) => { // We defer the check to happen after all focus/click events have // been processed, in case the state changes as a result setTimeout(() => { - if (this.isFocused && viewUpdate.selectionSet) { + if (this.isFocused && !update.state.selection.eq(update.startState.selection)) { this.sendCursorHistory(); } }, 0); diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 92a7eab511cb..22ebe3e2e3bd 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -100,12 +100,12 @@ export default class LiveEditor { onFocus = this._onFocus.event; /** @private */ - _onViewUpdate = new Emitter(); + _onSelectionChange = new Emitter(); /** - * Registers a callback called whenever the editor updates the view. + * Registers a callback called whenever the editor changes selection. */ - onViewUpdate = this._onViewUpdate.event; + onSelectionChange = this._onSelectionChange.event; constructor( container, @@ -332,8 +332,8 @@ export default class LiveEditor { { key: "Alt-Enter", run: insertBlankLineAndCloseHints }, ]; - const selectionChangeListener = EditorView.updateListener.of((viewUpdate) => - this.handleViewUpdate(viewUpdate), + const selectionChangeListener = EditorView.updateListener.of((update) => + this.handleOnSelectionChange(update), ); this.view = new EditorView({ @@ -379,13 +379,13 @@ export default class LiveEditor { }), this.intellisense ? [ - autocompletion({ override: [this.completionSource.bind(this)] }), - hoverDetails(this.docsHoverTooltipSource.bind(this)), - signature(this.signatureSource.bind(this), { - activateOnTyping: settings.editor_auto_signature, - }), - formatter(this.formatterSource.bind(this)), - ] + autocompletion({ override: [this.completionSource.bind(this)] }), + hoverDetails(this.docsHoverTooltipSource.bind(this)), + signature(this.signatureSource.bind(this), { + activateOnTyping: settings.editor_auto_signature, + }), + formatter(this.formatterSource.bind(this)), + ] : [], settings.editor_mode === "vim" ? [vim()] : [], settings.editor_mode === "emacs" ? [emacs()] : [], @@ -444,8 +444,8 @@ export default class LiveEditor { } /** @private */ - handleViewUpdate(viewUpdate) { - this._onViewUpdate.dispatch(viewUpdate); + handleOnSelectionChange(update) { + this._onSelectionChange.dispatch(update); } /** @private */ diff --git a/assets/js/hooks/session/cursor_history.js b/assets/js/hooks/session/cursor_history.js index 8a9f65a74627..563804120a9b 100644 --- a/assets/js/hooks/session/cursor_history.js +++ b/assets/js/hooks/session/cursor_history.js @@ -42,16 +42,19 @@ export default class CursorHistory { // We need to make sure the last entry from history // doesn't belong to the given cell id that we need // to remove from the entries list. - let currentEntryIndex = this.index; - let currentEntry = this.entries[currentEntryIndex]; + let cellIdCount = 0; - while (currentEntry.cellId === cellId) { - currentEntryIndex--; - currentEntry = this.entries[currentEntryIndex]; + for (let i = 0; i <= this.index; i++) { + const entry = this.get(i); + if (entry === null) continue; + + if (entry.cellId === cellId) cellIdCount++; } + this.index = this.index - cellIdCount; + if (this.index < 0) this.index = 0; + this.entries = this.entries.filter((entry) => entry.cellId !== cellId); - this.index = this.entries.lastIndexOf(currentEntry); } /** @@ -95,8 +98,18 @@ export default class CursorHistory { * Otherwise, returns null; */ getCurrent() { + return this.get(this.index); + } + + /** @private **/ + getEntries() { + return this.entries; + } + + /** @private **/ + get(index) { if (this.entries.length <= 0) return null; - return this.entries[this.index]; + return this.entries[index]; } /** @private **/ @@ -117,7 +130,7 @@ export default class CursorHistory { /** @private **/ isSameCell(cellId) { - const lastEntry = this.entries[this.index]; - return lastEntry !== undefined && cellId === lastEntry.cellId; + const lastEntry = this.get(this.index); + return lastEntry !== null && cellId === lastEntry.cellId; } } diff --git a/assets/test/hooks/session/cursor_history.test.js b/assets/test/hooks/session/cursor_history.test.js index 040f4f2c2d29..d620fc6a2ac1 100644 --- a/assets/test/hooks/session/cursor_history.test.js +++ b/assets/test/hooks/session/cursor_history.test.js @@ -2,7 +2,7 @@ import CursorHistory from "../../../js/hooks/session/cursor_history"; test("goBack returns when there's at least one cell", () => { const cursorHistory = new CursorHistory(); - const entry = { cellId: Math.random().toString(36), line: 1, offset: 1 }; + const entry = { cellId: "c1", line: 1, offset: 1 }; expect(cursorHistory.canGoBack()).toBe(false); expect(cursorHistory.getCurrent()).toStrictEqual(null); @@ -16,7 +16,7 @@ test("goBack returns when there's at least one cell", () => { describe("push", () => { test("does not add duplicated cells", () => { const cursorHistory = new CursorHistory(); - const entry = { cellId: Math.random().toString(36), line: 1, offset: 1 }; + const entry = { cellId: "c1", line: 1, offset: 1 }; expect(cursorHistory.canGoBack()).toBe(false); @@ -36,6 +36,7 @@ describe("push", () => { cursorHistory.push(`123${value}`, value, 1); } + const firstEntry = cursorHistory.get(0); cursorHistory.push("231", 1, 1); // Navigates to the bottom of the stack @@ -43,34 +44,32 @@ describe("push", () => { cursorHistory.goBack(); } - expect(cursorHistory.getCurrent()).toStrictEqual({ - cellId: "1232", - offset: 1, - line: 2, - }); + cursorHistory + .getEntries() + .forEach((entry) => expect(entry).not.toStrictEqual(firstEntry)); }); test("rewrites the subsequent cells if go back and saves a new cell", () => { const cursorHistory = new CursorHistory(); expect(cursorHistory.canGoBack()).toBe(false); - cursorHistory.push(Math.random().toString(36), 1, 1); - cursorHistory.push(Math.random().toString(36), 1, 1); - cursorHistory.push(Math.random().toString(36), 2, 1); + cursorHistory.push("c1", 1, 1); + cursorHistory.push("c2", 1, 1); + cursorHistory.push("c3", 2, 1); expect(cursorHistory.canGoBack()).toBe(true); - // Go back to cell id 456 + // Go back to cell id c2 cursorHistory.goBack(); expect(cursorHistory.canGoBack()).toBe(true); - // Go back to cell id 123 + // Go back to cell id c1 cursorHistory.goBack(); expect(cursorHistory.canGoBack()).toBe(false); // Removes the subsequent cells from stack // and adds this cell to the stack - cursorHistory.push(Math.random().toString(36), 1, 1); + cursorHistory.push("c4", 1, 1); expect(cursorHistory.canGoForward()).toBe(false); }); }); From 38089cc20efd949ab8c9a68fd0698c94e23fd2c1 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 25 Sep 2024 13:18:10 -0300 Subject: [PATCH 16/17] Apply review comments --- assets/js/hooks/session/cursor_history.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/assets/js/hooks/session/cursor_history.js b/assets/js/hooks/session/cursor_history.js index 563804120a9b..ae5dab32bda5 100644 --- a/assets/js/hooks/session/cursor_history.js +++ b/assets/js/hooks/session/cursor_history.js @@ -46,15 +46,12 @@ export default class CursorHistory { for (let i = 0; i <= this.index; i++) { const entry = this.get(i); - if (entry === null) continue; - if (entry.cellId === cellId) cellIdCount++; } - this.index = this.index - cellIdCount; - if (this.index < 0) this.index = 0; - this.entries = this.entries.filter((entry) => entry.cellId !== cellId); + this.index = this.index - cellIdCount; + if (this.index === -1 && this.entries.length > 0) this.index = 0; } /** From dba207af5db9ae39267891efc31bc67c35e8ee4b Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Wed, 25 Sep 2024 13:28:53 -0300 Subject: [PATCH 17/17] Apply review comments --- assets/js/hooks/cell.js | 14 ++++++-------- assets/js/hooks/cell_editor/live_editor.js | 22 ++++++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index 872306f82278..e79e46052e7b 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -207,18 +207,16 @@ const Cell = { // gives it focus if (!this.isFocused || !this.insertMode) { this.currentEditor().blur(); + } else { + this.sendCursorHistory(); } }, 0); }); - liveEditor.onSelectionChange((update) => { - // We defer the check to happen after all focus/click events have - // been processed, in case the state changes as a result - setTimeout(() => { - if (this.isFocused && !update.state.selection.eq(update.startState.selection)) { - this.sendCursorHistory(); - } - }, 0); + liveEditor.onSelectionChange(() => { + if (this.isFocused) { + this.sendCursorHistory(); + } }); if (tag === "primary") { diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index 22ebe3e2e3bd..7fc950030a47 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -333,7 +333,7 @@ export default class LiveEditor { ]; const selectionChangeListener = EditorView.updateListener.of((update) => - this.handleOnSelectionChange(update), + this.handleViewUpdate(update), ); this.view = new EditorView({ @@ -379,13 +379,13 @@ export default class LiveEditor { }), this.intellisense ? [ - autocompletion({ override: [this.completionSource.bind(this)] }), - hoverDetails(this.docsHoverTooltipSource.bind(this)), - signature(this.signatureSource.bind(this), { - activateOnTyping: settings.editor_auto_signature, - }), - formatter(this.formatterSource.bind(this)), - ] + autocompletion({ override: [this.completionSource.bind(this)] }), + hoverDetails(this.docsHoverTooltipSource.bind(this)), + signature(this.signatureSource.bind(this), { + activateOnTyping: settings.editor_auto_signature, + }), + formatter(this.formatterSource.bind(this)), + ] : [], settings.editor_mode === "vim" ? [vim()] : [], settings.editor_mode === "emacs" ? [emacs()] : [], @@ -444,8 +444,10 @@ export default class LiveEditor { } /** @private */ - handleOnSelectionChange(update) { - this._onSelectionChange.dispatch(update); + handleViewUpdate(update) { + if (!update.state.selection.eq(update.startState.selection)) { + this._onSelectionChange.dispatch(); + } } /** @private */