-
-
Notifications
You must be signed in to change notification settings - Fork 54
Fix missing System.Drawing assembly reference #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing System.Drawing assembly reference #218
Conversation
Added System.Drawing.Common 10.0.0 package reference to all platform projects that compile shared System.Drawing source files. This fixes the IImage type reference error in GeneratedIconSource.System.Drawing.cs for .NET 10 target frameworks. Projects updated: - H.NotifyIcon.Wpf (net10.0-windows) - H.NotifyIcon.WinUI (net10.0-windows10.0.17763.0) - H.NotifyIcon.Uno (net10.0) - H.NotifyIcon.Uno.WinUI (net10.0, net10.0-windows10.0.19041) - H.NotifyIcon.Maui (net10.0, net10.0-windows10.0.19041.0) Resolves: Assembly reference to System.Private.Windows.GdiPlus
WalkthroughConditional PackageReference entries for System.Drawing.Common (version 10.0.0) are added to five project files (Maui, Uno.WinUI, Uno, WinUI, and Wpf), each targeting specific .NET 10.0 framework variants without altering functional code or public APIs. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
📚 Learning: 2025-10-23T11:04:44.417ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request ReviewSummaryThis PR adds System.Drawing.Common version 10.0.0 as a package reference to all platform-specific projects that compile shared System.Drawing source files for .NET 10 target frameworks. This resolves build issues where System.Drawing types (Icon, Bitmap) were not available. Positive Findings1. Problem Identification
2. Consistency
3. Scope
See next comment for issues and recommendations... |
Issues and Concerns (CRITICAL)1. Incomplete Target Framework Coverage Several projects target multiple .NET 10 frameworks, but the conditions do not cover all of them. For example, H.NotifyIcon.Uno.WinUI targets:
But the condition only covers 2 of 5 frameworks (net10.0 and net10.0-windows10.0.19041). Missing coverage: net10.0-android, net10.0-ios, net10.0-maccatalyst The same issue affects H.NotifyIcon.Maui which also targets these mobile frameworks. Why this matters: The shared GeneratedIconSource.System.Drawing.cs file uses System.Drawing.Icon and System.Drawing.Bitmap types via the global usings when HAS_SYSTEM_DRAWING is defined. These types will be unavailable on mobile target frameworks, causing build failures. |
|
2. Platform Appropriateness System.Drawing.Common is not recommended for mobile platforms (Android, iOS, macOS). Microsoft guidance states:
Questions to consider:
3. Code Observation Looking at src/libs/H.NotifyIcon.Shared/GeneratedIconSource.System.Drawing.cs, all methods are marked with [SupportedOSPlatform("windows")]. This suggests the System.Drawing code path is Windows-only by design. However, the package reference is being added for all platforms without conditional compilation to prevent System.Drawing code from being included on mobile. |
RecommendationsHigh Priority:
Medium Priority:
|
Summary AssessmentCode Quality: Needs improvement The PR correctly identifies the need for explicit System.Drawing.Common references in .NET 10, but does not fully account for multi-platform scenarios where System.Drawing may not be appropriate or supported. Before merging, please verify the behavior on all target frameworks and clarify the intended platform support for the GeneratedIconSource functionality. Alternative ApproachConsider leveraging the existing GraphicsLibrary infrastructure in Directory.Build.props to add platform-specific conditions that use SkiaSharp on mobile platforms. This would avoid System.Drawing.Common dependency on non-Windows platforms and align with the existing HAS_SYSTEM_DRAWING vs HAS_SKIA_SHARP defines. |
Pull request was closed
Added System.Drawing.Common 10.0.0 package reference to all platform projects that compile shared System.Drawing source files. This fixes the IImage type reference error in GeneratedIconSource.System.Drawing.cs for .NET 10 target frameworks.
Projects updated:
Resolves: Assembly reference to System.Private.Windows.GdiPlus
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.