Add support for customizing display order of ATIS stations #139
Conversation
|
Warning Rate limit exceeded@justinshannon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThis pull request introduces a new sorting feature for ATIS stations. A dedicated dialog for sorting stations is added, along with its view model and corresponding UI layout. The service provider now registers the dialog and view model as transient services. Existing models and view models have been enhanced with an ordinal property and composite key logic to support ordering. Additionally, an event record is introduced to signal ordinal changes, and a behavior for auto-scrolling the TreeDataGrid to the selected item is implemented and integrated within existing dialogs and the main configuration window. Changes
Sequence Diagram(s)sequenceDiagram
participant ConfigVM as AtisConfigurationWindowViewModel
participant WindowFactory as IWindowFactory
participant SortDialog as SortAtisStationsDialog
participant SortVM as SortAtisStationsDialogViewModel
participant EventBus as Event Bus
ConfigVM->>WindowFactory: Invoke OpenSortAtisStationsDialogCommand
WindowFactory->>SortDialog: CreateSortAtisStationsDialog()
SortDialog->>SortVM: Initialize with view model
User->>SortDialog: Trigger move up/down commands
SortVM->>SortDialog: Update order of ATIS stations
SortVM->>EventBus: Publish AtisStationOrdinalChanged event
EventBus->>ConfigVM: Notify MainWindowViewModel for UI update
sequenceDiagram
participant TreeGrid as TreeDataGrid
participant ScrollBehavior as ScrollToSelectedItemBehavior
participant RowsPresenter as RowsPresenter
TreeGrid->>ScrollBehavior: SelectionChanged event occurs
ScrollBehavior->>RowsPresenter: Convert index and invoke ScrollToItemIndex
RowsPresenter-->>TreeGrid: Scroll to selected item
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
vATIS.Desktop/Profiles/Models/AtisStation.cs (1)
220-241:⚠️ Potential issueAdd the Ordinal property to the Clone method.
The
Ordinalproperty is not included in theClonemethod, which could lead to inconsistencies when cloning ATIS stations.Apply this diff to include the
Ordinalproperty in the cloned instance:public AtisStation Clone() { return new AtisStation { Id = Guid.NewGuid().ToString(), Identifier = Identifier, Name = Name, AtisType = AtisType, CodeRange = CodeRange, AtisFormat = AtisFormat.Clone(), NotamsBeforeFreeText = NotamsBeforeFreeText, AirportConditionsBeforeFreeText = AirportConditionsBeforeFreeText, Frequency = Frequency, IdsEndpoint = IdsEndpoint, UseDecimalTerminology = UseDecimalTerminology, AtisVoice = AtisVoice.Clone(), Presets = Presets.Select(x => x.Clone()).ToList(), Contractions = Contractions.Select(x => x.Clone()).ToList(), AirportConditionDefinitions = AirportConditionDefinitions.Select(x => x.Clone()).ToList(), NotamDefinitions = NotamDefinitions.Select(x => x.Clone()).ToList(), + Ordinal = Ordinal, }; }
🧹 Nitpick comments (5)
vATIS.Desktop/Ui/Dialogs/SortPresetsDialog.axaml (1)
40-42: Fix indentation for consistency.The indentation of the Interaction.Behaviors section doesn't match the surrounding code style.
- <Interaction.Behaviors> - <behaviors:ScrollToSelectedItemBehavior/> - </Interaction.Behaviors> + <Interaction.Behaviors> + <behaviors:ScrollToSelectedItemBehavior/> + </Interaction.Behaviors>vATIS.Desktop/Ui/Behaviors/ScrollToSelectedItemBehavior.cs (1)
31-59: Consider extracting index calculation logic into a separate method.The index calculation logic within the Observable chain is complex and would be more maintainable if extracted into a dedicated method.
Observable.FromEventPattern(rowSelection, nameof(rowSelection.SelectionChanged)) - .Select(_ => - { - // Retrieve the first selected index. - var selectedIndexPath = rowSelection.SelectedIndex.FirstOrDefault(); - if (AssociatedObject.Rows is null) - { - return selectedIndexPath; - } - - // Convert the logical index to the actual row index in the UI. - var rowIndex = AssociatedObject.Rows.ModelIndexToRowIndex(selectedIndexPath); - - // Adjust the index if the selected item is a child of a parent row. - if (rowSelection.SelectedIndex.Count > 1) - { - // Skip the first index (parent), sum the child indices, and adjust. - rowIndex += rowSelection.SelectedIndex.Skip(1).Sum(); - - // Add 1 to correct the index for proper positioning. - rowIndex += 1; - } - - return rowIndex; - }) + .Select(_ => CalculateRowIndex(rowSelection)) .WhereNotNull() .Do(ScrollToItemIndex) .Subscribe() .DisposeWith(disposable); + private int? CalculateRowIndex(ITreeDataGridRowSelection rowSelection) + { + var selectedIndexPath = rowSelection.SelectedIndex.FirstOrDefault(); + if (AssociatedObject.Rows is null) + { + return selectedIndexPath; + } + + var rowIndex = AssociatedObject.Rows.ModelIndexToRowIndex(selectedIndexPath); + + if (rowSelection.SelectedIndex.Count > 1) + { + rowIndex += rowSelection.SelectedIndex.Skip(1).Sum() + 1; + } + + return rowIndex; + }vATIS.Desktop/Ui/Dialogs/SortAtisStationsDialog.axaml (2)
46-47: Enhance button labels for better accessibility.Consider using more descriptive labels for the Up/Down buttons to improve accessibility and user experience.
-<Button Theme="{StaticResource Dark}" Command="{Binding MoveStationUpCommand, DataType=vm:SortAtisStationsDialogViewModel}">Up</Button> -<Button Theme="{StaticResource Dark}" Command="{Binding MoveStationDownCommand, DataType=vm:SortAtisStationsDialogViewModel}">Down</Button> +<Button Theme="{StaticResource Dark}" Command="{Binding MoveStationUpCommand, DataType=vm:SortAtisStationsDialogViewModel}">Move Up</Button> +<Button Theme="{StaticResource Dark}" Command="{Binding MoveStationDownCommand, DataType=vm:SortAtisStationsDialogViewModel}">Move Down</Button>
31-43: Add accessibility attributes to TreeDataGrid.The TreeDataGrid should have accessibility attributes to improve screen reader support.
-<TreeDataGrid Grid.Column="0" CornerRadius="5" BorderBrush="#646464" BorderThickness="1" MaxHeight="200" Source="{Binding Source, DataType=vm:SortAtisStationsDialogViewModel, Mode=OneWay}" CanUserResizeColumns="False" CanUserSortColumns="False"> +<TreeDataGrid Grid.Column="0" CornerRadius="5" BorderBrush="#646464" BorderThickness="1" MaxHeight="200" Source="{Binding Source, DataType=vm:SortAtisStationsDialogViewModel, Mode=OneWay}" CanUserResizeColumns="False" CanUserSortColumns="False" + AutomationProperties.Name="ATIS Stations List" + AutomationProperties.HelpText="Use Up and Down buttons to reorder stations">vATIS.Desktop/Ui/ViewModels/MainWindowViewModel.cs (1)
115-116: Update the comment to accurately reflect the composite key components.The comment doesn't mention all components of the composite key.
Apply this diff to improve the comment accuracy:
-// Generate composite keys using Identifier + Ordinal +// Generate composite keys using Identifier + AtisType + Ordinal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
vATIS.Desktop/Container/Factory/WindowFactory.cs(1 hunks)vATIS.Desktop/Container/ServiceProvider.cs(2 hunks)vATIS.Desktop/Events/AtisStationOrdinalChanged.cs(1 hunks)vATIS.Desktop/Profiles/Models/AtisStation.cs(1 hunks)vATIS.Desktop/Ui/Behaviors/ScrollToSelectedItemBehavior.cs(1 hunks)vATIS.Desktop/Ui/Dialogs/SortAtisStationsDialog.axaml(1 hunks)vATIS.Desktop/Ui/Dialogs/SortAtisStationsDialog.axaml.cs(1 hunks)vATIS.Desktop/Ui/Dialogs/SortPresetsDialog.axaml(2 hunks)vATIS.Desktop/Ui/IWindowFactory.cs(1 hunks)vATIS.Desktop/Ui/ViewModels/AtisConfigurationWindowViewModel.cs(4 hunks)vATIS.Desktop/Ui/ViewModels/AtisStationViewModel.cs(3 hunks)vATIS.Desktop/Ui/ViewModels/MainWindowViewModel.cs(7 hunks)vATIS.Desktop/Ui/ViewModels/SortAtisStationsDialogViewModel.cs(1 hunks)vATIS.Desktop/Ui/Windows/AtisConfigurationWindow.axaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (11)
vATIS.Desktop/Events/AtisStationOrdinalChanged.cs (1)
8-17: Well-structured event record implementation!The record type is a good choice for this immutable event data, and the XML documentation clearly describes the purpose of each parameter.
vATIS.Desktop/Ui/Dialogs/SortAtisStationsDialog.axaml.cs (1)
16-49: Clean and well-structured dialog implementation!The dialog follows best practices with proper cleanup, window drag functionality, and clear separation of concerns.
vATIS.Desktop/Ui/IWindowFactory.cs (1)
101-105: LGTM!The new factory method follows the established pattern and is well-documented.
vATIS.Desktop/Container/ServiceProvider.cs (1)
75-75: LGTM!The new service registrations follow the established pattern and are correctly registered as transient services.
Also applies to: 100-100
vATIS.Desktop/Container/Factory/WindowFactory.cs (1)
200-210: LGTM! The implementation follows the established factory pattern.The new method
CreateSortAtisStationsDialogis well-implemented, following the same pattern as other dialog creation methods in the class. It properly retrieves the view model from the service provider and initializes the dialog with it.vATIS.Desktop/Ui/Windows/AtisConfigurationWindow.axaml (1)
107-110: LGTM! The UI changes are well-structured and consistent.The grid layout modifications and the addition of the "Sort" button are well-implemented:
- Grid layout properly expanded to 3 columns
- Buttons are consistently sized with "Stretch" alignment
- Commands are correctly bound to their respective view model commands
vATIS.Desktop/Profiles/Models/AtisStation.cs (1)
27-30: LGTM! The Ordinal property is well-documented.The new property is properly documented with XML comments.
vATIS.Desktop/Ui/ViewModels/MainWindowViewModel.cs (1)
199-206: LGTM! The event subscription is well-implemented.The subscription to
AtisStationOrdinalChangedevents properly updates theOrdinalproperty of the corresponding station.vATIS.Desktop/Ui/ViewModels/AtisConfigurationWindowViewModel.cs (1)
129-133: LGTM!The sorting logic correctly prioritizes the
Ordinalproperty while maintainingIdentifierandAtisTypeas secondary sort keys.vATIS.Desktop/Ui/ViewModels/AtisStationViewModel.cs (2)
76-76: LGTM!The ordinal field is correctly declared and initialized from the station parameter.
Also applies to: 140-140
596-603: LGTM!The property is well-documented and correctly implements change notifications using
RaiseAndSetIfChanged.
vATIS.Desktop/Ui/ViewModels/AtisConfigurationWindowViewModel.cs
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit