Skip to content

Conversation

@t1m0thyj
Copy link
Owner

@t1m0thyj t1m0thyj commented Nov 18, 2025

Advantages:

  • Smaller binaries (by ~15 MB) and memory footprint
  • Can achieve higher refresh rate for animation (120 fps)

Disadvantages:

  • More complex code to manually render UI elements

@t1m0thyj t1m0thyj force-pushed the feat/replace-wpf-with-skia branch 3 times, most recently from c0a9ee5 to 34a90ab Compare November 20, 2025 04:40
@t1m0thyj t1m0thyj force-pushed the feat/replace-wpf-with-skia branch from 34a90ab to 54d5a49 Compare November 20, 2025 04:41
@t1m0thyj t1m0thyj marked this pull request as ready for review November 22, 2025 05:29
@t1m0thyj t1m0thyj force-pushed the feat/replace-wpf-with-skia branch from caf4450 to ba21f1f Compare November 29, 2025 03:39
@t1m0thyj t1m0thyj force-pushed the feat/replace-wpf-with-skia branch from c2fbd4b to d82463d Compare November 30, 2025 04:54
@t1m0thyj t1m0thyj requested a review from Copilot November 30, 2025 04:55
Copilot finished reviewing on behalf of t1m0thyj November 30, 2025 04:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the theme preview rendering from WPF to SkiaSharp, aiming to reduce binary size by ~15 MB and enable higher animation refresh rates (120 fps). The migration removes WPF dependencies and replaces XAML-based UI with manual rendering code using SkiaSharp's canvas API.

Key changes:

  • Replaced WPF ThemePreviewer UserControl with SkiaSharp-based SKControl implementation
  • Implemented manual UI rendering in ThemePreviewRenderer for overlays, buttons, and animations
  • Created ImageCache to manage SkiaSharp image resources with thread-safe caching
  • Removed WPF dependencies from project configuration and replaced System.Windows.Rect with custom RECT struct

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 22 comments.

Show a summary per file
File Description
src/WinDynamicDesktop.csproj Removed WPF dependency, added SkiaSharp packages (v3.119.1), changed font resource from Resource to EmbeddedResource, added MSBuild target to exclude OpenTK/glfw3 from single-file bundle
src/WPF/ThemePreviewer.xaml.cs Deleted - WPF code-behind for theme previewer user control
src/WPF/ThemePreviewer.xaml Deleted - XAML markup defining WPF-based theme preview UI
src/WPF/RelayCommand.cs Deleted - WPF ICommand implementation no longer needed
src/WPF/BitmapCache.cs Deleted - WPF BitmapImage cache replaced by SkiaSharp ImageCache
src/ThemeDialog.cs Updated to use new Skia.ThemePreviewer instead of WPF ThemePreviewer, removed ElementHost initialization
src/ThemeDialog.Designer.cs Changed previewerHost type from ElementHost to Skia.ThemePreviewer
src/Skia/ThemePreviewerViewModel.cs Refactored from WPF namespace to Skia, replaced BitmapImage with SKImage, changed from WPF DispatcherTimer to Windows Forms Timer, removed ICommand dependencies, added public methods for UI interactions
src/Skia/ThemePreviewer.cs New SkiaSharp-based control inheriting from SKControl, implements 120 FPS animations using Forms Timer, handles mouse/keyboard input, manages FontAwesome font loading
src/Skia/ThemePreviewRenderer.cs New renderer class handling all manual drawing of UI elements (images, overlays, buttons, text) using SkiaSharp canvas APIs
src/Skia/ImageCache.cs New thread-safe image cache for SkiaSharp SKImage resources with decode size limiting and explicit disposal
src/COM/DesktopWallpaper.cs Added RECT struct definition and changed GetMonitorRECT return type from System.Windows.Rect to custom RECT to remove WPF dependency
.github/workflows/codeql.yml Added .NET setup step with version 10.0.x and security-extended queries
Files not reviewed (1)
  • src/ThemeDialog.Designer.cs: Language not supported
Comments suppressed due to low confidence (7)

src/Skia/ThemePreviewerViewModel.cs:386

  • [nitpick] Potential memory leak: BackImage and FrontImage SKImage objects are set to null without being disposed. Since these are references to images from the cache, and the cache handles disposal in Clear(), this is okay. However, for clarity and to follow best practices, consider adding a comment explaining that these images are owned by the cache and will be disposed there.
    src/Skia/ThemePreviewer.cs:51
  • Write to static field from instance method, property, or constructor.
                    fontAwesome = SKTypeface.FromStream(fontStream);

src/Skia/ThemePreviewerViewModel.cs:447

  • Poor error handling: empty catch block.
    src/Skia/ThemePreviewerViewModel.cs:399
  • Local scope variable 'previewText' shadows ThemePreviewerViewModel.previewText.
    src/Skia/ThemePreviewerViewModel.cs:227
  • Local scope variable 'downloadAction' shadows ThemePreviewerViewModel.downloadAction.
    src/Skia/ThemePreviewerViewModel.cs:399
  • This assignment to previewText is useless, since its value is never read.
    src/Skia/ThemePreviewerViewModel.cs:36
  • Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +142 to +143
