Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix vertical splitter in variables pane #5391

Merged
merged 4 commits into from
Nov 22, 2024
+124 −139
Merged

Fix vertical splitter in variables pane #5391

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46b41c0
Fix vertical splitter resize behavior
dhruvisompura Nov 15, 2024
549e29d
Fix expand and collapse behavior
dhruvisompura Nov 16, 2024
cb6b2e6
Rename pointer down handler for code consistency
dhruvisompura Nov 19, 2024
bdc38df
Assign expression to a variable for readability
dhruvisompura Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
263 changes: 124 additions & 139 deletions src/vs/base/browser/ui/positronComponents/splitters/verticalSplitter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import 'vs/css!./verticalSplitter';

// React.
import * as React from 'react';
import { PointerEvent, useEffect, useRef, useState } from 'react'; // eslint-disable-line no-duplicate-imports
import { useEffect, useRef, useState } from 'react'; // eslint-disable-line no-duplicate-imports

// Other dependencies.
import * as DOM from 'vs/base/browser/dom';
import { Delayer } from 'vs/base/common/async';
import { isMacintosh } from 'vs/base/common/platform';
import { DisposableStore } from 'vs/base/common/lifecycle';
import { useStateRef } from 'vs/base/browser/ui/react/useStateRef';
import { positronClassNames } from 'vs/base/common/positronUtilities';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { Button, KeyboardModifiers, MouseTrigger } from 'vs/base/browser/ui/positronComponents/button/button';
Expand Down Expand Up @@ -53,27 +54,14 @@ type VerticalSplitterCollapseProps = | {
type VerticalSplitterProps = VerticalSplitterBaseProps & VerticalSplitterCollapseProps;

/**
* VerticalSplitterResizeParams interface. This defines the parameters of a resize operation. When
* invert is true, the mouse delta is subtracted from the starting width instead of being added to
* it, which inverts the resize operation.
* VerticalSplitterResizeParams interface. This defines the parameters of a resize operation.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the comment here since it looked to be incorrect.

*/
export interface VerticalSplitterResizeParams {
readonly minimumWidth: number;
readonly maximumWidth: number;
readonly columnsWidth: number;
}

/**
* ResizeParams interface. Represents a resize that's in progress.
*/
interface ResizeParams {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer require this interface because the react state variable for the resize params is no longer needed for the new approach to handle resizing.

readonly minimumWidth: number;
readonly maximumWidth: number;
readonly startingWidth: number;
readonly clientX: number;
readonly styleSheet: HTMLStyleElement;
}

/**
* Gets the sash size.
* @param configurationService The configuration service.
Expand Down Expand Up @@ -166,8 +154,8 @@ export const VerticalSplitter = ({
const [hovering, setHovering] = useState(false);
const [highlightExpandCollapse, setHighlightExpandCollapse] = useState(false);
const [hoveringDelayer, setHoveringDelayer] = useState<Delayer<void>>(undefined!);
const [collapsed, setCollapsed] = useState(false);
const [resizeParams, setResizeParams] = useState<ResizeParams | undefined>(undefined);
const [collapsed, setCollapsed, collapsedRef] = useStateRef(false);
const [resizing, setResizing] = useState(false);

// Main useEffect.
useEffect(() => {
Expand Down Expand Up @@ -205,7 +193,7 @@ export const VerticalSplitter = ({
* Sash onPointerEnter handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const sashPointerEnterHandler = (e: PointerEvent<HTMLDivElement>) => {
const sashPointerEnterHandler = (e: React.PointerEvent<HTMLDivElement>) => {
hoveringDelayer.trigger(() => {
setHovering(true);
const rect = sashRef.current.getBoundingClientRect();
Expand All @@ -220,130 +208,18 @@ export const VerticalSplitter = ({
* Sash onPointerLeave handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const sashPointerLeaveHandler = (e: PointerEvent<HTMLDivElement>) => {
const sashPointerLeaveHandler = (e: React.PointerEvent<HTMLDivElement>) => {
// When not resizing, trigger the delayer.
if (!resizeParams) {
if (!resizing) {
hoveringDelayer.trigger(() => setHovering(false), hoverDelay);
}
};

/**
* Sash onPointerDown handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const sashPointerDownHandler = (e: PointerEvent<HTMLDivElement>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function has been replaced by pointerDownHandler - most of the logic is the same and just been moved over. I may end up renaming pointerDownHandler to sashPointerDownHandler if that's the preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename pointerDownHandler to sashPointerDownHandler for consistency.

// Ignore events we don't process.
if (e.pointerType === 'mouse' && e.buttons !== 1) {
return;
}

// Determine whether the event occurred inside the expand / collapse button. If it did,
// don't process the event.
if (isPointInsideElement(e.clientX, e.clientY, expandCollapseButtonRef.current)) {
return;
}

// Consume the event.
e.preventDefault();
e.stopPropagation();

// Begin resize to obtain the resize parameters.
const { minimumWidth, maximumWidth, columnsWidth } = onBeginResize();

// Set the resize params.
setResizeParams({
minimumWidth,
maximumWidth,
startingWidth: collapsed ? sashWidth : columnsWidth,
clientX: e.clientX,
styleSheet: DOM.createStyleSheet(sashRef.current)
});

// Set pointer capture.
sashRef.current.setPointerCapture(e.pointerId);
};

/**
* Sash onPointerDown handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const sashPointerEventHandler = (e: PointerEvent<HTMLDivElement>) => {
// Ignore events we do not process.
if (!resizeParams) {
return;
}

// Consume the event.
e.preventDefault();
e.stopPropagation();

// Determine whether to end the resize operation.
const endResize = e.type === 'lostpointercapture';

// Calculate the delta.
const delta = Math.trunc(e.clientX - resizeParams.clientX);

// Calculate the new width.
let newWidth = !invert ?
resizeParams.startingWidth + delta :
resizeParams.startingWidth - delta;

// Setup event processing state.
let newCollapsed;
let newCursor: string | undefined = undefined;
if (newWidth < resizeParams.minimumWidth / 2) {
newWidth = resizeParams.minimumWidth;
newCollapsed = true;
if (!endResize) {
newCursor = isMacintosh ? 'col-resize' : 'ew-resize';
}
} else if (newWidth < resizeParams!.minimumWidth) {
newWidth = resizeParams.minimumWidth;
newCollapsed = false;
if (!endResize) {
newCursor = !invert ? 'e-resize' : 'w-resize';
}
} else if (newWidth > resizeParams!.maximumWidth) {
newWidth = resizeParams.maximumWidth;
newCollapsed = false;
if (!endResize) {
newCursor = !invert ? 'w-resize' : 'e-resize';
}
} else {
newCollapsed = false;
if (!endResize) {
newCursor = isMacintosh ? 'col-resize' : 'ew-resize';
}
}

// Set the cursor.
if (newCursor) {
resizeParams!.styleSheet.textContent = `* { cursor: ${newCursor} !important; }`;
}

// Perform the resize.
onResize(newWidth);

// Set the collapsed state.
if (newCollapsed !== collapsed) {
setCollapsed(newCollapsed);
onCollapsedChanged?.(newCollapsed);
}

// End the resize.
if (endResize) {
sashRef.current.removeChild(resizeParams.styleSheet);
hoveringDelayer.cancel();
setResizeParams(undefined);
setHovering(isPointInsideElement(e.clientX, e.clientY, sashRef.current));
}
};

/**
* Expand / collapse button onPointerEnter handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const expandCollapseButtonPointerEnterHandler = (e: PointerEvent<HTMLDivElement>) => {
const expandCollapseButtonPointerEnterHandler = (e: React.PointerEvent<HTMLDivElement>) => {
hoveringDelayer.cancel();
setHovering(true);
setHighlightExpandCollapse(true);
Expand All @@ -353,7 +229,7 @@ export const VerticalSplitter = ({
* Expand / collapse button onPointerLeave handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const expandCollapseButtonPointerLeaveHandler = (e: PointerEvent<HTMLDivElement>) => {
const expandCollapseButtonPointerLeaveHandler = (e: React.PointerEvent<HTMLDivElement>) => {
hoveringDelayer.trigger(() => setHovering(false), hoverDelay);
setHighlightExpandCollapse(false);
};
Expand All @@ -376,6 +252,117 @@ export const VerticalSplitter = ({
setHighlightExpandCollapse(false);
};

/**
* pointerDown handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const sashPointerDownHandler = (e: React.PointerEvent<HTMLDivElement>) => {
// Ignore events we don't process.
const isNonLeftMouseClick = e.pointerType === 'mouse' && e.buttons !== 1;
if (isNonLeftMouseClick) {
return;
}

// Determine whether the event occurred inside the expand / collapse button. If it did,
// don't process the event.
if (isPointInsideElement(e.clientX, e.clientY, expandCollapseButtonRef.current)) {
return;
}

// Consume the event.
e.preventDefault();
e.stopPropagation();

// Setup the resize state.
const resizeParams = onBeginResize();
const startingWidth = collapsed ? sashWidth : resizeParams.columnsWidth;
const target = DOM.getWindow(e.currentTarget).document.body;
Copy link
Contributor

Choose a reason for hiding this comment

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

My first inclination is that the whole window body is too broad, but you're adding the events in a way that they don't clobber anything else listening... so this seems good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, great point! 😄 I had the same exact inclination initially as well and I chatted about this exact thing with @softwarenerd. My understanding is that we need to know when a user does a drag and a release that ends outside of the application window. I'm a little new to handling drag events and followed the existing pattern we have in <HorizontalSplitter>.

const clientX = e.clientX;
const styleSheet = DOM.createStyleSheet(target);

/**
* pointermove event handler.
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const pointerMoveHandler = (e: PointerEvent) => {
// Consume the event.
e.preventDefault();
e.stopPropagation();

// Calculate the delta.
const delta = Math.trunc(e.clientX - clientX);

// Calculate the new width.
let newWidth = !invert ?
startingWidth + delta :
startingWidth - delta;

// Adjust the new width to be within limits and set cursor and collapsed state accordingly
let newCollapsed = false;
let cursor: string | undefined = undefined;
if (newWidth < resizeParams.minimumWidth / 2) {
newWidth = resizeParams.minimumWidth;
newCollapsed = true;
cursor = isMacintosh ? 'col-resize' : 'ew-resize';
} else if (newWidth < resizeParams.minimumWidth) {
newWidth = resizeParams.minimumWidth;
newCollapsed = false;
cursor = !invert ? 'e-resize' : 'w-resize';
} else if (newWidth > resizeParams.maximumWidth) {
newWidth = resizeParams.maximumWidth;
newCollapsed = false;
cursor = !invert ? 'w-resize' : 'e-resize';
} else {
newCollapsed = false;
cursor = isMacintosh ? 'col-resize' : 'ew-resize';
}

// Set the cursor.
if (cursor) {
styleSheet.textContent = `* { cursor: ${cursor} !important; }`;
}

// Perform the resize
onResize(newWidth);

// Set the collapsed state.
if (newCollapsed !== collapsedRef.current) {
setCollapsed(newCollapsed);
onCollapsedChanged?.(newCollapsed);
}
};

/**
* lostpointercapture event handler
* @param e A PointerEvent that describes a user interaction with the pointer.
*/
const lostPointerCaptureHandler = (e: PointerEvent) => {
// Handle the last possible move change.
pointerMoveHandler(e);

// Remove our pointer event handlers.
target.removeEventListener('pointermove', pointerMoveHandler);
target.removeEventListener('lostpointercapture', lostPointerCaptureHandler);

// Remove the style sheet.
target.removeChild(styleSheet);

// Clear the resizing flag.
setResizing(false);
hoveringDelayer.cancel();
setHovering(isPointInsideElement(e.clientX, e.clientY, sashRef.current));
};

// Set the dragging flag
setResizing(true);

// Set the capture target of future pointer events to be the current target and add our
// pointer event handlers.
target.setPointerCapture(e.pointerId);
target.addEventListener('pointermove', pointerMoveHandler);
target.addEventListener('lostpointercapture', lostPointerCaptureHandler);
Comment on lines +361 to +363
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the secret sauce that fixed the bug. This event listener has been moved to the window body. When the event listener was on the component, a re-render of the component meant that we stopped listening to the original event that was happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly, this relies on the lostpointercapture event to fire. I guess there may be some convoluted situation where that doesn't happen like using a keyboard shortcut to toggle the sidebar while actively dragging the sash. Testing this though the callback for lost pointer will still eventually fire and clean stuff up. So I think it's fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's a great edge case that you caught! I hadn't tested that so its good to know it works 😅!

};

// Render.
return (
<div
Expand All @@ -397,23 +384,21 @@ export const VerticalSplitter = ({
onPointerEnter={sashPointerEnterHandler}
onPointerLeave={sashPointerLeaveHandler}
onPointerDown={sashPointerDownHandler}
onPointerMove={sashPointerEventHandler}
onLostPointerCapture={sashPointerEventHandler}
>
{showSash && (hovering || resizeParams) &&
{showSash && (hovering || resizing) &&
<div
className={positronClassNames(
'sash-indicator',
{ 'hovering': showSash && hovering },
{ 'resizing': showSash && resizeParams },
{ 'resizing': showSash && resizing },
)}
style={{
width: sashIndicatorWidth,
}}
/>
}
</div>
{collapsible && (hovering || resizeParams || collapsed) &&
{collapsible && (hovering || resizing || collapsed) &&
<Button
ref={expandCollapseButtonRef}
className='expand-collapse-button'
Expand Down