Merge pull request #9236 from gitbutlerapp/fix-workspace-reordering

Rewrote stack reordering to use simpler (and not broken) logic

Author: Caleb-T-Owens

apps/desktop/src/components/v3/MultiStackView.svelte

@@ -4,8 +4,12 @@
 	import MultiStackPagination, { scrollToLane } from '$components/v3/MultiStackPagination.svelte';
 	import StackDraft from '$components/v3/StackDraft.svelte';
 	import StackView from '$components/v3/StackView.svelte';
-	import { onReorderEnd, onReorderMouseDown, onReorderStart } from '$lib/dragging/reordering';
-	import { onReorderDragOver } from '$lib/dragging/reordering';
+	import {
+		onReorderEnd,
+		onReorderMouseDown,
+		onReorderStart,
+		onDragOver
+	} from '$lib/dragging/reordering';
 	import { IntelligentScrollingService } from '$lib/intelligentScrolling/service';
 	import { branchesPath } from '$lib/routes/routes.svelte';
 	import { type SelectionId } from '$lib/selection/key';
@@ -117,7 +121,7 @@
 	});
 
 	// Throttle calls to the reordering code in order to save some cpu cycles.
-	const throttledDragOver = throttle(onReorderDragOver, 250);
+	const throttledDragOver = throttle(onDragOver, 25);
 
 	// To support visual reordering of stacks we need a copy of the array
 	// that can be mutated as the stack is being dragged around.
@@ -130,8 +134,6 @@
 			mutableStacks = stacks;
 		}
 	});
-
-	let viewWrapperEl: HTMLDivElement | undefined;
 </script>
 
 {#if isNotEnoughHorzSpace}
@@ -154,18 +156,12 @@
 <div
 	role="presentation"
 	class="lanes-scrollable hide-native-scrollbar"
-	bind:this={viewWrapperEl}
 	class:panning={isPanning}
 	bind:this={lanesScrollableEl}
 	bind:clientWidth={lanesScrollableWidth}
 	bind:clientHeight={lanesScrollableHeight}
 	class:multi={stacks.length < SHOW_PAGINATION_THRESHOLD}
 	onmousedown={handleMouseDown}
-	ondragover={(e) => {
-		// This call will actually mutate the array of stacks, showing
-		// where the lane will be placed when dropped.
-		throttledDragOver(e, mutableStacks);
-	}}
 	ondrop={() => {
 		stackService.updateStackOrder({
 			projectId,
@@ -189,12 +185,13 @@
 			class="reorderable-stack"
 			role="presentation"
 			animate:flip={{ duration: 150 }}
-			onmousedown={(e) => onReorderMouseDown(e, viewWrapperEl as HTMLDivElement)}
+			onmousedown={onReorderMouseDown}
 			ondragstart={(e) =>
 				onReorderStart(e, stack.id, () => {
 					selection.set(undefined);
 					intelligentScrollingService.show(projectId, stack.id, 'stack');
 				})}
+			ondragover={(e) => throttledDragOver(e, mutableStacks, stack.id)}
 			ondragend={onReorderEnd}
 		>
 			<StackView

apps/desktop/src/components/v3/StackView.svelte

@@ -184,6 +184,8 @@
 	}}
 	<div
 		bind:this={viewWrapperEl}
+		bind:clientWidth
+		bind:clientHeight
 		class="stack-view-wrapper"
 		role="presentation"
 		class:dimmed
@@ -210,8 +212,6 @@
 		<div
 			class="stack-view"
 			style:width={$persistedStackWidth + 'rem'}
-			bind:clientWidth
-			bind:clientHeight
 			bind:this={stackViewEl}
 			{@attach scrollingAttachment(intelligentScrollingService, projectId, stack.id, 'stack')}
 		>

apps/desktop/src/lib/dragging/reordering.ts

@@ -28,11 +28,9 @@ let dragHandle: any;
 let dragged: HTMLDivElement | undefined;
 let clone: any;
 let draggedId: string | undefined;
-let lanesScrollableEl: HTMLElement | undefined;
 
-export function onReorderMouseDown(e: MouseEvent, element: HTMLDivElement | undefined) {
+export function onReorderMouseDown(e: MouseEvent) {
 	dragHandle = e.target;
-	lanesScrollableEl = element;
 }
 
 export function onReorderStart(
@@ -81,43 +79,45 @@ export function onReorderEnd() {
 	clone?.remove();
 }
 
-export function onReorderDragOver(
+export function onDragOver(
 	e: MouseEvent & { currentTarget: HTMLDivElement },
-	sortedStacks: Stack[]
+	sortedStacks: Stack[],
+	thisStackId: string
 ) {
-	e.preventDefault();
-	if (!dragged) {
-		return; // Something other than a lane is being dragged.
+	// Return early if we are currently dragging over ourself.
+	if (draggedId === thisStackId) {
+		return;
+	}
+
+	const thisIdx = sortedStacks.findIndex((stack) => stack.id === thisStackId);
+	const draggedIdx = sortedStacks.findIndex((stack) => stack.id === draggedId);
+	if (draggedIdx === -1 || thisIdx === -1) {
+		return;
 	}
 
-	const children = Array.from(e.currentTarget.children);
-	const currentPosition = sortedStacks.findIndex((stack) => stack.id === draggedId);
+	// If we are dragging over an adjacent stack, only swap if the mouse is half
+	// way over the adjacent stack.
+	if (Math.abs(thisIdx - draggedIdx) === 1) {
+		// The mouse position relative to the LHS of the current stack.
+		const mouseLeft = e.clientX - (e.currentTarget?.getBoundingClientRect().left ?? 0);
 
-	let dropPosition = 0;
-	const mouseLeft = e.clientX - (lanesScrollableEl?.getBoundingClientRect().left ?? 0);
-	let cumulativeWidth = lanesScrollableEl?.offsetLeft ?? 0;
+		const isRightOfTarget = thisIdx > draggedIdx;
 
-	for (let i = 0; i < children.length; i++) {
-		const childWidth = (children[i] as HTMLElement).offsetWidth;
-		// The commented out code below is necessary if the drag handle is
-		// aligned with the left side of the stack. Leaving it here until
-		// we are more certain about the layout.
-		// if (i === currentPosition) {
-		// 	continue;
-		// }
-		if (mouseLeft > cumulativeWidth + childWidth / 2) {
-			// New position depends on drag direction.
-			dropPosition = i < currentPosition ? i + 1 : i;
-			cumulativeWidth += childWidth;
+		const midpoint = (e.currentTarget?.clientWidth ?? 0) / 2;
+
+		let pastOfMidpoint = false;
+		if (isRightOfTarget) {
+			pastOfMidpoint = mouseLeft > midpoint;
 		} else {
-			break;
+			pastOfMidpoint = mouseLeft < midpoint;
 		}
-	}
 
-	// Update sorted branch array manually.
-	if (currentPosition !== dropPosition) {
-		const el = sortedStacks.splice(currentPosition, 1);
-		sortedStacks.splice(dropPosition, 0, ...el);
+		if (pastOfMidpoint) {
+			const draggedStack = sortedStacks.splice(draggedIdx, 1);
+			sortedStacks.splice(thisIdx, 0, ...draggedStack);
+		}
+	} else {
+		const draggedStack = sortedStacks.splice(draggedIdx, 1);
+		sortedStacks.splice(thisIdx, 0, ...draggedStack);
 	}
-	return sortedStacks;
 }