Enhances OpenRGB integration with zone support#17
Conversation
Introduces zone-based control for OpenRGB devices, allowing finer-grained control over lighting effects. Updates the UI to display devices and their zones, enabling users to select a function (Off, LightPod, Strobe, Fogger) for each zone. Retains backward compatibility with device-based settings.
Improves UI responsiveness using a dynamic layout based on window size. Enhances reconnection reliability by clearing device lists and UI elements.
Improves OpenRGB integration UI and reconnection
There was a problem hiding this comment.
Pull request overview
This pull request introduces zone-based control for OpenRGB devices, enabling more granular lighting configurations through a redesigned UI. The changes add support for controlling individual zones within OpenRGB devices rather than just entire devices.
Changes:
- Implements zone-based lighting control with new data structures (
DeviceWithZones,DeviceZoneCategory,ZoneInfo) to manage individual zones within devices - Redesigns the OpenRGB tab UI with a responsive layout that adapts between compact and wide modes, featuring modern card-based design with device categorization
- Enhances reconnection logic to properly clear and reinitialize both legacy device-based and new zone-based data structures
- Adds VS Code debugging configuration files for improved development workflow
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| YALCY/Views/Tabs/OpenRgbTabView.axaml | Complete UI redesign with responsive two-column layout, device cards with zone management, and modern styling |
| YALCY/Views/Tabs/OpenRgbTabView.axaml.cs | Implements responsive layout logic with dynamic grid reconfiguration and adds DeviceTypeToIconConverter for device type icons |
| YALCY/ViewModels/MainWindowViewModel.cs | Adds DeviceWithZones and DeviceZoneCategory classes for zone-based device management with category switching logic |
| YALCY/ViewModels/OpenRgbSettings.cs | Adds DevicesWithZones collection and initialization logic for the new zone-based UI |
| YALCY/Integrations/OpenRGB/OpenRgbTalker.cs | Implements zone-based dictionaries alongside legacy device-based lists, updates effects to support both modes, and improves reconnection handling |
| .vscode/launch.json | Adds debugging configurations for YALCY and YALCY.CLI projects |
| .vscode/tasks.json | Adds build, publish, and watch tasks for both projects |
| YALCY/Program.cs | Minor formatting change (trailing whitespace) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Device Device { get; set; } | ||
| public Zone Zone { get; set; } | ||
| public int ZoneIndex { get; set; } |
There was a problem hiding this comment.
The properties Device, Zone, and ZoneIndex should be initialized with null-forgiving operators or marked appropriately. Following the pattern in DeviceCategory (line 418), these should have = null!; since they are always set in the constructor: public Device Device { get; set; } = null!; and public Zone Zone { get; set; } = null!;
| public class DeviceTypeToIconConverter : IValueConverter | ||
| { | ||
| public object Convert(object? value, Type targetType, object? parameter, CultureInfo culture) | ||
| { | ||
| if (value == null) | ||
| { | ||
| System.Diagnostics.Debug.WriteLine($"DeviceType is NULL"); | ||
| return GetIconPath("default"); | ||
| } | ||
|
|
||
| var typeString = value.ToString(); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(typeString)) | ||
| { | ||
| System.Diagnostics.Debug.WriteLine($"DeviceType is empty/whitespace"); | ||
| return GetIconPath("default"); | ||
| } | ||
|
|
||
| var type = typeString.ToLowerInvariant().Trim(); | ||
|
|
||
| if (type.Contains("keyboard")) return GetIconPath("keyboard"); | ||
| if (type.Contains("mouse")) return GetIconPath("mouse"); | ||
| if (type.Contains("headset") || type.Contains("headphone")) return GetIconPath("headset"); | ||
| if (type.Contains("motherboard") || type.Contains("mainboard")) return GetIconPath("motherboard"); | ||
| if (type.Contains("gpu") || type.Contains("graphics") || type.Contains("video")) return GetIconPath("gpu"); | ||
| if (type.Contains("ram") || type.Contains("memory") || type.Contains("dram")) return GetIconPath("ram"); | ||
| if (type.Contains("cooler") || type.Contains("fan")) return GetIconPath("fan"); | ||
| if (type.Contains("strip") || type.Contains("led") || type.Contains("light")) return GetIconPath("led"); | ||
| if (type.Contains("case") || type.Contains("chassis")) return GetIconPath("case"); | ||
| if (type.Contains("speaker") || type.Contains("audio")) return GetIconPath("speaker"); | ||
| if (type.Contains("monitor") || type.Contains("display")) return GetIconPath("monitor"); | ||
| if (type.Contains("controller") || type.Contains("gamepad")) return GetIconPath("controller"); | ||
|
|
||
| return GetIconPath("default"); | ||
| } | ||
|
|
||
| private static Geometry GetIconPath(string iconType) | ||
| { | ||
| return iconType switch | ||
| { | ||
| "keyboard" => Geometry.Parse("M19,10H17V8H19M19,13H17V11H19M16,10H14V8H16M16,13H14V11H16M16,17H8V15H16M7,10H5V8H7M7,13H5V11H7M8,11H10V13H8M8,8H10V10H8M11,11H13V13H11M11,8H13V10H11M20,5H4C2.89,5 2,5.89 2,7V17A2,2 0 0,0 4,19H20A2,2 0 0,0 22,17V7C22,5.89 21.1,5 20,5Z"), | ||
| "mouse" => Geometry.Parse("M11,1.07C7.05,1.56 4,4.92 4,9H11M4,15A8,8 0 0,0 12,23A8,8 0 0,0 20,15V11H4M13,1.07V9H20C20,4.92 16.94,1.56 13,1.07Z"), | ||
| "headset" => Geometry.Parse("M12,1C7,1 3,5 3,10V17A3,3 0 0,0 6,20H9V12H5V10A7,7 0 0,1 12,3A7,7 0 0,1 19,10V12H15V20H19V21H12V23H18A3,3 0 0,0 21,20V10C21,5 16.97,1 12,1Z"), | ||
| "motherboard" => Geometry.Parse("M19,5V7H15V5H19M9,5V11H5V5H9M19,13V19H15V13H19M9,17V19H5V17H9M21,3H13V9H21V3M11,3H3V13H11V3M21,11H13V21H21V11M11,15H3V21H11V15Z"), | ||
| "gpu" => Geometry.Parse("M2,7V8.5H3V17H4.5V7C3.7,7 2.8,7 2,7M6,7V7L6,16H7V17H14V16H22V7H6M17.5,9A2.5,2.5 0 0,1 20,11.5A2.5,2.5 0 0,1 17.5,14A2.5,2.5 0 0,1 15,11.5A2.5,2.5 0 0,1 17.5,9Z"), | ||
| "ram" => Geometry.Parse("M22 19V16H2V19H22M22 11V6H2V11H3.5C4.33 11 5 11.67 5 12.5S4.33 14 3.5 14L2 14V15H22V14H20.5C19.67 14 19 13.33 19 12.5S19.67 11 20.5 11H22M13 13C13 13.55 12.55 14 12 14S11 13.55 11 13V11C11 10.45 11.45 10 12 10S13 10.45 13 11V13Z"), | ||
| "fan" => Geometry.Parse("M12,11A1,1 0 0,0 11,12A1,1 0 0,0 12,13A1,1 0 0,0 13,12A1,1 0 0,0 12,11M12.5,2C17,2 17.11,5.57 14.75,6.75C13.76,7.24 13.32,8.29 13.13,9.22C13.61,9.42 14.03,9.73 14.35,10.13C18.05,8.13 22.03,8.92 22.03,12.5C22.03,17 18.46,17.1 17.28,14.73C16.78,13.74 15.72,13.3 14.79,13.11C14.59,13.59 14.28,14 13.88,14.34C15.87,18.03 15.08,22 11.5,22C7,22 6.91,18.42 9.27,17.24C10.25,16.75 10.69,15.71 10.89,14.79C10.4,14.59 9.97,14.27 9.65,13.87C5.96,15.85 2,15.07 2,11.5C2,7 5.56,6.89 6.74,9.26C7.24,10.25 8.29,10.68 9.22,10.87C9.41,10.39 9.73,9.97 10.14,9.65C8.15,5.96 8.94,2 12.5,2Z"), | ||
| "led" => Geometry.Parse("M12,6A6,6 0 0,1 18,12C18,14.22 16.79,16.16 15,17.2V19A1,1 0 0,1 14,20H10A1,1 0 0,1 9,19V17.2C7.21,16.16 6,14.22 6,12A6,6 0 0,1 12,6M14,21V22A1,1 0 0,1 13,23H11A1,1 0 0,1 10,22V21H14M20,11H23V13H20V11M1,11H4V13H1V11M13,1V4H11V1H13M4.92,3.5L7.05,5.64L5.63,7.05L3.5,4.93L4.92,3.5M16.95,5.63L19.07,3.5L20.5,4.93L18.37,7.05L16.95,5.63Z"), | ||
| "case" => Geometry.Parse("M8,2H16A2,2 0 0,1 18,4V20A2,2 0 0,1 16,22H8A2,2 0 0,1 6,20V4A2,2 0 0,1 8,2M8,4V6H16V4H8M16,8H8V10H16V8M16,18H14V20H16V18Z"), | ||
| "speaker" => Geometry.Parse("M12,12A3,3 0 0,0 9,15A3,3 0 0,0 12,18A3,3 0 0,0 15,15A3,3 0 0,0 12,12M12,20A5,5 0 0,1 7,15A5,5 0 0,1 12,10A5,5 0 0,1 17,15A5,5 0 0,1 12,20M12,4A2,2 0 0,1 14,6A2,2 0 0,1 12,8C10.89,8 10,7.1 10,6C10,4.89 10.89,4 12,4M17,2H7C5.89,2 5,2.89 5,4V20A2,2 0 0,0 7,22H17A2,2 0 0,0 19,20V4C19,2.89 18.1,2 17,2Z"), | ||
| "monitor" => Geometry.Parse("M21,16H3V4H21M21,2H3C1.89,2 1,2.89 1,4V16A2,2 0 0,0 3,18H10V20H8V22H16V20H14V18H21A2,2 0 0,0 23,16V4C23,2.89 22.1,2 21,2Z"), | ||
| "controller" => Geometry.Parse("M7.97,16L5,19C4.67,19.3 4.23,19.5 3.75,19.5A1.75,1.75 0 0,1 2,17.75V17.5L3,10.12C3.21,7.81 5.14,6 7.5,6H16.5C18.86,6 20.79,7.81 21,10.12L22,17.5V17.75A1.75,1.75 0 0,1 20.25,19.5C19.77,19.5 19.33,19.3 19,19L16.03,16H7.97M7,8V10H5V11H7V13H8V11H10V10H8V8H7M16.5,8A0.75,0.75 0 0,0 15.75,8.75A0.75,0.75 0 0,0 16.5,9.5A0.75,0.75 0 0,0 17.25,8.75A0.75,0.75 0 0,0 16.5,8M14.75,9.75A0.75,0.75 0 0,0 14,10.5A0.75,0.75 0 0,0 14.75,11.25A0.75,0.75 0 0,0 15.5,10.5A0.75,0.75 0 0,0 14.75,9.75M18.25,9.75A0.75,0.75 0 0,0 17.5,10.5A0.75,0.75 0 0,0 18.25,11.25A0.75,0.75 0 0,0 19,10.5A0.75,0.75 0 0,0 18.25,9.75M16.5,11.5A0.75,0.75 0 0,0 15.75,12.25A0.75,0.75 0 0,0 16.5,13A0.75,0.75 0 0,0 17.25,12.25A0.75,0.75 0 0,0 16.5,11.5Z"), | ||
| _ => Geometry.Parse("M15,7V11H16V13H13V5H15L12,1L9,5H11V13H8V10.93C8.7,10.56 9.2,9.85 9.2,9C9.2,7.78 8.21,6.8 7,6.8C5.78,6.8 4.8,7.78 4.8,9C4.8,9.85 5.3,10.56 6,10.93V13A2,2 0 0,0 8,15H11V18.05C10.29,18.41 9.8,19.15 9.8,20A2.2,2.2 0 0,0 12,22.2A2.2,2.2 0 0,0 14.2,20C14.2,19.15 13.71,18.41 13,18.05V15H16A2,2 0 0,0 18,13V11H19V7H15Z") | ||
| }; | ||
| } | ||
|
|
||
| public object ConvertBack(object? value, Type targetType, object? parameter, CultureInfo culture) | ||
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The DeviceTypeToIconConverter is defined in the Views layer but follows the pattern seen with IntToBoolConverter which is defined in the ViewModels layer (YALCY/ViewModels/IntToBoolConverter.cs). For consistency with the existing codebase conventions, consider moving this converter to a separate file in the ViewModels namespace or creating a Converters folder under ViewModels.
| if (_mainViewModel != null) | ||
| { | ||
| Dispatcher.UIThread.InvokeAsync(() => _mainViewModel.ClearDevicesWithZones()); | ||
| } | ||
|
|
||
| foreach (var dev in devices) | ||
| { | ||
| OffList.Add(dev); | ||
| OpenRgbDeviceInserted?.Invoke(dev); | ||
|
|
||
| if (_mainViewModel != null) | ||
| { | ||
| Dispatcher.UIThread.InvokeAsync(() => | ||
| _mainViewModel.DevicesWithZones.Add(new DeviceWithZones(dev, _mainViewModel))); | ||
| } |
There was a problem hiding this comment.
The ClearDevicesWithZones() method is called via Dispatcher.UIThread.InvokeAsync (line 187), but the check for _mainViewModel != null happens outside the dispatcher call. If _mainViewModel becomes null between the check and the dispatcher execution (though unlikely), this could cause issues. For consistency with the pattern used for ClearOpenRgbVisualList (line 183), consider moving the null check inside the dispatcher callback or use the same pattern for both operations.
| if (_mainViewModel != null) | |
| { | |
| Dispatcher.UIThread.InvokeAsync(() => _mainViewModel.ClearDevicesWithZones()); | |
| } | |
| foreach (var dev in devices) | |
| { | |
| OffList.Add(dev); | |
| OpenRgbDeviceInserted?.Invoke(dev); | |
| if (_mainViewModel != null) | |
| { | |
| Dispatcher.UIThread.InvokeAsync(() => | |
| _mainViewModel.DevicesWithZones.Add(new DeviceWithZones(dev, _mainViewModel))); | |
| } | |
| Dispatcher.UIThread.InvokeAsync(() => | |
| { | |
| if (_mainViewModel != null) | |
| { | |
| _mainViewModel.ClearDevicesWithZones(); | |
| } | |
| }); | |
| foreach (var dev in devices) | |
| { | |
| OffList.Add(dev); | |
| OpenRgbDeviceInserted?.Invoke(dev); | |
| Dispatcher.UIThread.InvokeAsync(() => | |
| { | |
| var vm = _mainViewModel; | |
| if (vm != null) | |
| { | |
| vm.DevicesWithZones.Add(new DeviceWithZones(dev, vm)); | |
| } | |
| }); |
| _viewModel = viewModel; | ||
| } | ||
|
|
||
| public void SetViewModel(MainWindowViewModel viewModel) | ||
| { | ||
| _viewModel = viewModel; |
There was a problem hiding this comment.
The constructor sets _category to initialCategory (which is 0) but never calls UpdateDeviceCategoryList. This means newly created zones won't be added to any category list (e.g., OffZones) until the user explicitly changes the category through the UI. This differs from the behavior of DeviceCategory which has the same pattern. Consider whether zones should be automatically added to the OffZones list upon creation by calling UpdateDeviceCategoryList(initialCategory) at the end of the constructor.
| _viewModel = viewModel; | |
| } | |
| public void SetViewModel(MainWindowViewModel viewModel) | |
| { | |
| _viewModel = viewModel; | |
| _viewModel = viewModel; | |
| UpdateDeviceCategoryList(initialCategory); | |
| } | |
| public void SetViewModel(MainWindowViewModel viewModel) | |
| { | |
| _viewModel = viewModel; | |
| UpdateDeviceCategoryList(_category); |
| .UsePlatformDetect() | ||
| .WithInterFont() | ||
| .LogToTrace() | ||
| .LogToTrace() |
There was a problem hiding this comment.
There is trailing whitespace after .LogToTrace(). This should be removed for consistency with code formatting standards.
| .LogToTrace() | |
| .LogToTrace() |
| <!-- Connection Settings Card --> | ||
| <Border Grid.Column="0" x:Name="ConnectionCard" | ||
| Background="#101224" | ||
|
|
There was a problem hiding this comment.
There is an empty line (line 72) in the middle of the Border element's attribute list. This should be removed for consistency and cleaner code formatting.
| // Header | ||
| HeaderPanel.Margin = new Thickness(0, 0, 24, 0); | ||
| HeaderPanel.VerticalAlignment = VerticalAlignment.Center; | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. This should be removed for consistency with code formatting standards.
| foreach (var dev in devices) | ||
| { | ||
| OffList.Add(dev); | ||
| OpenRgbDeviceInserted?.Invoke(dev); | ||
|
|
||
| if (_mainViewModel != null) | ||
| { | ||
| Dispatcher.UIThread.InvokeAsync(() => | ||
| _mainViewModel.DevicesWithZones.Add(new DeviceWithZones(dev, _mainViewModel))); | ||
| } | ||
| } |
There was a problem hiding this comment.
When clearing OffList and updating the device list, only DevicesWithZones is being repopulated (line 197-198) but DeviceCategories is not. This creates an inconsistency - on initial connection (lines 122-127), both collections are populated, but on device list update, only DevicesWithZones is repopulated. This means the old DataGrid-based UI using DeviceCategories won't update when devices are hot-plugged. Either remove the line that adds to DeviceCategories during initial connection (since it appears deprecated) or also repopulate it here for consistency.
| LightPodStates.Clear(); | ||
| OffZones.Clear(); | ||
| LightPodZones.Clear(); | ||
| StrobeZones.Clear(); | ||
| FoggerZones.Clear(); | ||
| LightPodZoneStates.Clear(); |
There was a problem hiding this comment.
The clearing of zone-based dictionaries (lines 87-91) is not thread-safe, while the legacy LightPodStates dictionary is protected with a lock in other parts of the code (e.g., lines 349, 380, 458). If multiple threads access these dictionaries simultaneously (e.g., during reconnection while effects are running), this could lead to race conditions. Consider adding lock statements around dictionary operations for LightPodZoneStates similar to how LightPodStates is protected, or ensure that reconnection properly cancels all ongoing tasks before clearing.
| LightPodStates.Clear(); | |
| OffZones.Clear(); | |
| LightPodZones.Clear(); | |
| StrobeZones.Clear(); | |
| FoggerZones.Clear(); | |
| LightPodZoneStates.Clear(); | |
| lock (LightPodStates) | |
| { | |
| LightPodStates.Clear(); | |
| } | |
| lock (OffZones) | |
| { | |
| OffZones.Clear(); | |
| } | |
| lock (LightPodZones) | |
| { | |
| LightPodZones.Clear(); | |
| } | |
| lock (StrobeZones) | |
| { | |
| StrobeZones.Clear(); | |
| } | |
| lock (FoggerZones) | |
| { | |
| FoggerZones.Clear(); | |
| } | |
| lock (LightPodZoneStates) | |
| { | |
| LightPodZoneStates.Clear(); | |
| } |
| if ((parameter & (1 << (area - areaOffset))) != 0) | ||
| { | ||
| colors[ledIndex] = color; | ||
| } | ||
| else | ||
| { | ||
| colors[ledIndex] = new Color(0, 0, 0); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
|
TODO:
|
Adds OpenRGB zone support and enhances the UI for better device and zone management.
Key improvements: