[WIP] Explore device's macros - #264
Draft
vicocz wants to merge 6 commits into
Draft
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional issues in the macro feature (mismatched translation keys, missing macro name resource, and command enablement not updating on connect/disconnect) plus a likely build/config risk from using the field backing keyword.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an initial “device macros” concept and UI so users can browse and execute macros exposed by connected devices (starting with BuWizz/BuWizz2 and PFx Brick), alongside refactoring PFx protocol code into the FxBricks area.
Changes:
- Add macro domain types (
MacroDescriptor,MacroChoice,MacroInvocation, etc.) and surface macros from supported devices. - Add a macros view to
DevicePagewith toolbar navigation between Channels / Sensors / Macros and a macro execution flow. - Move/expand PFx protocol implementation under
DeviceManagement/FxBricksand adjust tests/namespaces accordingly.
File summaries
| File | Description |
|---|---|
| BrickController2/BrickController2/UI/ViewModels/MacroItemViewModel.cs | New view-model wrapper for displaying a macro descriptor with translated name. |
| BrickController2/BrickController2/UI/ViewModels/DevicePageViewModel.cs | Adds macro list + view switching flags/commands + macro execution dialog flow. |
| BrickController2/BrickController2/UI/Pages/DevicePage.xaml | Adds Macros UI section and secondary toolbar items to switch views and execute macros. |
| BrickController2/BrickController2/Resources/TranslationResources*.resx | Adds new macro-related translation keys and BuWizz level labels (partial locale coverage). |
| BrickController2/BrickController2/Protocols/PfxProtocol.cs | Removes the old PFx protocol location (moved under FxBricks). |
| BrickController2/BrickController2/DeviceManagement/Macros/*.cs | New macro domain model types. |
| BrickController2/BrickController2/DeviceManagement/FxBricks/PfxProtocol.cs | New/expanded PFx protocol implementation (file directory + sound control, parsing helpers). |
| BrickController2/BrickController2/DeviceManagement/FxBricks/PfxFileDirEntry.cs | New record for PFx file directory entry parsing results. |
| BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDeviceManager.cs | Namespace moved under DeviceManagement.FxBricks. |
| BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs | Adds PFx macros (sound-file selection) and directory scan to populate choices. |
| BrickController2/BrickController2/DeviceManagement/DI/DeviceManagementModule.cs | Adds FxBricks using to match new namespace. |
| BrickController2/BrickController2/DeviceManagement/Device.cs | Adds macro capability surface to base Device. |
| BrickController2/BrickController2/DeviceManagement/BuwizzDevice.cs | Adds static macro for output level selection + execution hook. |
| BrickController2/BrickController2/DeviceManagement/BuWizz2Device.cs | Adds static macro for output level selection + execution hook. |
| BrickController2/BrickController2.Tests/DeviceManagement/FxBricks/PfxProtocolTests.cs | Updates references/namespaces for moved PFx protocol. |
| BrickController2/BrickController2.Tests/DeviceManagement/FxBricks/PfxBrickDeviceManagerTests.cs | Updates references/namespaces for moved PFx manager. |
Review details
Files not reviewed (1)
- BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file
Suppressed comments (5)
BrickController2/BrickController2/UI/ViewModels/DevicePageViewModel.cs:135
- These properties use the
fieldkeyword as an implicit backing field. The rest of the codebase uses explicit backing fields (e.g., UI/ViewModels/ScannerPageViewModelBase.cs), andfieldrequires a newer language feature that may not be enabled for this project.
public bool ShowChannelView
{
get;
set
{
BrickController2/BrickController2/UI/ViewModels/DevicePageViewModel.cs:147
- These properties use the
fieldkeyword as an implicit backing field. The rest of the codebase uses explicit backing fields (e.g., UI/ViewModels/ScannerPageViewModelBase.cs), andfieldrequires a newer language feature that may not be enabled for this project.
public bool ShowMacroView
{
get;
set
{
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:17
- PlaySoundMacroNameKey references a translation key ("PfxPlaySoundMacroName") that isn't present in the translation resources, so the UI will display the raw key text.
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:275 - A PlaySound macro is added even when no sound files are found (choices is empty). In that case the macro appears in the UI but does nothing when executed (choiceValue stays null and ExecuteMacroAsync completes without writing).
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:75 invocation.ChoiceValueis the string "soundId" (FileId.ToString()), not a file name; naming itfileNameis misleading and makes the lookup logic harder to follow.
- Files reviewed: 22/23 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+37
to
+41
| new MacroChoice<int>("MacroChoice_BuWizz_Low", (int)BuWizz2OutputLevels.Low), | ||
| new MacroChoice<int>("MacroChoice_BuWizz_Normal", (int)BuWizz2OutputLevels.Normal), | ||
| new MacroChoice<int>("MacroChoice_BuWizz_High", (int)BuWizz2OutputLevels.High), | ||
| new MacroChoice<int>("MacroChoice_BuWizz_Ludicrous", (int)BuWizz2OutputLevels.Ludicrous), | ||
| ]) |
Comment on lines
+34
to
+36
| new MacroChoice<int>("MacroChoice_BuWizz_Low", (int)BuWizzOutputLevels.Low), | ||
| new MacroChoice<int>("MacroChoice_BuWizz_Normal", (int)BuWizzOutputLevels.Normal), | ||
| new MacroChoice<int>("MacroChoice_BuWizz_High", (int)BuWizzOutputLevels.High) |
Comment on lines
+67
to
+72
| SwitchToChannelViewCommand = new SafeCommand(() => SwitchView(showChannels:true), () => CanSwitchToChannelView); | ||
| SwitchToSensorViewCommand = new SafeCommand(() => SwitchView(showInputs: true), () => CanSwitchToSensorView); | ||
| SwitchToMacroViewCommand = new SafeCommand(() => SwitchView(showMacros: true), () => CanSwitchToMacroView); | ||
| ExecuteMacroCommand = new SafeCommand<MacroItemViewModel>( | ||
| ExecuteMacroAsync, | ||
| _ => Device.DeviceState == DeviceState.Connected && !_dialogService.IsDialogOpen); |
|
|
||
| public ObservableCollection<MacroItemViewModel> Macros { get; } = []; | ||
|
|
||
| public bool IsChannelDeviceDevice => Device.HasOutputChannel; |
There was a problem hiding this comment.
🟡 Changes recommended
Macro labels, PFx file filtering, empty macro behavior, and command-state updates contain user-visible defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- BrickController2/BrickController2/Resources/TranslationResources.Designer.cs: Generated file
Suppressed comments (4)
BrickController2/BrickController2/DeviceManagement/BuwizzDevice.cs:36
- These label keys do not match the resources added by this PR (
BuWizz_Low,BuWizz_Normal, andBuWizz_High).TranslationHelpertherefore falls back to displaying the literalMacroChoice_BuWizz_*identifiers in the selection dialog. Use the resource keys that actually exist.
new MacroChoice<int>("MacroChoice_BuWizz_Low", (int)BuWizzOutputLevels.Low),
new MacroChoice<int>("MacroChoice_BuWizz_Normal", (int)BuWizzOutputLevels.Normal),
new MacroChoice<int>("MacroChoice_BuWizz_High", (int)BuWizzOutputLevels.High)
BrickController2/BrickController2/DeviceManagement/BuWizz2Device.cs:40
- These label keys do not match the newly added
BuWizz_*resources, so the choice dialog displays the rawMacroChoice_BuWizz_*identifiers instead of translated labels. Use the existing resource keys.
new MacroChoice<int>("MacroChoice_BuWizz_Low", (int)BuWizz2OutputLevels.Low),
new MacroChoice<int>("MacroChoice_BuWizz_Normal", (int)BuWizz2OutputLevels.Normal),
new MacroChoice<int>("MacroChoice_BuWizz_High", (int)BuWizz2OutputLevels.High),
new MacroChoice<int>("MacroChoice_BuWizz_Ludicrous", (int)BuWizz2OutputLevels.Ludicrous),
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:314
- Every valid directory entry is currently offered as playable audio, including text, archive, image, and configuration files represented by
PfxFileFormat. Also, converting the 16-bit file ID tobytesilently redirects IDs above 255 to another file. Restrict choices to supported audio formats and IDs representable by the sound command.
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:317 - Play/stop descriptors are added even when the device has no selectable sound files. Since the UI skips selection for an empty choice list, pressing either macro then reports success while
ExecuteMacroAsyncsilently performs no operation. Only expose these macros when at least one sound choice exists.
- Files reviewed: 22/23 changed files
- Comments generated: 4
- Review effort level: Balanced
| _translationService = translationService; | ||
| _dialogService = dialogService; | ||
|
|
||
| ExecuteMacroCommand = new SafeCommand(ExecuteMacroAsync, () => _device.DeviceState == DeviceState.Connected && !_dialogService.IsDialogOpen); |
|
|
||
| public ICommand ExecuteMacroCommand { get; } | ||
|
|
||
| public int Idx => Math.Abs(_descriptor.Id.GetHashCode()); |
| /// exactly (no extra echoed request-code byte, unlike the "Get File Count" response). An entry with | ||
| /// <see cref="PfxFileDirEntry.FirstSector"/> == 0xFFFF is an empty/unused directory slot. | ||
| /// </remarks> | ||
| public static PfxFileDirEntry? ParseFileDirEntry(byte[] data) |
| { | ||
| await _dialogService.ShowMessageBoxAsync( | ||
| Translate("Warning"), | ||
| Translate("ExecuteMacroFailed") + ": " + ex.Message, |
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.