Skip to content

Conversation

@cmdcolin
Copy link
Collaborator

Alternate approach to #5237

Instead of calculateDynamicBlocks derive from calculateStaticBlocks, which involves having to undo various things that static blocks does (e.g. the calculateDynamicBlocks in #5237 has to 'merge blocks' for example) this PR instead approaches the problem by making static blocks and dynamic blocks share some helper functions that makes them more synchronized.

This PR also adds tests to ensure that the offsetPx values are synchronized in some of the scenarios that were shown in #5237

@garrettjstevens
Copy link
Collaborator

It makes sense to not have the dynamic blocks "undo" some of the static blocks calculations.

I'm getting some mismatching static and dynamic blocks in this branch, though. Here's a currently failing test case:

diff --git a/packages/core/util/calculateDynamicBlocks.test.ts b/packages/core/util/calculateDynamicBlocks.test.ts
index 47c9e71528..9404f13d5a 100644
--- a/packages/core/util/calculateDynamicBlocks.test.ts
+++ b/packages/core/util/calculateDynamicBlocks.test.ts
@@ -176,6 +176,33 @@ describe('dynamic and static blocks have matching offsets for full regions', ()
     expect(dynamicBlock4000?.offsetPx).toEqual(1004)
     expect(staticBlock4000?.offsetPx).toEqual(1004)
   })
+
+  it('when multiple complete regions are offscreen to the left', () => {
+    const view = {
+      width: 980,
+      displayedRegions: [
+        { ...ctgA, start: 0, end: 1000 },
+        { ...ctgA, start: 2000, end: 3000 },
+        { ...ctgA, start: 4000, end: 5000 },
+        { ...ctgA, start: 6000, end: 7000 },
+      ],
+      minimumBlockWidth: 3,
+      interRegionPaddingWidth: 2,
+      offsetPx: 1000,
+      bpPerPx: 2,
+    }
+    const dynamicBlocks = calculateVisibleRegions(view).getBlocks()
+    const staticBlocks = calculateStaticBlocks(view).getBlocks()
+
+    const dynamicBlock6000 = dynamicBlocks.find(
+      b => b.start === 4000 && b.end === 5000 && b.type === 'ContentBlock',
+    )
+    const staticBlock6000 = staticBlocks.find(
+      b => b.start === 4000 && b.end === 5000 && b.type === 'ContentBlock',
+    )
+    expect(dynamicBlock6000?.offsetPx).toEqual(1002)
+    expect(staticBlock6000?.offsetPx).toEqual(1002)
+  })
 })
 
 // ai generated test

Offset of dynamic blocks above is 1004, static blocks is 1002.

@cmdcolin
Copy link
Collaborator Author

Indeed, I meant to comment that this branch is broken still, I think it has all bugs related to the the screenshots that I posted in the other thread and other bugs too. complicated code. I don't even know what is causing so much complication. I considered completely removing interregionpaddingblocks and elided blocks at least temporarily just to see what was causing all the trouble

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants