DashboardViewModel#6
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements runtime UAC elevation instead of always requiring administrator privileges at startup, and enhances the Event IDs dashboard chart with proper axes configuration. The changes allow the application to request elevation only when needed while improving chart readability through dedicated axis labels.
- Replaced manifest-based
requireAdministratorwith runtime admin detection and elevation prompting - Added X and Y axes properties with labels and styling to the Event IDs chart for better visualization
- Updated application version to 1.4.1.0
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| LogAnalyzerForWindows/app.manifest | Changed execution level from requireAdministrator to asInvoker and removed documentation comments |
| LogAnalyzerForWindows/Program.cs | Added runtime admin detection (IsRunningAsAdmin) and elevation logic (TryRestartAsAdmin) with UAC prompt handling |
| LogAnalyzerForWindows/ViewModels/DashboardViewModel.cs | Added EventIdsXAxes and EventIdsYAxes properties with initialization and update logic for the Event IDs chart |
| LogAnalyzerForWindows/Views/DashboardView.axaml | Bound new EventIds axes properties to the CartesianChart control |
| LogAnalyzerForWindows/LogAnalyzerForWindows.csproj | Bumped version numbers from 1.4.0.0 to 1.4.1.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Labels = [], | ||
| LabelsPaint = new SolidColorPaint(SKColors.LightGray), | ||
| TextSize = 12 | ||
| } | ||
| ]; | ||
|
|
||
| EventIdsXAxes = | ||
| [ | ||
| new Axis | ||
| { | ||
| Name = "Count", | ||
| NamePaint = new SolidColorPaint(SKColors.White), | ||
| LabelsPaint = new SolidColorPaint(SKColors.LightGray), | ||
| MinLimit = 0 | ||
| } |
There was a problem hiding this comment.
Code duplication detected: The initialization logic for EventIds axes in the InitializeChartAxes method duplicates the same logic that will be executed in UpdateTopEventIdsChart. The axes configuration (including Name, NamePaint, LabelsPaint, MinLimit, and TextSize) is set here and then completely replaced in UpdateTopEventIdsChart when data is loaded. This creates maintainability issues as changes to axis styling need to be made in two places.
Consider removing the EventIds axes initialization from InitializeChartAxes entirely, or keeping only a minimal initialization (empty Labels array) and moving the shared configuration to a helper method that both InitializeChartAxes and UpdateTopEventIdsChart can use.
| Labels = [], | |
| LabelsPaint = new SolidColorPaint(SKColors.LightGray), | |
| TextSize = 12 | |
| } | |
| ]; | |
| EventIdsXAxes = | |
| [ | |
| new Axis | |
| { | |
| Name = "Count", | |
| NamePaint = new SolidColorPaint(SKColors.White), | |
| LabelsPaint = new SolidColorPaint(SKColors.LightGray), | |
| MinLimit = 0 | |
| } | |
| // Initialize with empty labels; full styling will be applied when data is loaded. | |
| Labels = [] | |
| } | |
| ]; | |
| EventIdsXAxes = | |
| [ | |
| // Minimal initialization; full styling will be applied when data is loaded. | |
| new Axis() |
| FileName = exePath, | ||
| UseShellExecute = true, | ||
| Verb = "runas", | ||
| Arguments = string.Join(" ", Environment.GetCommandLineArgs().Skip(1)) |
There was a problem hiding this comment.
The command-line arguments reconstruction is incorrect. Using Environment.GetCommandLineArgs().Skip(1) will skip the first argument passed to the application, not the executable path. This could lead to missing arguments when the application restarts with elevation.
For proper argument forwarding, you should either use the args parameter directly (string.Join(" ", args)) which already excludes the executable path, or if you need to handle arguments with spaces properly, use string.Join(" ", args.Select(a => a.Contains(' ') ? $""{a}"" : a)).
| if (TryRestartAsAdmin()) | ||
| { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential infinite restart loop if elevation is required. When TryRestartAsAdmin successfully launches the elevated process and returns true, the current process exits. However, if the elevated process also fails the admin check (which shouldn't happen but could in edge cases), it would attempt to elevate again. Additionally, the --no-elevate flag is not automatically added when restarting, so if the user cancels the UAC prompt (error 1223), the application continues running without admin privileges but will attempt elevation again on next start.
Consider adding the --no-elevate flag when restarting to prevent repeated UAC prompts if the user cancels, or documenting this behavior clearly.
| if (topEventIds.Count == 0) | ||
| { | ||
| TopEventIdsSeries = []; | ||
| EventIdsYAxes = [new Axis { Labels = [] }]; |
There was a problem hiding this comment.
Inconsistent axis reset behavior in empty data handling. When topEventIds.Count == 0, only EventIdsYAxes is reset but EventIdsXAxes is not. This is inconsistent with the UpdateTopSourcesChart method which doesn't reset any axes when data is empty, and also inconsistent within the same method. For consistency and to prevent displaying stale axis labels, both EventIdsXAxes and EventIdsYAxes should be reset when there's no data, similar to how both axes are updated together when data is present.
| EventIdsYAxes = [new Axis { Labels = [] }]; | |
| EventIdsYAxes = []; | |
| EventIdsXAxes = []; |
This pull request introduces two main improvements: it adds a programmatic User Account Control (UAC) elevation mechanism for running the app as administrator (instead of always requiring admin via manifest), and it enhances the Event IDs chart in the dashboard with proper axes and labels for better clarity and maintainability.
Application startup and permissions:
requireAdministratormanifest setting. This allows the app to start normally without admin unless needed, improving user experience and flexibility. [1] [2] [3] [4]Dashboard chart improvements:
EventIdsXAxes,EventIdsYAxes) for the Event IDs chart in the dashboard, including proper labels and styling for improved readability. [1] [2] [3] [4] [5]Other:
1.4.1.0in the project file to reflect these changes.