-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Restore auto-resizing of the code console input prompts #17819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e7ad275
d0970dc
95bf9ad
9b4d5ba
148d747
df34e8c
6a4c0dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -134,6 +134,11 @@ export class CodeConsole extends Widget { | |||||
|
|
||||||
| layout.addWidget(this._splitPanel); | ||||||
|
|
||||||
| // Listen for manual split panel resizing | ||||||
| this._splitPanel.handleMoved.connect(() => { | ||||||
| this._hasManualResize = true; | ||||||
| }, this); | ||||||
|
|
||||||
| // initialize the console with defaults | ||||||
| this.setConfig({ | ||||||
| clearCellsOnExecute: false, | ||||||
|
|
@@ -317,6 +322,16 @@ export class CodeConsole extends Widget { | |||||
| if (this.isDisposed) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| // Clean up ResizeObserver from the current prompt cell | ||||||
| const promptCell = this.promptCell; | ||||||
| if (promptCell) { | ||||||
| if (this._promptResizeObserver) { | ||||||
| this._promptResizeObserver.disconnect(); | ||||||
| this._promptResizeObserver = null; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| this._msgIdCells = null!; | ||||||
| this._msgIds = null!; | ||||||
| this._history.dispose(); | ||||||
|
|
@@ -675,9 +690,14 @@ export class CodeConsole extends Widget { | |||||
| // the `readOnly` configuration gets updated before editor signals | ||||||
| // get disconnected (see `Cell.onUpdateRequest`). | ||||||
| const oldCell = promptCell; | ||||||
| const promptResizeObserver = this._promptResizeObserver; | ||||||
| requestIdleCallback(() => { | ||||||
| // Clear the signals to avoid memory leaks | ||||||
| Signal.clearData(oldCell.editor); | ||||||
|
|
||||||
| if (promptResizeObserver) { | ||||||
| promptResizeObserver.disconnect(); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| // Ensure to clear the cursor | ||||||
|
|
@@ -700,8 +720,24 @@ export class CodeConsole extends Widget { | |||||
| // Add the prompt cell to the DOM, making `this.promptCell` valid again. | ||||||
| this._input.addWidget(promptCell); | ||||||
|
|
||||||
| // Reset input size to default (unless the split has manually been resized) | ||||||
| this._resetInputSize(); | ||||||
|
|
||||||
| this._history.editor = promptCell.editor; | ||||||
|
|
||||||
| // Detect height changes | ||||||
| if (promptCell.node) { | ||||||
| this._promptResizeObserver = new ResizeObserver(() => { | ||||||
| if (!this._hasManualResize) { | ||||||
| this._resetInputSize(); | ||||||
| } | ||||||
| requestAnimationFrame(() => { | ||||||
| this._adjustSplitPanelForInputGrowth(); | ||||||
| }); | ||||||
|
Comment on lines
+734
to
+736
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to delay this until next frame? I am not sure if this is the exact source of the issue, but it looks like each time I press enter a really visual jitter is present jitter.webm
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wow, I haven't experienced such jitter locally. Which browser is it? iirc the delay was needed to be able to pick up the correct size. But I can try to take another look at it to double check.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's Chrome.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried it on Binder, and I’m noticing the same jitter on Brave browser as well.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I could reproduce locally with Chrome 141: jupyterlab-console-jitter.mp4 |
||||||
| }); | ||||||
| this._promptResizeObserver.observe(promptCell.node); | ||||||
| } | ||||||
|
|
||||||
|
Comment on lines
+728
to
+740
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about any performance considerations or what the previous implementation was like, but would this cause a lot of redraws unless we debounce it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the previous implementation was just using The main idea is that we need to catch any change to the height of the prompt cell, which happens most of the time when a user adds a new line. |
||||||
| if (!this._config.clearCodeContentOnExecute) { | ||||||
| promptCell.model.sharedModel.setSource(previousContent); | ||||||
| if (previousCursorPosition) { | ||||||
|
|
@@ -943,12 +979,95 @@ export class CodeConsole extends Widget { | |||||
| this.node.classList.toggle(READ_WRITE_CLASS, inReadWrite); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Calculate relative sizes for split panel based on prompt cell position. | ||||||
| */ | ||||||
| private _calculateRelativeSizes(): number[] { | ||||||
| const { promptCellPosition = 'bottom' } = this._config; | ||||||
|
|
||||||
| let sizes = [1, 1]; | ||||||
| if (promptCellPosition === 'top') { | ||||||
| sizes = [1, 100]; | ||||||
| } else if (promptCellPosition === 'bottom') { | ||||||
| sizes = [100, 1]; | ||||||
| } | ||||||
| return sizes; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Reset input area size to default when new prompt is created. | ||||||
| */ | ||||||
| private _resetInputSize(): void { | ||||||
| if (this._hasManualResize) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const { promptCellPosition = 'bottom' } = this._config; | ||||||
|
|
||||||
| // Only reset for vertical layouts (top/bottom positions) | ||||||
| if (promptCellPosition === 'left' || promptCellPosition === 'right') { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| this._splitPanel.setRelativeSizes(this._calculateRelativeSizes()); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Adjust split panel sizes when the input cell grows or shrinks. | ||||||
| */ | ||||||
| private _adjustSplitPanelForInputGrowth(): void { | ||||||
| if (!this._input.node || !this._content.node || this._hasManualResize) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const { promptCellPosition = 'bottom' } = this._config; | ||||||
|
|
||||||
| // Only adjust for vertical layouts (top/bottom positions) | ||||||
| if (promptCellPosition === 'left' || promptCellPosition === 'right') { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const inputHeight = this._input.node.scrollHeight; | ||||||
| const totalHeight = this._splitPanel.node.clientHeight; | ||||||
|
|
||||||
| if (totalHeight <= 0 || inputHeight <= 0) { | ||||||
| this._splitPanel.fit(); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const remainingHeight = totalHeight - inputHeight; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, what would happen if If yes, we could also consider if we want to refactor this math into a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normally it should not be possible for |
||||||
| let contentRatio: number; | ||||||
| let inputRatio: number; | ||||||
|
|
||||||
| if (promptCellPosition === 'bottom') { | ||||||
| contentRatio = remainingHeight / totalHeight; | ||||||
| inputRatio = inputHeight / totalHeight; | ||||||
| } else { | ||||||
| inputRatio = inputHeight / totalHeight; | ||||||
| contentRatio = remainingHeight / totalHeight; | ||||||
| } | ||||||
|
|
||||||
| // Convert to the format expected by setRelativeSizes | ||||||
| const totalRatio = contentRatio + inputRatio; | ||||||
| if (totalRatio > 0) { | ||||||
| const normalizedSizes = | ||||||
| promptCellPosition === 'bottom' | ||||||
| ? [contentRatio / totalRatio, inputRatio / totalRatio] | ||||||
| : [inputRatio / totalRatio, contentRatio / totalRatio]; | ||||||
|
|
||||||
| this._splitPanel.setRelativeSizes(normalizedSizes); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Update the layout of the code console. | ||||||
| */ | ||||||
| private _updateLayout(): void { | ||||||
| const { promptCellPosition = 'bottom' } = this._config; | ||||||
|
|
||||||
| // Reset manual resize flag when layout changes | ||||||
| this._hasManualResize = false; | ||||||
|
|
||||||
| this._splitPanel.orientation = ['left', 'right'].includes( | ||||||
| promptCellPosition | ||||||
| ) | ||||||
|
|
@@ -967,14 +1086,12 @@ export class CodeConsole extends Widget { | |||||
| this._splitPanel.insertWidget(1, this._content); | ||||||
| } | ||||||
|
|
||||||
| // Default relative sizes | ||||||
| let sizes = [1, 1]; | ||||||
| if (promptCellPosition === 'top') { | ||||||
| sizes = [1, 100]; | ||||||
| } else if (promptCellPosition === 'bottom') { | ||||||
| sizes = [100, 1]; | ||||||
| } | ||||||
| this._splitPanel.setRelativeSizes(sizes); | ||||||
| this._splitPanel.setRelativeSizes(this._calculateRelativeSizes()); | ||||||
|
|
||||||
| requestAnimationFrame(() => { | ||||||
| // adjust the sizes if the prompt cell is moved with code in it | ||||||
| this._adjustSplitPanelForInputGrowth(); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| private _banner: RawCell | null = null; | ||||||
|
|
@@ -999,6 +1116,8 @@ export class CodeConsole extends Widget { | |||||
| private _focusedCell: Cell | null = null; | ||||||
| private _translator: ITranslator; | ||||||
| private _splitPanel: SplitPanel; | ||||||
| private _promptResizeObserver: ResizeObserver | null = null; | ||||||
| private _hasManualResize = false; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ this is not needed as typescript can infer this type |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is flaky as per GitHub annotations?