[WIP] Add support for channel macro actions. - #265
Draft
vicocz wants to merge 5 commits into
Draft
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Macro persistence currently breaks SQLite mapping, and several validation, UI state, refresh, and localization paths are incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds channel macro actions to controller profiles and refactors dynamic macro discovery.
Changes:
- Adds macro selection, persistence, validation, and playback.
- Introduces reusable Bluetooth macro caching/discovery.
- Refactors PFx macro support and protocol namespace.
File summaries
| File | Description |
|---|---|
DevicePageViewModel.cs |
Refreshes dynamic macros. |
ControllerProfilePageViewModel.cs |
Handles missing-macro validation. |
ControllerActionPageViewModel.cs |
Adds macro-action configuration. |
ControllerActionPage.xaml |
Adds macro selector controls. |
PfxProtocol.cs |
Moves the PFx protocol namespace. |
PfxBrickDevice.cs |
Adopts dynamic macro discovery. |
Device.cs |
Adds the macro retrieval API. |
BluetoothDeviceWithMacros.cs |
Implements macro caching. |
ICreationManager.cs |
Extends action persistence parameters. |
CreationManager.cs |
Stores macro selections. |
Creation.cs |
Collects macro references. |
ControllerButtonType.cs |
Adds the Macro action type. |
ControllerAction.cs |
Adds macro fields. |
PlayLogic.cs |
Validates and invokes macros. |
CreationValidationResult.cs |
Adds missing-macro status. |
PfxProtocolTests.cs |
Updates the protocol namespace. |
Review details
Suppressed comments (3)
BrickController2/BrickController2/UI/Pages/ControllerActionPage.xaml:243
MacroChoiceis not defined in any translation resource, so this label renders the raw key in every locale. Add the key to all resource files.
<Label Grid.Column="0" Text="{extensions:Translate MacroChoice}" VerticalOptions="Center"/>
BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:444
SelectMacrois not present in any translation resource, so the selection dialog title is displayed as the raw key. Add it to all localization resource files.
var result = await _dialogService.ShowSelectionDialogAsync(
labels,
Translate("SelectMacro"),
Translate("Cancel"),
DisappearingToken);
BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:475
SelectMacroChoiceis not present in any translation resource, so the selection dialog title is displayed as the raw key. Add it to all localization resource files.
var result = await _dialogService.ShowSelectionDialogAsync(
labels,
Translate("SelectMacroChoice"),
Translate("Cancel"),
DisappearingToken);
- Files reviewed: 16/16 changed files
- Comments generated: 13
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+129
to
+133
| public object? MacroChoiceValue | ||
| { | ||
| get { return _macroChoiceValue; } | ||
| set { _macroChoiceValue = value; RaisePropertyChanged(); } | ||
| } |
Comment on lines
+194
to
+195
| protected override ValueTask<IReadOnlyList<MacroDescriptor>> DiscoverDynamicMacrosAsync(CancellationToken token) | ||
| => GetAvailableMacrosAsync(token); |
Comment on lines
+452
to
+456
| Action.MacroId = macro.Id; | ||
| SetSelectedChoice(macro.Choices.Count > 0 ? macro.Choices[0] : null); | ||
| RaisePropertyChanged(nameof(SelectedMacro)); | ||
| RaisePropertyChanged(nameof(SelectedMacroDisplayName)); | ||
| RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName)); |
Comment on lines
+496
to
+502
| await device.ConnectAsync( | ||
| false, | ||
| (_) => { }, | ||
| [], | ||
| startOutputProcessing: false, | ||
| false, | ||
| token); |
Comment on lines
+504
to
+507
| if (device.DeviceState == DeviceState.Connected) | ||
| { | ||
| await device.GetMacrosAsync(forceRefresh: true, token); | ||
| } |
Comment on lines
+529
to
+534
| Action.MacroChoiceValue = (choice?.BoxedValue) switch | ||
| { | ||
| int intValue => intValue, | ||
| string stringValue => stringValue, | ||
| _ => null, | ||
| }; |
added 2 commits
September 10, 2026 17:46
There was a problem hiding this comment.
🟡 Changes recommended
Macro persistence can break SQLite initialization, and several macro selection and execution paths contain functional 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 (5)
Previously missed (1) — in code that hasn't changed since the last review.
BrickController2/BrickController2/DeviceManagement/FxBricks/PfxBrickDevice.cs:171
- The stop command runs only after disabling notifications. If
DisableNotificationAsyncthrows,BluetoothDevice.DisconnectInternalAsyncswallows the exception and proceeds to disconnect without sendingAllOff, leaving outputs active despite this method's stated guarantee. SendAllOffbefore notification cleanup (or put it in afinally).
BrickController2/BrickController2/CreationManagement/ControllerAction.cs:132
ControllerActionis mapped directly by sqlite-net (CreationRepository.InitAsynccallsCreateTableAsync<ControllerAction>()), but sqlite-net cannot map a property declared asobject. Adding this property therefore makes table creation/migration fail before creations can be loaded, even when no macro action exists. Store the choice in supported columns (for example, a serialized text blob plus type information) and expose the boxed value as an ignored convenience property; the import/export path must preserve the same type as well.
public object? MacroChoiceValue
{
get { return _macroChoiceValue; }
set { _macroChoiceValue = value; RaisePropertyChanged(); }
BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:456
- Selecting a macro changes this command's predicate from false to true, but
SafeCommandonly updates bound controls whenRaiseCanExecuteChangedis called. For a newly created action, the macro-choice button therefore remains disabled after choosing a macro with choices.
RaisePropertyChanged(nameof(SelectedMacro));
RaisePropertyChanged(nameof(SelectedMacroDisplayName));
RaisePropertyChanged(nameof(SelectedMacroChoiceDisplayName));
BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:473
SelectMacroChoiceis not defined in the translation resources, so this title is guaranteed to render as the raw resource key. Add the corresponding neutral/localized resource (and regenerate the designer).
Translate("SelectMacroChoice"),
BrickController2/BrickController2/UI/ViewModels/ControllerActionPageViewModel.cs:546
MacroChoice<T>permits any struct value, and the existing macro catalog already usesfloat, but this conversion silently replaces every type exceptintandstringwithnull. Preserve the descriptor's boxed value so valid choice types are not discarded.
Action.MacroChoiceValue = (choice?.BoxedValue) switch
{
int intValue => intValue,
string stringValue => stringValue,
_ => null,
};
- Files reviewed: 21/22 changed files
- Comments generated: 3
- Review effort level: Balanced
|
|
||
| private static void InvokeMacro(ControllerAction controllerAction, Device device, CancellationToken token = default) | ||
| { | ||
| var macro = device.AvailableMacros.FirstOrDefault(m => m.Id == controllerAction.MacroId); |
| public System.Collections.Generic.IReadOnlyList<MacroDescriptor> AvailableMacros | ||
| => _selectedDevice?.AvailableMacros.Where(m => m.Scope == MacroScope.Channel).ToList() ?? []; | ||
|
|
||
| public bool HasMacros => _selectedDevice?.SupportsMacros == true; |
|
|
||
| var result = await _dialogService.ShowSelectionDialogAsync( | ||
| labels, | ||
| Translate("SelectMacro"), |
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.