Rotate panes #496
Rotate panes #496
Conversation
Introduced a new "Rotate active layout" button to the interface and implemented functionality for switching the pane directions between top-bottom and right-left. Modified `window.ts` to break down `moveFocus` and added `switchPane` for future use. Adjusted `pane.ts` to handle new pane direction switching logic. Added method `changeDir` in `layout.ts` for rotating the pane orientation.
Changed the data-sc attribute for the rotate button to "⇧ R" to better indicate the keyboard shortcut. This enhances the user interface by making the shortcut more explicit and user-friendly.
Added a new `swapPanes` method in `window.ts` to swap locations of two panes. Modified key event handling in `pane.ts` to enable swapping panes when Shift is held and an arrow key is pressed. Removed debug logging and ensured dividers refresh in `layout.ts`.
Changed the rotate layout button icon from `arrow_2_circlepath` to `arrow_clockwise` for clarity. Additionally, implemented an event listener to rotate the active layout on button click, ensuring better user interaction.
WalkthroughThe pull request introduces several enhancements to the terminal application, primarily focusing on user interface improvements and functionality. A new "Rotate active layout" button is added to the navigation bar in Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for terminal7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dazzling-kringle-090742 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
index.html (1)
150-154: LGTM! Consider a minor improvement for consistency.The changes look good and align with the PR objective of introducing a rotate panes feature. The new "Rotate active layout" button is well-integrated into the existing UI.
For consistency, consider applying the same inline rotation style to the "Split Right to Left" button icon:
- <button type="button" aria-label="Split Right to Left" data-sc="'" id="divide-h" class="pane-buttons off has-help"><i class="f7-icons">divide</i></button> + <button type="button" aria-label="Split Right to Left" data-sc="'" id="divide-h" class="pane-buttons off has-help"> + <i class="f7-icons" style="display: inline-block;">divide</i> + </button>This change would make all split buttons visually consistent.
src/terminal7.ts (3)
316-321: LGTM! Consider adding error handling.The implementation of the rotate layout functionality looks good and aligns with the PR objective. It correctly checks for an active gate before attempting to rotate the layout.
Consider adding error handling to gracefully manage cases where
this.activeG,this.activeG.activeW, orthis.activeG.activeW.activePmight be undefined. For example:rotateLayout.addEventListener("click", () => { - if(this.activeG){ + if(this.activeG?.activeW?.activeP){ this.activeG.activeW.activeP.layout.changeDir() + } else { + console.warn("Unable to rotate layout: No active pane") } })
Line range hint
1022-1041: LGTM! Consider adding error handling.The implementation of the
loadChangelogmethod looks good. It effectively fetches and parses the changelog, and handles potential id conflicts and link behavior.Consider adding error handling to manage potential issues during the fetch operation. For example:
async loadChangelog() { - const resp = await fetch(changelogURL) - const changelog = await resp.text() - const e = document.getElementById("changelog-content") - e.innerHTML = marked.parse(changelog) + try { + const resp = await fetch(changelogURL) + if (!resp.ok) { + throw new Error(`HTTP error! status: ${resp.status}`) + } + const changelog = await resp.text() + const e = document.getElementById("changelog-content") + e.innerHTML = marked.parse(changelog) + // Rest of the method... + } catch (error) { + console.error("Failed to load changelog:", error) + // Optionally, update the UI to inform the user + } }This change will help gracefully handle network errors or issues with the changelog URL.
Line range hint
1-1041: Resize functionality is present, but pane deletion logic is missing.The resize functionality is implemented in
src/webrtc_session.ts, but no corresponding pane deletion logic was found across the entire codebase. This means the reported bug regarding resizing issues after deleting panes in a mixed layout is not addressed.Please implement pane deletion logic to ensure that panes are properly removed and that the layout resizes correctly afterward.
🔗 Analysis chain
Line range hint
1-1041: Changes look good, but the reported bug is not addressed.The changes to add the rotate layout functionality and improve the changelog loading are well-implemented and integrated into the existing codebase. They align with the PR objective and don't introduce any apparent conflicts.
However, the bug mentioned in the PR description about resizing issues after deleting panes in a mixed layout is not addressed in this file. To verify if this issue is handled elsewhere, we can search for relevant code:
If the bug is not addressed in other files, it should be investigated and fixed before merging this PR.
Line range hint
1-51: Resize functionality is present, but pane deletion logic needs verification.To ensure that the reported bug regarding resizing issues after deleting panes is addressed, we need to verify the pane deletion logic within
src/webrtc_session.ts.
Line range hint
1-1041: Resize functionality is present, but pane deletion logic appears to be missing.The previous search for pane deletion logic in
src/webrtc_session.tsdid not yield any results. This indicates that the reported bug regarding resizing issues after deleting panes is likely unaddressed.To ensure comprehensive verification, we need to check the entire codebase for any pane deletion implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pane deletion and resizing logic rg -n "delete.*pane|resize.*pane" --type tsLength of output: 170
Script:
#!/bin/bash # Search for pane deletion logic in webrtc_session.ts rg -n "delete.*pane" src/webrtc_session.tsLength of output: 44
Script:
#!/bin/bash # Search for pane deletion logic across the entire TypeScript codebase rg -n "delete.*pane" --type tsLength of output: 32
src/window.ts (3)
Line range hint
185-216: Handle potential null reference whenthis.activePis undefined ingetPanemethodIn the
getPanemethod,this.activePis used without checking if it isnullorundefined. If there is no active pane, accessing properties likethis.activeP.t.buffer.activewill cause a runtime error.Consider adding a null check at the beginning of the method:
+ if (!this.activeP) { + return null; + } const a = this.activeP, b = a.t.buffer.active, x = a.xoff + b.cursorX * a.sx / a.t.cols, y = a.yoff + b.cursorY * a.sy / a.t.rows;
Line range hint
192-209: Ensurematchfunction is defined to prevent runtime errorsIf the
whereparameter does not match any of the expected cases ("left","right","up","down"), thematchfunction remainsnull. Callingmatch(c)whenmatchisnullwill result in a runtime error.Consider adding a default case or validating the
whereparameter:switch(where) { case "left": // existing code break; // other cases + default: + console.warn(`Unrecognized direction: ${where}`); + return null; }Alternatively, you can check if
matchis defined before using it:// existing code this.t7.cells.forEach(c => { if ((nextPane == null) && (c instanceof Pane) && c.w && (c.w == this)) + if (match && match(c)) nextPane = c })
Line range hint
194-207: Define an epsilon constant for floating-point comparisonsUsing the magic number
0.00001for floating-point comparisons can reduce code readability. Defining it as a constant improves maintainability.+ const EPSILON = 0.00001; // You could also use Number.EPSILON if appropriate switch(where) { case "left": match = p => ((Math.abs(p.xoff + p.sx - a.xoff) < EPSILON) && (p.yoff <= y) && (p.yoff+p.sy >= y)) break // other cases }src/layout.ts (1)
373-373: Remove unnecessaryconsole.logstatements to clean up the codeThere is an active
console.logstatement at line 373 and commented-outconsole.logstatements at lines 390-395 and 407-412. Leaving debugging statements in production code can clutter the console output and may impact performance. It's good practice to remove or properly manage these statements before merging.Apply this diff to remove the unnecessary logs:
- console.log('origdir',this.dir) ... - // console.log('c',{ - // xoff: c.xoff, - // yoff: c.yoff, - // sx: c.sx, - // sy: c.sy, - // }) ... - // console.log('c2',{ - // xoff: c.xoff, - // yoff: c.yoff, - // sx: c.sx, - // sy: c.sy, - // })Also applies to: 390-395, 407-412
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- index.html (1 hunks)
- src/layout.ts (1 hunks)
- src/pane.ts (1 hunks)
- src/terminal7.ts (1 hunks)
- src/window.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/window.ts (1)
219-225: LGTM:moveFocusmethod correctly changes focus to the next paneThe
moveFocusmethod properly obtains the next pane usinggetPane(where)and shifts focus if a pane is found.
| switchPane(where){ | ||
| console.log(this.activeP) | ||
| } |
There was a problem hiding this comment.
Implement functionality for switchPane method
The switchPane method currently only logs the active pane to the console. To fulfill its intended purpose, please implement the logic to switch or rotate panes.
Consider implementing the pane-switching logic or, if this is a placeholder, add a TODO comment to indicate future work:
switchPane(where){
// TODO: Implement pane-switching logic based on the 'where' parameter
console.log(this.activeP);
}
There was a problem hiding this comment.
@daonb, happy to help! Here's a possible implementation for the switchPane method that swaps the active pane with the pane in the specified direction:
switchPane(where: string) {
const paneToSwap = this.getPane(where);
if (paneToSwap) {
this.swapPanes(this.activeP, paneToSwap);
this.gate.sendState();
} else {
console.log(`No pane found in the direction: ${where}`);
}
}😊🐰
| const pane1Xoff = pane1.xoff | ||
| const pane1Yoff = pane1.yoff | ||
| const pane1Sx = pane1.sx | ||
| const pane1Sy = pane1.sy | ||
|
|
||
| pane1.xoff = pane2.xoff | ||
| pane1.yoff = pane2.yoff | ||
| pane1.sx = pane2.sx | ||
| pane1.sy = pane2.sy | ||
|
|
||
| pane2.xoff = pane1Xoff | ||
| pane2.yoff = pane1Yoff | ||
| pane2.sx = pane1Sx | ||
| pane2.sy = pane1Sy | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor property swapping to reduce code duplication
The swapping of xoff, yoff, sx, and sy properties between pane1 and pane2 can be made more concise.
Consider simplifying the code using array destructuring:
// Swap position and size properties
- const pane1Xoff = pane1.xoff
- const pane1Yoff = pane1.yoff
- const pane1Sx = pane1.sx
- const pane1Sy = pane1.sy
-
- pane1.xoff = pane2.xoff
- pane1.yoff = pane2.yoff
- pane1.sx = pane2.sx
- pane1.sy = pane2.sy
-
- pane2.xoff = pane1Xoff
- pane2.yoff = pane1Yoff
- pane2.sx = pane1Sx
- pane2.sy = pane1Sy
+ [pane1.xoff, pane2.xoff] = [pane2.xoff, pane1.xoff];
+ [pane1.yoff, pane2.yoff] = [pane2.yoff, pane1.yoff];
+ [pane1.sx, pane2.sx] = [pane2.sx, pane1.sx];
+ [pane1.sy, pane2.sy] = [pane2.sy, pane1.sy];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pane1Xoff = pane1.xoff | |
| const pane1Yoff = pane1.yoff | |
| const pane1Sx = pane1.sx | |
| const pane1Sy = pane1.sy | |
| pane1.xoff = pane2.xoff | |
| pane1.yoff = pane2.yoff | |
| pane1.sx = pane2.sx | |
| pane1.sy = pane2.sy | |
| pane2.xoff = pane1Xoff | |
| pane2.yoff = pane1Yoff | |
| pane2.sx = pane1Sx | |
| pane2.sy = pane1Sy | |
| // Swap position and size properties | |
| [pane1.xoff, pane2.xoff] = [pane2.xoff, pane1.xoff]; | |
| [pane1.yoff, pane2.yoff] = [pane2.yoff, pane1.yoff]; | |
| [pane1.sx, pane2.sx] = [pane2.sx, pane1.sx]; | |
| [pane1.sy, pane2.sy] = [pane2.sy, pane1.sy]; |
| swapPanes(pane1: Pane, pane2: Pane){ | ||
| const pane1Layout = pane1.layout | ||
| const pane2Layout = pane2.layout | ||
| const pane1Index = pane1Layout.cells.indexOf(pane1); | ||
| const pane2Index = pane2Layout.cells.indexOf(pane2); | ||
| pane1Layout.cells[pane1Index] = pane2; | ||
| pane2Layout.cells[pane2Index] = pane1; | ||
| pane1.layout = pane2Layout; | ||
| pane2.layout = pane1Layout; | ||
|
|
||
| const pane1Xoff = pane1.xoff | ||
| const pane1Yoff = pane1.yoff | ||
| const pane1Sx = pane1.sx | ||
| const pane1Sy = pane1.sy | ||
|
|
||
| pane1.xoff = pane2.xoff | ||
| pane1.yoff = pane2.yoff | ||
| pane1.sx = pane2.sx | ||
| pane1.sy = pane2.sy | ||
|
|
||
| pane2.xoff = pane1Xoff | ||
| pane2.yoff = pane1Yoff | ||
| pane2.sx = pane1Sx | ||
| pane2.sy = pane1Sy | ||
|
|
||
| pane1Layout.refreshDividers(); | ||
| pane2Layout.refreshDividers(); | ||
| } |
There was a problem hiding this comment.
Handle swapping panes within the same layout to prevent errors
The swapPanes method may encounter issues when pane1 and pane2 are within the same layout. Swapping them without checking can lead to incorrect indexing and behavior.
Add a condition to handle cases where panes share the same layout:
swapPanes(pane1: Pane, pane2: Pane){
const pane1Layout = pane1.layout
const pane2Layout = pane2.layout
const pane1Index = pane1Layout.cells.indexOf(pane1);
const pane2Index = pane2Layout.cells.indexOf(pane2);
+ if (pane1Layout === pane2Layout) {
+ // Swap panes within the same layout
+ [pane1Layout.cells[pane1Index], pane1Layout.cells[pane2Index]] = [pane2, pane1];
+ } else {
pane1Layout.cells[pane1Index] = pane2;
pane2Layout.cells[pane2Index] = pane1;
+ }
pane1.layout = pane2Layout;
pane2.layout = pane1Layout;
// Rest of the code remains the same
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| swapPanes(pane1: Pane, pane2: Pane){ | |
| const pane1Layout = pane1.layout | |
| const pane2Layout = pane2.layout | |
| const pane1Index = pane1Layout.cells.indexOf(pane1); | |
| const pane2Index = pane2Layout.cells.indexOf(pane2); | |
| pane1Layout.cells[pane1Index] = pane2; | |
| pane2Layout.cells[pane2Index] = pane1; | |
| pane1.layout = pane2Layout; | |
| pane2.layout = pane1Layout; | |
| const pane1Xoff = pane1.xoff | |
| const pane1Yoff = pane1.yoff | |
| const pane1Sx = pane1.sx | |
| const pane1Sy = pane1.sy | |
| pane1.xoff = pane2.xoff | |
| pane1.yoff = pane2.yoff | |
| pane1.sx = pane2.sx | |
| pane1.sy = pane2.sy | |
| pane2.xoff = pane1Xoff | |
| pane2.yoff = pane1Yoff | |
| pane2.sx = pane1Sx | |
| pane2.sy = pane1Sy | |
| pane1Layout.refreshDividers(); | |
| pane2Layout.refreshDividers(); | |
| } | |
| swapPanes(pane1: Pane, pane2: Pane){ | |
| const pane1Layout = pane1.layout | |
| const pane2Layout = pane2.layout | |
| const pane1Index = pane1Layout.cells.indexOf(pane1); | |
| const pane2Index = pane2Layout.cells.indexOf(pane2); | |
| if (pane1Layout === pane2Layout) { | |
| // Swap panes within the same layout | |
| [pane1Layout.cells[pane1Index], pane1Layout.cells[pane2Index]] = [pane2, pane1]; | |
| } else { | |
| pane1Layout.cells[pane1Index] = pane2; | |
| pane2Layout.cells[pane2Index] = pane1; | |
| } | |
| pane1.layout = pane2Layout; | |
| pane2.layout = pane1Layout; | |
| const pane1Xoff = pane1.xoff | |
| const pane1Yoff = pane1.yoff | |
| const pane1Sx = pane1.sx | |
| const pane1Sy = pane1.sy | |
| pane1.xoff = pane2.xoff | |
| pane1.yoff = pane2.yoff | |
| pane1.sx = pane2.sx | |
| pane1.sy = pane2.sy | |
| pane2.xoff = pane1Xoff | |
| pane2.yoff = pane1Yoff | |
| pane2.sx = pane1Sx | |
| pane2.sy = pane1Sy | |
| pane1Layout.refreshDividers(); | |
| pane2Layout.refreshDividers(); | |
| } |
There was a problem hiding this comment.
I believe we assume both panes are of the same layout. It does make sense to check it.
There was a problem hiding this comment.
@daonb, understood. Noting that the code assumes both panes are within the same layout makes sense. Adding the check will help ensure proper handling.
(^_^)
✏️ Learnings added
Learnt from: daonb
PR: tuzig/terminal7#496
File: src/window.ts:278-305
Timestamp: 2024-10-21T15:24:31.289Z
Learning: In the `swapPanes` method in `src/window.ts`, it is assumed that both panes are within the same layout.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| changeDir(){ | ||
| if(this.dir === "TBD") return; | ||
| const parentDir = this.layout?.dir | ||
| const newDir = this.dir === 'rightleft' ? 'topbottom' : 'rightleft' | ||
| let keepXoff = false; | ||
| let keepYoff = false; | ||
| let cellSize; | ||
| console.log('origdir',this.dir) | ||
| if(this.dir === 'topbottom'){ | ||
| keepYoff = true | ||
| cellSize = this.sy / this.cells.length | ||
| }else if(this.dir === 'rightleft') { | ||
| keepXoff = true | ||
| cellSize = this.sx / this.cells.length | ||
| } | ||
| this.cells.forEach((c, i) => { | ||
| const oldXoff = c.xoff | ||
| const oldYoff = c.yoff | ||
| const oldSx = c.sx; | ||
| const oldSy = c.sy; | ||
| c.yoff = oldXoff | ||
| c.xoff = oldYoff | ||
| c.sx = oldSy | ||
| c.sy = oldSx | ||
| // console.log('c',{ | ||
| // xoff: c.xoff, | ||
| // yoff: c.yoff, | ||
| // sx: c.sx, | ||
| // sy: c.sy, | ||
| // }) | ||
| if(keepYoff){ | ||
| c.yoff = this.yoff + cellSize * i | ||
| c.xoff = this.xoff | ||
| c.sy = cellSize | ||
| c.sx = this.sx | ||
| }else if(keepXoff){ | ||
| c.yoff = this.yoff | ||
| c.xoff = this.xoff + cellSize * i | ||
| c.sy = this.sy | ||
| c.sx = cellSize | ||
| } | ||
| // console.log('c2',{ | ||
| // xoff: c.xoff, | ||
| // yoff: c.yoff, | ||
| // sx: c.sx, | ||
| // sy: c.sy, | ||
| // }) | ||
| }); | ||
| this.dir = newDir | ||
| this.refreshDividers() | ||
| if(this.dir === parentDir){ | ||
| const replaceIndex = this.layout.cells.findIndex(c => c.id === this.id) | ||
| this.layout.cells.splice(replaceIndex, 1, ...this.cells) | ||
| this.layout.refreshDividers() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recommend adding unit tests for the new changeDir method
The changeDir method introduces significant new functionality that manipulates the layout of cells. To ensure correctness and prevent regressions, it's advisable to add unit tests covering various scenarios, such as:
- Rotating layouts with different numbers of cells.
- Handling nested layouts.
- Verifying that cells retain expected properties after rotation.
| if(this.dir === parentDir){ | ||
| const replaceIndex = this.layout.cells.findIndex(c => c.id === this.id) | ||
| this.layout.cells.splice(replaceIndex, 1, ...this.cells) | ||
| this.layout.refreshDividers() | ||
| } |
There was a problem hiding this comment.
Prevent potential runtime error when this.layout is undefined
In the conditional block starting at line 416, accessing this.layout.cells without ensuring that this.layout is defined can lead to a TypeError if this.layout is undefined. This can occur when the current layout has no parent layout. To prevent this, add a null check for this.layout before accessing its properties.
Apply this diff to add a null check:
this.dir = newDir
this.refreshDividers()
- if(this.dir === parentDir){
+ if(this.dir === parentDir && this.layout){
const replaceIndex = this.layout.cells.findIndex(c => c.id === this.id)
this.layout.cells.splice(replaceIndex, 1, ...this.cells)
this.layout.refreshDividers()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(this.dir === parentDir){ | |
| const replaceIndex = this.layout.cells.findIndex(c => c.id === this.id) | |
| this.layout.cells.splice(replaceIndex, 1, ...this.cells) | |
| this.layout.refreshDividers() | |
| } | |
| if(this.dir === parentDir && this.layout){ | |
| const replaceIndex = this.layout.cells.findIndex(c => c.id === this.id) | |
| this.layout.cells.splice(replaceIndex, 1, ...this.cells) | |
| this.layout.refreshDividers() | |
| } |
| if(ev.shiftKey){ | ||
| this.layout.cells.forEach(c => console.log(c)) | ||
| f = () => this.layout.changeDir() |
There was a problem hiding this comment.
Remove debugging console.log statement in Shift+R handler
The console.log statement inside the Shift+R handler appears to be leftover debugging code. Removing it will clean up the console output in production.
Apply this diff to remove the unnecessary console.log call:
if (ev.shiftKey) {
- this.layout.cells.forEach(c => console.log(c));
f = () => this.layout.changeDir();
} else {
f = () => this.gate.reset();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ev.shiftKey){ | |
| this.layout.cells.forEach(c => console.log(c)) | |
| f = () => this.layout.changeDir() | |
| if(ev.shiftKey){ | |
| f = () => this.layout.changeDir() |
| if(ev.shiftKey){ | ||
| const pane = this.w.getPane("left") | ||
| if(!pane) return; | ||
| this.w.swapPanes(this,pane) | ||
| }else { | ||
| f = () => this.w.moveFocus("left") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback when no pane exists to swap with on Shift+ArrowLeft
When Shift+ArrowLeft is pressed and there is no pane to the left, the function returns silently. Consider notifying the user to improve the user experience.
Apply this diff to inform the user when no adjacent pane is available:
if (ev.shiftKey) {
const pane = this.w.getPane("left");
if (!pane) {
+ this.gate.notify("No pane to the left to swap with.");
return;
}
this.w.swapPanes(this, pane);
} else {
f = () => this.w.moveFocus("left");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("left") | |
| if(!pane) return; | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("left") | |
| } | |
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("left") | |
| if(!pane) { | |
| this.gate.notify("No pane to the left to swap with."); | |
| return; | |
| } | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("left") | |
| } |
| if(ev.shiftKey){ | ||
| const pane = this.w.getPane("down") | ||
| if(!pane) return; | ||
| this.w.swapPanes(this,pane) | ||
| }else { | ||
| f = () => this.w.moveFocus("down") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback when no pane exists to swap with on Shift+ArrowDown
When swapping panes downward, inform the user if there's no pane below.
Apply this diff:
if (ev.shiftKey) {
const pane = this.w.getPane("down");
if (!pane) {
+ this.gate.notify("No pane below to swap with.");
return;
}
this.w.swapPanes(this, pane);
} else {
f = () => this.w.moveFocus("down");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("down") | |
| if(!pane) return; | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("down") | |
| } | |
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("down") | |
| if(!pane) { | |
| this.gate.notify("No pane below to swap with."); | |
| return; | |
| } | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("down") | |
| } |
| if(ev.shiftKey){ | ||
| const pane = this.w.getPane("right") | ||
| if(!pane) return; | ||
| this.w.swapPanes(this,pane) | ||
| }else { | ||
| f = () => this.w.moveFocus("right") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback when no pane exists to swap with on Shift+ArrowRight
Similar to the left direction, inform the user when there's no pane to the right to swap with.
Apply this diff:
if (ev.shiftKey) {
const pane = this.w.getPane("right");
if (!pane) {
+ this.gate.notify("No pane to the right to swap with.");
return;
}
this.w.swapPanes(this, pane);
} else {
f = () => this.w.moveFocus("right");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("right") | |
| if(!pane) return; | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("right") | |
| } | |
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("right") | |
| if(!pane) { | |
| this.gate.notify("No pane to the right to swap with."); | |
| return; | |
| } | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("right") | |
| } |
| if(ev.shiftKey){ | ||
| const pane = this.w.getPane("up") | ||
| if(!pane) return; | ||
| this.w.swapPanes(this,pane) | ||
| }else { | ||
| f = () => this.w.moveFocus("up") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback when no pane exists to swap with on Shift+ArrowUp
When swapping panes upward, notify the user if no pane is available above.
Apply this diff:
if (ev.shiftKey) {
const pane = this.w.getPane("up");
if (!pane) {
+ this.gate.notify("No pane above to swap with.");
return;
}
this.w.swapPanes(this, pane);
} else {
f = () => this.w.moveFocus("up");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("up") | |
| if(!pane) return; | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("up") | |
| } | |
| if(ev.shiftKey){ | |
| const pane = this.w.getPane("up") | |
| if(!pane) { | |
| this.gate.notify("No pane above to swap with."); | |
| return; | |
| } | |
| this.w.swapPanes(this,pane) | |
| }else { | |
| f = () => this.w.moveFocus("up") | |
| } |
Added rotate panes functionality.
one bug I can't find the source of yet, after rotating a mixed layout:
|-----1------|
|--2--|--3--|
after deleting the middle pane, and than the bottom pane, the top pane won't resize.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor