windows-reactor normalize svg/bitmap/icon support - #4764
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The API normalization is implemented consistently across widgets/backend, with updated docs, samples, and both unit + selftest coverage for SVG/bitmap source selection and update/unset behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Normalizes windows-reactor image and icon handling by introducing a shared ImageSource abstraction used by both Image widgets and image-backed icons, with backend support for decoding SVG vs bitmap sources based on URI extension.
Changes:
- Replace bitmap-only icon support with
Icon::image(ImageSource)backed by WinUIImageIcon, while keepingIcon::bitmap(uri)as a compatibility shorthand. - Make
Image::new(...)acceptimpl Into<ImageSource>and route URI sources toSvgImageSourcevsBitmapImagebased on the final path extension. - Update unit/integration tests, selftests, docs, and samples to cover URI conversions, update/unset behavior, and SVG decoding.
File summaries
| File | Description |
|---|---|
| docs/crates/windows-reactor.md | Documents Icon::image and Image::new URI/SVG behavior and points to updated samples. |
| crates/tools/reactor/src/test.txt | Extends the selftest bindgen filter to include get_Source for Image/ImageIcon. |
| crates/tools/reactor/src/base.txt | Swaps WinUI bindings from BitmapIcon to ImageIcon + adds SvgImageSource pieces needed by the backend. |
| crates/tests/libs/reactor/tests/image_bindings.rs | Updates bindings expectations to use PropValue::ImageSource and adds conversion coverage for Image::new. |
| crates/tests/libs/reactor/tests/icon_bindings.rs | Updates icon tests to validate Icon::Image(ImageSource) and Icon conversions from ImageSource. |
| crates/tests/libs/reactor/tests/controls_shapes_media.rs | Verifies ImageSource set/update/unset behavior via reconciler ops without remounting. |
| crates/tests/libs/reactor_selftest/src/registry.rs | Renames the interaction fixture key to reflect ImageIcon usage. |
| crates/tests/libs/reactor_selftest/src/fixtures/interactions.rs | Updates the interaction fixture to validate both bitmap and SVG image icon sources at runtime. |
| crates/tests/libs/reactor_selftest/src/fixtures/controls.rs | Expands the image mount fixture to validate both bitmap and SVG sources on Image. |
| crates/tests/libs/reactor_selftest/src/bindings.rs | Regenerates selftest bindings to include ImageIcon, SvgImageSource, and Source() getters needed by fixtures. |
| crates/samples/reactor/samples/examples/image.svg | Adds an SVG asset for the updated image sample. |
| crates/samples/reactor/samples/examples/image.rs | Updates the image sample to show both PNG and SVG plus existing stretch modes. |
| crates/samples/reactor/samples/examples/icon_elements.rs | Updates the icon sample to use Icon::image (SVG) instead of bitmap-only icon usage. |
| crates/samples/reactor/direct2d/src/surface_image_source.rs | Simplifies Image::new usage with SurfaceImageSource now accepted directly via Into<ImageSource>. |
| crates/libs/reactor/src/widgets/image.rs | Introduces ImageSource helpers/conversions and emits unified PropValue::ImageSource bindings. |
| crates/libs/reactor/src/widgets/icon.rs | Replaces bitmap icon variant with Icon::Image(ImageSource) and keeps bitmap() as a compatibility constructor. |
| crates/libs/reactor/src/bindings.rs | Regenerates reactor bindings to remove BitmapIcon and add ImageIcon/SvgImageSource support. |
| crates/libs/reactor/src/backend/winui/mod.rs | Updates property application/mounting to use unified build_image_source for images. |
| crates/libs/reactor/src/backend/winui/convert.rs | Adds URI classification + construction for BitmapImage vs SvgImageSource, and builds ImageIcon elements for icons. |
| crates/libs/reactor/src/backend/mod.rs | Replaces PropValue::SurfaceImageSource with PropValue::ImageSource in the backend value model. |
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 0
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Tam1SH
pushed a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 4, 2026
Tam1SH
added a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 4, 2026
…around Upstream's microsoft#4764 (7ad6448, already the base of HEAD) collapsed Icon::Bitmap{uri}/Icon::Svg{uri,width,height} into a unified Icon::Image(ImageSource) with no size fields, moving sizing to ordinary Element width()/height() modifiers instead. Our two size-carrying local features built on the old Icon::Svg shape don't translate onto that: - TitleBar.icon() (build_icon_source, ITitleBar.SetIconSource) is removed entirely rather than ported - it never actually applied Icon::Svg's width/height in the first place (build_icon_source matched `Icon::Bitmap { uri } | Icon::Svg { uri, .. }`, silently dropping the size via `..`), so there's no working behavior to preserve, and Icon::Image has nowhere to put a size even if there were. - The NavigationViewItem 16x16 IconElement-clamp bypass (rendering Icon::Svg/Bitmap as a plain Image in Content instead of via SetIcon, to work around microsoft-ui-xaml#1710) is also removed. Nav items go back to plain SetIcon, clamped to 16x16 like stock WinUI3 - matching upstream's own microsoft-ui-reactor, which has the identical BitmapIcon/ImageIcon resolution capability and no workaround for this either. - select_nav_item_by_tag's "no-op if already selected" guard (added alongside the clamp bypass to stop it collapsing WinUI's own selection-indicator slide animation) reverts with it, back to always calling SetSelectedItem. Also deleted a duplicate, newly-dead is_svg_uri in backend/winui/mod.rs left over from the upstream merge (convert.rs has its own copy that upstream's build_uri_image_source uses). cargo check -p windows-reactor, test_reactor, test_reactor_selftest, reactor_samples, and reactor_gallery all pass clean.
Tam1SH
added a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 5, 2026
Squashed history of local fork work whose functionality either matches or is superseded by upstream's own merged fixes (PR microsoft#4764 SVG/icon support, PR microsoft#4782 keyed-list reorder / pointer capture / NavigationView pane events) - those commits' actual code effect is fully gone from this diff since it now equals origin/master exactly. What remains is only what upstream doesn't have yet: - TextBlock.TextTrimming: SetTextTrimming was a dead usize vtable slot with no safe wrapper; adds the TextTrimming type, a real wrapper, Prop::TextTrimming wiring, and TextBlock::single_line_ellipsis(). Filed upstream, not yet addressed. - Multiple ControlId component-instance tracking + force-rerender of dirty-descendant ancestors (memoised_ancestor_dirty_descendant.rs). - Passthrough-component ControlId collision fix (passthrough_component_control_id_collision.rs, ISSUE draft included). - templated_list_keyed_reorder.rs kept as a regression suite against upstream's remap_keyed_realized_rows - 2/3 tests pass as-is; the third (reorder_with_added_and_removed_key_mounts_and_unmounts_correctly) is #[ignore]d with an explanation: upstream's remap bails to a positional (non-keyed) fallback the instant any old key has no home in the new set, silently reusing a surviving row's native content for an unrelated key instead of destroying/remounting it. Not fixed here. - drag_repro.rs sample, ISSUE draft notes for the above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tam1SH
added a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 5, 2026
Squashed history of local fork work whose functionality either matches or is superseded by upstream's own merged fixes (PR microsoft#4764 SVG/icon support, PR microsoft#4782 keyed-list reorder / pointer capture / NavigationView pane events) - those commits' actual code effect is fully gone from this diff since it now equals origin/master exactly. What remains is only what upstream doesn't have yet: - TextBlock.TextTrimming: SetTextTrimming was a dead usize vtable slot with no safe wrapper; adds the TextTrimming type, a real wrapper, Prop::TextTrimming wiring, and TextBlock::single_line_ellipsis(). Filed upstream, not yet addressed. - Multiple ControlId component-instance tracking + force-rerender of dirty-descendant ancestors (memoised_ancestor_dirty_descendant.rs). - Passthrough-component ControlId collision fix (passthrough_component_control_id_collision.rs, ISSUE draft included). Dropped from the earlier version of this squash after review: - drag_repro.rs sample - upstream's own pointer_resize.rs (added by microsoft#4782) is a strict superset, built on the real capture API. - templated_list_keyed_reorder.rs - upstream's templated_list.rs (also microsoft#4782) already covers keyed-reorder/swap/duplicate-key scenarios with its own tests; ours was redundant except for one #[ignore]d gap (add+remove-in-the-same-update loses key identity via the positional fallback) that has no upstream equivalent test yet - tracked outside this repo rather than kept as dead weight here. - ISSUE-DRAFT-svg-icon-support-request.md, ISSUE-templated-list-key-ignored-on-reorder.md - both describe problems microsoft#4764/microsoft#4782 already fixed upstream. - ISSUE-pointer-capture-and-resubscription.md trimmed to only its still-unfixed half: upstream's set_pointer_handlers still does a full revoke+resubscribe on every render, which silently ends an in-progress .capture_pointer_on_press() capture mid-drag. The other half (no capture API at all) is exactly what microsoft#4782 added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tam1SH
added a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 5, 2026
Squashed history of local fork work whose functionality either matches or is superseded by upstream's own merged fixes (PR microsoft#4764 SVG/icon support, PR microsoft#4782 keyed-list reorder / pointer capture / NavigationView pane events) - those commits' actual code effect is fully gone from this diff since it now equals origin/master exactly. What remains is only what upstream doesn't have yet: - TextBlock.TextTrimming: SetTextTrimming was a dead usize vtable slot with no safe wrapper; adds the TextTrimming type, a real wrapper, Prop::TextTrimming wiring, and TextBlock::single_line_ellipsis(). Filed upstream, not yet addressed. - Multiple ControlId component-instance tracking + force-rerender of dirty-descendant ancestors (memoised_ancestor_dirty_descendant.rs). - Passthrough-component ControlId collision fix (passthrough_component_control_id_collision.rs, ISSUE draft included). Dropped from the earlier version of this squash after review: - drag_repro.rs sample - upstream's own pointer_resize.rs (added by microsoft#4782) is a strict superset, built on the real capture API. - templated_list_keyed_reorder.rs - upstream's templated_list.rs (also microsoft#4782) already covers keyed-reorder/swap/duplicate-key scenarios with its own tests; ours was redundant except for one #[ignore]d gap (add+remove-in-the-same-update loses key identity via the positional fallback) that has no upstream equivalent test yet - tracked outside this repo rather than kept as dead weight here. - ISSUE-DRAFT-svg-icon-support-request.md, ISSUE-templated-list-key-ignored-on-reorder.md - both describe problems microsoft#4764/microsoft#4782 already fixed upstream. - ISSUE-pointer-capture-and-resubscription.md trimmed to only its still-unfixed half: upstream's set_pointer_handlers still does a full revoke+resubscribe on every render, which silently ends an in-progress .capture_pointer_on_press() capture mid-drag. The other half (no capture API at all) is exactly what microsoft#4782 added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tam1SH
added a commit
to Tam1SH/windows-rs
that referenced
this pull request
Aug 5, 2026
Squashed history of local fork work whose functionality either matches or is superseded by upstream's own merged fixes (PR microsoft#4764 SVG/icon support, PR microsoft#4782 keyed-list reorder / pointer capture / NavigationView pane events) - those commits' actual code effect is fully gone from this diff since it now equals origin/master exactly. What remains is only what upstream doesn't have yet: - TextBlock.TextTrimming: SetTextTrimming was a dead usize vtable slot with no safe wrapper; adds the TextTrimming type, a real wrapper, Prop::TextTrimming wiring, and TextBlock::single_line_ellipsis(). Filed upstream, not yet addressed. - Multiple ControlId component-instance tracking + force-rerender of dirty-descendant ancestors (memoised_ancestor_dirty_descendant.rs). - Passthrough-component ControlId collision fix (passthrough_component_control_id_collision.rs, ISSUE draft included). Dropped from the earlier version of this squash after review: - drag_repro.rs sample - upstream's own pointer_resize.rs (added by microsoft#4782) is a strict superset, built on the real capture API. - templated_list_keyed_reorder.rs - upstream's templated_list.rs (also microsoft#4782) already covers keyed-reorder/swap/duplicate-key scenarios with its own tests; ours was redundant except for one #[ignore]d gap (add+remove-in-the-same-update loses key identity via the positional fallback) that has no upstream equivalent test yet - tracked outside this repo rather than kept as dead weight here. - ISSUE-DRAFT-svg-icon-support-request.md, ISSUE-templated-list-key-ignored-on-reorder.md - both describe problems microsoft#4764/microsoft#4782 already fixed upstream. - ISSUE-pointer-capture-and-resubscription.md trimmed to only its still-unfixed half: upstream's set_pointer_handlers still does a full revoke+resubscribe on every render, which silently ends an in-progress .capture_pointer_on_press() capture mid-drag. The other half (no capture API at all) is exactly what microsoft#4782 added. - icon_elements.rs's added module doc comment - not worth carrying. - NavigationView.on_pane_closed/on_pane_opened - upstream's on_pane_open_changed(bool) (also microsoft#4782, closes microsoft#4771) is a strict superset; the discrete Closed/Opened callbacks added nothing a `if open`/`if !open` branch on the bool callback doesn't already cover. Event::PaneOpened variant removed with it; Event::PaneClosed stays (SplitView has its own unrelated use of it). Co-Authored-By: Claude Sonnet 5 <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
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.
No description provided.