feat(gpui): expose item offset and content height on ListState - #10
ps49developer wants to merge 3 commits into
Conversation
|
Caution PR Summary Skipped - Monthly Quota ExceededPR summary skipped as you have reached the free tier limit of 50 PR summaries per month. Please upgrade to a paid plan for MatterAI. Current Plan: Free Tier Upgrade your plan on the console here: https://app.matterai.so/ai-code-reviews?tab=Billing |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/gpui/src/window.rs">
<violation number="1" location="crates/gpui/src/window.rs:4148">
P1: On macOS, this public method records `SurfaceContent::ExternalTexture`, but Metal explicitly skips that variant, so callers see no pixels. Implement the Metal path or gate and document this method as unavailable on macOS.</violation>
<violation number="2" location="crates/gpui/src/window.rs:4159">
P2: When this method runs inside an element with styled opacity, the external texture remains fully opaque because both external-texture backends hardcode `opacity` to `1.0`. Carry `self.element_opacity()` through `PaintSurface` into the external-texture instance.</violation>
</file>
<file name="crates/gpui/src/external_texture.rs">
<violation number="1" location="crates/gpui/src/external_texture.rs:240">
P2: While a renderer uploads a frame, this mutex remains locked for the entire `with_frame` visitor, so the Chromium producer blocks in `submit` instead of using the promised double buffer. Keep front/back frame storage or snapshot/swap before invoking the visitor so GPU upload time cannot stall paint callbacks.</violation>
<violation number="2" location="crates/gpui/src/external_texture.rs:348">
P1: When one source is displayed by two windows, the first renderer's acknowledgement lets `ExternalTextureBuffer` discard dirty regions before the other renderer uploads them. Track acknowledgements per renderer/cache, or retain the union until every consumer has acknowledged it.</violation>
</file>
<file name="crates/gpui_windows/src/directx_renderer.rs">
<violation number="1" location="crates/gpui_windows/src/directx_renderer.rs:944">
P2: When an external frame is displayed at a different size than its texture, this pipeline samples the page with the shared wrap sampler, so edge pixels can bleed from the opposite edge. Use a dedicated linear clamp sampler for external textures instead of binding `self.globals.sampler`.</violation>
</file>
<file name="crates/gpui_wgpu/src/wgpu_renderer.rs">
<violation number="1" location="crates/gpui_wgpu/src/wgpu_renderer.rs:1601">
P2: When an external frame exceeds the adapter's maximum 2D texture dimension, this creates an invalid GPU texture and can poison the frame instead of rejecting the frame. Check `width` and `height` against `self.max_texture_size` before `create_texture`, return failure, and avoid acknowledging that frame.</violation>
</file>
<file name="crates/gpui/src/elements/list.rs">
<violation number="1" location="crates/gpui/src/elements/list.rs:694">
P2: When the list has nonzero style padding, this method reports item 0 at 0 and omits both padding edges from content height. `List` places the first item at `padding.top` and includes both edges in scrollable content, so add the recorded padding to both returned values.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// the entire atlas every frame. See [`crate::external_texture`]. | ||
| /// | ||
| /// Call only during the paint phase of element drawing. | ||
| pub fn paint_external_texture( |
There was a problem hiding this comment.
P1: On macOS, this public method records SurfaceContent::ExternalTexture, but Metal explicitly skips that variant, so callers see no pixels. Implement the Metal path or gate and document this method as unavailable on macOS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui/src/window.rs, line 4148:
<comment>On macOS, this public method records `SurfaceContent::ExternalTexture`, but Metal explicitly skips that variant, so callers see no pixels. Implement the Metal path or gate and document this method as unavailable on macOS.</comment>
<file context>
@@ -4130,7 +4130,37 @@ impl Window {
+ /// the entire atlas every frame. See [`crate::external_texture`].
+ ///
+ /// Call only during the paint phase of element drawing.
+ pub fn paint_external_texture(
+ &mut self,
+ bounds: Bounds<Pixels>,
</file context>
| } | ||
| state.bytes[..needed].copy_from_slice(&bytes[..needed]); | ||
|
|
||
| let acknowledged = self.acknowledged.load(Ordering::Acquire); |
There was a problem hiding this comment.
P1: When one source is displayed by two windows, the first renderer's acknowledgement lets ExternalTextureBuffer discard dirty regions before the other renderer uploads them. Track acknowledgements per renderer/cache, or retain the union until every consumer has acknowledged it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui/src/external_texture.rs, line 348:
<comment>When one source is displayed by two windows, the first renderer's acknowledgement lets `ExternalTextureBuffer` discard dirty regions before the other renderer uploads them. Track acknowledgements per renderer/cache, or retain the union until every consumer has acknowledged it.</comment>
<file context>
@@ -0,0 +1,1048 @@
+ }
+ state.bytes[..needed].copy_from_slice(&bytes[..needed]);
+
+ let acknowledged = self.acknowledged.load(Ordering::Acquire);
+ if reallocated {
+ // A recreated texture is uploaded whole; per-rect bookkeeping would
</file context>
|
|
||
| let bounds = self.snap_bounds(bounds); | ||
| let content_mask = self.snapped_content_mask(); | ||
| self.next_frame.scene.insert_primitive(PaintSurface { |
There was a problem hiding this comment.
P2: When this method runs inside an element with styled opacity, the external texture remains fully opaque because both external-texture backends hardcode opacity to 1.0. Carry self.element_opacity() through PaintSurface into the external-texture instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui/src/window.rs, line 4159:
<comment>When this method runs inside an element with styled opacity, the external texture remains fully opaque because both external-texture backends hardcode `opacity` to `1.0`. Carry `self.element_opacity()` through `PaintSurface` into the external-texture instance.</comment>
<file context>
@@ -4130,7 +4130,37 @@ impl Window {
+
+ let bounds = self.snap_bounds(bounds);
+ let content_mask = self.snapped_content_mask();
+ self.next_frame.scene.insert_primitive(PaintSurface {
+ order: 0,
+ bounds,
</file context>
| slice::from_ref(&view), | ||
| slice::from_ref(&resources.viewport), | ||
| slice::from_ref(&self.globals.global_params_buffer), | ||
| slice::from_ref(&self.globals.sampler), |
There was a problem hiding this comment.
P2: When an external frame is displayed at a different size than its texture, this pipeline samples the page with the shared wrap sampler, so edge pixels can bleed from the opposite edge. Use a dedicated linear clamp sampler for external textures instead of binding self.globals.sampler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui_windows/src/directx_renderer.rs, line 944:
<comment>When an external frame is displayed at a different size than its texture, this pipeline samples the page with the shared wrap sampler, so edge pixels can bleed from the opposite edge. Use a dedicated linear clamp sampler for external textures instead of binding `self.globals.sampler`.</comment>
<file context>
@@ -697,13 +733,221 @@ impl DirectXRenderer {
+ slice::from_ref(&view),
+ slice::from_ref(&resources.viewport),
+ slice::from_ref(&self.globals.global_params_buffer),
+ slice::from_ref(&self.globals.sampler),
+ 0,
+ 1,
</file context>
|
|
||
| impl fmt::Debug for ExternalTextureBuffer { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let state = self.state.lock(); |
There was a problem hiding this comment.
P2: While a renderer uploads a frame, this mutex remains locked for the entire with_frame visitor, so the Chromium producer blocks in submit instead of using the promised double buffer. Keep front/back frame storage or snapshot/swap before invoking the visitor so GPU upload time cannot stall paint callbacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui/src/external_texture.rs, line 240:
<comment>While a renderer uploads a frame, this mutex remains locked for the entire `with_frame` visitor, so the Chromium producer blocks in `submit` instead of using the promised double buffer. Keep front/back frame storage or snapshot/swap before invoking the visitor so GPU upload time cannot stall paint callbacks.</comment>
<file context>
@@ -0,0 +1,1048 @@
+
+impl fmt::Debug for ExternalTextureBuffer {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let state = self.state.lock();
+ f.debug_struct("ExternalTextureBuffer")
+ .field("id", &self.id)
</file context>
| if width == 0 || height == 0 { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
P2: When an external frame exceeds the adapter's maximum 2D texture dimension, this creates an invalid GPU texture and can poison the frame instead of rejecting the frame. Check width and height against self.max_texture_size before create_texture, return failure, and avoid acknowledging that frame.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui_wgpu/src/wgpu_renderer.rs, line 1601:
<comment>When an external frame exceeds the adapter's maximum 2D texture dimension, this creates an invalid GPU texture and can poison the frame instead of rejecting the frame. Check `width` and `height` against `self.max_texture_size` before `create_texture`, return failure, and avoid acknowledging that frame.</comment>
<file context>
@@ -1446,6 +1505,201 @@ impl WgpuRenderer {
+ let format = resources.external_texture_format;
+ let width = frame.size.width.0.max(0) as u32;
+ let height = frame.size.height.0.max(0) as u32;
+ if width == 0 || height == 0 {
+ return true;
+ }
</file context>
| if width == 0 || height == 0 { | |
| return true; | |
| } | |
| if width == 0 || height == 0 { | |
| return true; | |
| } | |
| if width > self.max_texture_size || height > self.max_texture_size { | |
| log::error!( | |
| "external texture {id:?} exceeds the maximum texture dimension {}", | |
| self.max_texture_size | |
| ); | |
| return false; | |
| } |
| let total = state.items.summary(); | ||
| let known = total.count - total.unknown_height_count; | ||
| let mean = if known > 0 { | ||
| total.height.0 / known as f32 | ||
| } else { | ||
| 0. | ||
| }; | ||
| let mut cursor = state.items.cursor::<ListItemSummary>(()); | ||
| let before: ListItemSummary = cursor.summary(&Count(ix), Bias::Right); | ||
| ( | ||
| px(before.height.0 + mean * before.unknown_height_count as f32), | ||
| px(total.height.0 + mean * total.unknown_height_count as f32), | ||
| ) | ||
| } |
There was a problem hiding this comment.
P2: When the list has nonzero style padding, this method reports item 0 at 0 and omits both padding edges from content height. List places the first item at padding.top and includes both edges in scrollable content, so add the recorded padding to both returned values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/gpui/src/elements/list.rs, line 694:
<comment>When the list has nonzero style padding, this method reports item 0 at 0 and omits both padding edges from content height. `List` places the first item at `padding.top` and includes both edges in scrollable content, so add the recorded padding to both returned values.</comment>
<file context>
@@ -685,6 +686,26 @@ impl ListState {
+ /// numbers stay proportional before every item has been laid out.
+ pub fn item_top_and_content_height(&self, ix: usize) -> (Pixels, Pixels) {
+ let state = &*self.0.borrow();
+ let total = state.items.summary();
+ let known = total.count - total.unknown_height_count;
+ let mean = if known > 0 {
</file context>
| let total = state.items.summary(); | |
| let known = total.count - total.unknown_height_count; | |
| let mean = if known > 0 { | |
| total.height.0 / known as f32 | |
| } else { | |
| 0. | |
| }; | |
| let mut cursor = state.items.cursor::<ListItemSummary>(()); | |
| let before: ListItemSummary = cursor.summary(&Count(ix), Bias::Right); | |
| ( | |
| px(before.height.0 + mean * before.unknown_height_count as f32), | |
| px(total.height.0 + mean * total.unknown_height_count as f32), | |
| ) | |
| } | |
| let total = state.items.summary(); | |
| let padding = state.last_padding.unwrap_or_default(); | |
| let known = total.count - total.unknown_height_count; | |
| let mean = if known > 0 { | |
| total.height.0 / known as f32 | |
| } else { | |
| 0. | |
| }; | |
| let mut cursor = state.items.cursor::<ListItemSummary>(()); | |
| let before: ListItemSummary = cursor.summary(&Count(ix), Bias::Right); | |
| ( | |
| padding.top + px(before.height.0 + mean * before.unknown_height_count as f32), | |
| total.height | |
| + padding.top | |
| + padding.bottom | |
| + px(mean * total.unknown_height_count as f32), | |
| ) |
2 participants
Summary by cubic
Adds an unatlased external texture primitive so the embedded browser's Chromium frames composite into the GPUI scene without evicting the sprite atlas, and exposes item offsets and content height on
ListState. macOS keeps its CoreVideo surface path; external textures are a no-op there.New Features
Window::paint_external_texturedraws caller-owned RGBA/BGRA frames from a dedicated GPU texture.PaintSurfaceand now composite these frames.ExternalTextureBufferprovides double buffering, dirty-rect accumulation, and a generation that bumps on resize and forces a texture recreate.ListState::item_top_and_content_heightreturns an item's top and the content height, treating unmeasured items as the mean known height so values stay proportional before layout finishes.Migration
PaintSurfacenow carries aSurfaceContentenum instead of a CoreVideoimage_bufferfield; custom renderers that construct surfaces directly need to update.mark_uploadedfrom insidewith_framewithout taking the producer's lock, or the render thread deadlocks.Written for commit 90f4df8. Summary will update on new commits.