Repository navigation
Choose a surface material for the desktop app - #1035
Closed
alexeyzimarev wants to merge 47 commits into
Closed
alexeyzimarev wants to merge 47 commits into
alexeyzimarev wants to merge 47 commits into
Conversation
Material is a second axis beside palette: ThemeVariant stays pinned to Dark, so a light palette later does not multiply the glass variants. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Foreground drawn beside a glass layer is captured into its own backdrop unless the template root opts out of capture, so every glass template sets the flag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A site's local CornerRadius outranks any style, so the glass radius is its own property; the chip presenter is renamed because Fluent's state styles survive a Template swap. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A scratch spike settled the mechanisms first: the vendored source builds on Avalonia 12.1.2, inherited-property selectors reach flyouts, and an overlay popup refracts the window. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two of them are rows of the virtualised list, where a templated control triples the visuals; the permission and question cards still migrate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Later-declared styles win at equal priority, so an include above the inline kcapChip style would lose the picker padding. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The package is not on NuGet.org, and upstream reports no pipeline failure, so the copy carries one event the app can fall back on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The root .editorconfig reaches src/ThirdParty by directory ancestry, so not importing the root props does not stop its CA severities. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The stored choice is a string because the state store resets the whole file on any deserialization failure. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SetAsync resumes off the UI thread, so an unsynchronized publish could overwrite a latched failure with a stale snapshot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The library raises its failure event on the render thread, so the report is posted to the UI thread rather than made in place. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The glass template's root opts out of the window snapshot, or its own content is blurred underneath itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The XAML selector grammar rejects a :not() around a property match, so an any-glass style carries one arm per glass material. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The coordinator is built in a synchronous method on the UI thread, so the awaited load belongs one call earlier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The vendored glass surface builds a presenter of its own, so a lookup by type alone finds two. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The chat's own cards stay Borders: two are rows of the virtualised list, where a templated control triples the visuals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A templated control realises its content on first measure, so content under a collapsed ancestor is reachable by name scope only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The rail's own styles outrank application styles, so an app-level glass fill could lose without any layout assertion noticing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The presenter is renamed because Fluent's per-state fills target PART_ContentPresenter and survive a Template swap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two selector arms carry one template, so a dropped arm would leave Liquid glass opaque with no failing test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two selector arms carry one template, so a dropped arm leaves Liquid glass opaque with no failing test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The work-context LinkCard takes main's button and this branch's Surface: the card style selects kcap|Surface.card, so main's Border.card wrapper stands on its own transparent chrome. AppState carries main's five window fields and this branch's Material together — JSON is by name, so the order is free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The workspace host pins itself opaque, so a reading surface never sits on the backdrop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A control's own styles are applied after application styles and win an equal-priority tie. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A control's own styles are applied after application styles and win an equal-priority tie, so a glass row fill declared at application level lost to the rail's opaque row styles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The panel tints but never refracts: the same GlassLayer loses all stripe contrast in the window and in a bare overlay-layer popup, yet keeps 4.3 of 10.8 inside a flyout presenter. The styles and the helper stay out of App.axaml and off every flyout site until that is understood. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A glass layer in the presenter's own template tints but never paints the backdrop, while the same layer as flyout content does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Content wrapping paints no backdrop either: the same layer that reaches 0.0 edge contrast in a bare overlay-layer popup keeps 4.4 of 10.4 as flyout content, so the earlier 0.0 for content was Fluent's opaque presenter, not refraction. Everything stays unwired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Glass in a Flyout's popup never receives a backdrop snapshot in either placement, while a bare overlay-layer Popup does; the probe records the lead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Glass in a Flyout's popup never receives a backdrop snapshot, whether in the presenter's template or as wrapped content; the probe keeps both shapes runnable for the follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The flyout probe failed, so the copy the plan hands the docs task must not describe glass menus. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Nothing pinned IsExcludedFromCapture on the glass chip template's root; dropping it would blur each chip's own label under itself with the suite green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LiquidGlassPipeline's report latches once per process, and the render test beside it can trip that latch first, making the count assertion vacuous whichever test wins the race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Radial defaults are relative units, so one immutable instance paints any bounds; the vendored backdrop provider re-renders on every capture under glass, so a fresh brush per glow per Render was wasted allocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A SetAsync still awaiting its file write when the app quits resumes into Publish on a disposed BehaviorSubject; a _disposed flag under the same gate makes that publish a no-op instead of an unobserved ObjectDisposedException. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pins the rail's styled width against real headless layout, drops dead XAML (a Classes="card" no style targets anymore), removes change narration from two comments, and re-indents kcap:Surface continuation attributes to the rest of each file's convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoAdd selectable desktop surface materials with glass rendering
AI Description
Diagram
High-Level Assessment
Files changed (88)
|
Code Review by Qodo
1. Material choices can erase window state
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| public enum SurfaceMaterial { Opaque, SoftGlass, LiquidGlass } | ||
|
|
||
| public static class SurfaceMaterials { |
There was a problem hiding this comment.
4. The material file holds two main types 📘 Rule violation ⚙ Maintainability
SurfaceMaterial.cs declares both the SurfaceMaterial enum and the top-level SurfaceMaterials helper class. The helper also contains the non-extension Parse method and does not follow the enum-extension naming exception, leaving two independently discoverable types in one file.
Agent Prompt
## Issue description
`SurfaceMaterial.cs` contains both the enum and a separate top-level helper type that does not satisfy the narrow enum-extension exception.
## Fix Focus Areas
- src/Capacitor.App/Materials/SurfaceMaterial.cs[3-20]
## Recommended Fix
Move `SurfaceMaterials` into `SurfaceMaterials.cs`, or reshape it into a conventionally named enum extension class whose sole purpose is extension methods.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| } | ||
|
|
||
| internal sealed class LiquidGlassInnerShadowDrawOperation : ICustomDrawOperation |
There was a problem hiding this comment.
5. Two shadow operations share one file 📘 Rule violation ⚙ Maintainability
LiquidGlassShadowDrawOperations.cs declares LiquidGlassShadowDrawOperation and LiquidGlassInnerShadowDrawOperation as separate top-level draw-operation classes. Neither class matches the plural filename or belongs to an allowed private hierarchy, so maintainers must search inside an unrelated filename to find either implementation.
Agent Prompt
## Issue description
Two concrete top-level shadow draw operations are grouped under a plural filename that matches neither type.
## Fix Focus Areas
- src/ThirdParty/LiquidGlassAvaloniaUI/LiquidGlassShadowDrawOperations.cs[11-126]
## Recommended Fix
Move each draw-operation class into a source file whose name exactly matches that class, preserving namespace and internal visibility.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+2
| // Liquid Glass Gamma Adjustment Shader | ||
| // Adapted for SKRuntimeEffect. |
There was a problem hiding this comment.
2. Six shader headers narrate history 📘 Rule violation ⚙ Maintainability
Six vendored shader files begin with labels such as Liquid Glass Gamma Adjustment Shader and Adapted for SKRuntimeEffect, which repeat the effect name and record adaptation provenance rather than a rendering constraint. These comments accompany every newly added shader, leaving later readers to maintain origin text that explains no current behavior or usage trap.
Agent Prompt
## Issue description
The vendored shaders contain redundant effect labels and historical adaptation notes rather than behavior-critical comments.
## Fix Focus Areas
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassGamma.sksl[1-2]
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassHighlight.sksl[1-2]
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassInteractiveHighlight.sksl[1-2]
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassProgressiveMask.sksl[1-2]
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassShader.sksl[1-2]
- src/ThirdParty/LiquidGlassAvaloniaUI/Assets/Shaders/LiquidGlassBackdropTransform.sksl[1-2]
## Recommended Fix
Remove the redundant headers, retaining only comments that explain current shader invariants, coordinate conventions, or non-obvious rendering constraints.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| namespace LiquidGlassAvaloniaUI | ||
| { | ||
| // Local addition, not upstream: see VENDORED.md. |
There was a problem hiding this comment.
3. A pipeline comment records provenance 📘 Rule violation ⚙ Maintainability
The comment above LiquidGlassPipeline says Local addition, not upstream: see VENDORED.md instead of documenting the event latch or reporting contract implemented below it. Its truth depends on repository history and upstream status, so a later vendor refresh can make the comment stale without any behavioral change.
Agent Prompt
## Issue description
The pipeline comment records local-versus-upstream provenance rather than a current behavior-critical constraint.
## Fix Focus Areas
- src/ThirdParty/LiquidGlassAvaloniaUI/LiquidGlassPipeline.cs[6-6]
## Recommended Fix
Remove the provenance comment or replace it with a concise explanation of the one-report-per-process latch if that invariant is not already clear from the implementation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+293
to
+294
| _material ??= await MaterialService.LoadAsync( | ||
| new AppStateStore(_config.Path("app-state.json")), MaterialEnvironment.Detect()); |
There was a problem hiding this comment.
1. Material choices can erase window state 🐞 Bug ☼ Reliability
StartAsync creates a dedicated AppStateStore for MaterialService, while main-window construction creates a different store instance targeting the same app-state.json file, and each store protects full-file read-modify-write operations with only its own semaphore. When material selection overlaps the window-size timer or close-handler geometry save, both stores can read stale state and overwrite the other update, losing either the selected material or the latest window geometry.
Agent Prompt
## Issue description
Material persistence uses a separate `AppStateStore` instance from the one used by main-window and `WindowSizeMemory` persistence, even though both target `app-state.json`. Because synchronization is instance-local, concurrent full-file read-modify-write operations can overwrite one another.
## Fix Focus Areas
- src/Capacitor.App/App.axaml.cs[293-294]
- src/Capacitor.App/App.axaml.cs[1211-1250]
- src/Capacitor.App/Materials/MaterialService.cs[32-35]
- src/Capacitor.App/Services/AppStateStore.cs[35-52]
## Recommended Fix
Create and retain one application-level `IAppStateStore`/`AppStateStore` for `app-state.json`, then pass that same instance to `MaterialService.LoadAsync` and the main-window/window-size persistence path. Alternatively, ensure every `AppStateStore` targeting the same normalized path shares one synchronization gate, keeping the entire read-modify-write and temporary-file replacement operation inside that shared critical section.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89de5d9ceb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ??= keeps a second pass (the wizard hands over to a fresh graph) from building a | ||
| // second service or a second event subscription. | ||
| _material ??= await MaterialService.LoadAsync( | ||
| new AppStateStore(_config.Path("app-state.json")), MaterialEnvironment.Detect()); |
There was a problem hiding this comment.
This creates a separate AppStateStore for the material service even though the main window, lifecycle coordinator, and re-auth flows create other instances for the same app-state.json. Because the store's semaphore is instance-local and every update reads and rewrites the entire file, a material selection concurrent with a background shim/consent update can overwrite that update or have its new Material value overwritten. Reuse one store instance, or otherwise serialize updates by path, so independent fields cannot be lost.
Useful? React with 👍 / 👎.
Reduce transparency stays on the material state and in the Appearance hint, but it no longer chooses anything: glass is an explicit choice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1033 — AI-2983
What & why
The desktop app draws its session rail and launcher in a material chosen under Settings → Appearance: Opaque, Soft glass or Liquid glass; Opaque is the default and glass an explicit choice. Material is a second axis beside palette, carried by an inherited
MaterialScope.Materialproperty, so a later light palette does not multiply the glass variants. Glass is a control, not a brush: every inline card outside the chat view is aSurfacewhose template changes, and oneGlassLayerowns all glass drawing.LiquidGlassAvaloniaUIis vendored as source (it is not on NuGet.org) with one patch that reports a shader pipeline that cannot run; on it the app latches Opaque for the session and keeps the stored choice.Where to look
The glass rail row fills sit at the end of
SessionRailView's own styles: a view's styles are applied afterApplication.Stylesand win an equal-priority tie, so the same rule at application level is dead. Panel flyouts stay opaque — glass inside a Flyout's popup never receives a backdrop snapshot; the runnable probe underdocs/probes/holds the measurements.Verification
dotnet build Capacitor.slnx: 17 projects, 0 warnings, 0 errors.AttachmentTrayTests.Size_label_and_image_flag, this machine's decimal-comma locale, unrelated.#ff1e2e52instead of#244ed6ba;A_selected_rail_row_takes_the_glass_fill_under_glasscatches it. Dropping the chip root'sIsExcludedFromCapturefailsGlassChipTeststhe same way.🤖 Generated with Claude Code