var image = SKImage.FromBitmap(scaledBitmap);
return image;
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing null check after SKImage.FromBitmap() calls. If the bitmap conversion fails, this could return null but the code doesn't handle this case. Consider adding null checks before returning the image, or use a guard clause to ensure the image is not null before adding it to the cache.

Copilot uses AI. Check for mistakes.
Comment on lines +149 to +150
var image = SKImage.FromBitmap(sourceBitmap);
return image;
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing null check after SKImage.FromBitmap() call. If the bitmap conversion fails, this could return null but the code doesn't handle this case. Consider adding a null check before returning the image.

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +51
if (fontAwesome == null)
{
using (Stream fontStream = Assembly.GetExecutingAssembly()
.GetManifestResourceStream("WinDynamicDesktop.resources.fonts.fontawesome-webfont.ttf"))
{
fontAwesome = SKTypeface.FromStream(fontStream);
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The static fontAwesome SKTypeface is never disposed. Since it's loaded once and kept for the lifetime of the application, it should be properly disposed when the application terminates. Consider implementing IDisposable on a static manager class or ensuring proper cleanup. This could lead to a resource leak, though it may be acceptable if it's truly application-lifetime.

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +50
titleFont = new SKFont(SKTypeface.FromFamilyName("Segoe UI", SKFontStyleWeight.Bold, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright), 19);
previewFont = new SKFont(SKTypeface.FromFamilyName("Segoe UI", SKFontStyleWeight.Normal, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright), 16);
textFont = new SKFont(SKTypeface.FromFamilyName("Segoe UI"), 16);
iconFont16 = new SKFont(fontAwesome, 16);
iconFont20 = new SKFont(fontAwesome, 20);
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SKTypeface objects created from FromFamilyName() should be stored as fields and properly disposed. Currently, they are created inline and passed to SKFont constructors. The SKFont.Dispose() will not dispose the underlying typeface. Store these typefaces as fields and dispose them in the Dispose method to prevent resource leaks.

Copilot uses AI. Check for mistakes.
</Project>
<Target Name="ExplicitRemoveFromFilesToBundle" BeforeTargets="GenerateSingleFileBundle" DependsOnTargets="PrepareForBundle">
<ItemGroup>
<FilesToRemoveFromBundle Include="@(FilesToBundle)" Condition="($([System.String]::Copy('%(Filename)').StartsWith('OpenTK'))) OR ('%(Filename)' == 'glfw3')" />
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The MSBuild target filtering condition uses string operations that may not work as expected. The condition $([System.String]::Copy('%(Filename)').StartsWith('OpenTK')) creates an unnecessary copy. Consider simplifying to '%(Filename)'.StartsWith('OpenTK') or use a more standard MSBuild pattern like @(FilesToBundle->WithMetadataValue('Filename', 'OpenTK*')).

Suggested change
<FilesToRemoveFromBundle Include="@(FilesToBundle)" Condition="($([System.String]::Copy('%(Filename)').StartsWith('OpenTK'))) OR ('%(Filename)' == 'glfw3')" />
<FilesToRemoveFromBundle Include="@(FilesToBundle->WithMetadataValue('Filename', 'OpenTK*'))" />

Copilot uses AI. Check for mistakes.
// Title and preview text box (top left)
var titleBounds = new SKRect();
titleFont.MeasureText(viewModel.Title ?? "", out titleBounds);
var previewBounds = new SKRect();
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to previewBounds is useless, since its value is never read.

Copilot uses AI. Check for mistakes.
float playOpacity = hoveredItem == ThemePreviewer.HoveredItem.PlayButton ? OPACITY_HOVER : OPACITY_NORMAL;
basePaint.Color = SKColors.White.WithAlpha((byte)(255 * playOpacity));
string playIcon = viewModel.IsPlaying ? "\uf04c" : "\uf04b";
var textBounds = new SKRect();
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to textBounds is useless, since its value is never read.

Suggested change
var textBounds = new SKRect();
SKRect textBounds;

Copilot uses AI. Check for mistakes.
// Download message (centered bottom)
if (!string.IsNullOrEmpty(viewModel.Message))
{
var msgBounds = new SKRect();
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to msgBounds is useless, since its value is never read.

Copilot uses AI. Check for mistakes.
basePaint.Color = SKColors.White.WithAlpha((byte)(255 * opacity));

string icon = side == Side.Left ? "\uf053" : "\uf054";
var textBounds = new SKRect();
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to textBounds is useless, since its value is never read.

Copilot uses AI. Check for mistakes.
return;
}

var textBounds = new SKRect();
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to textBounds is useless, since its value is never read.

Copilot uses AI. Check for mistakes.
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.

2 participants