cleanup: dead code removal, duplicate column fixes, broken icon fix, horizontal scroll for docked views, code quality audit, cancellation and scheduled task fixes, UI polish#162
Conversation
…ntal scroll to docked views Co-authored-by: efargas <9705611+efargas@users.noreply.github.com> Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/3e155473-191d-44aa-a34a-4ee6c6c1f4f6
Co-authored-by: efargas <9705611+efargas@users.noreply.github.com> Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/c0d0ef0b-cf98-4da5-af86-b1f2a8a50b8b
…toolbar organization, and set a default memory dump folder.
There was a problem hiding this comment.
Pull request overview
Improves docked-view usability and cleans up UI/code by adding horizontal scrolling where fixed-width toolbars/content overflow, fixing a few task-grid issues (duplicate columns + wrong command), correcting a broken sidebar icon binding, and removing unused/orphaned views and ViewModels.
Changes:
- Add horizontal scroll support to several docked Views (toolbars/header areas and memory dump list).
- Fix task DataGrid issues (remove duplicate “Job” columns; wire “Logs” action to the correct command).
- Remove dead code (unused View/ViewModel files) and adjust defaults/settings.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/S7Tools/Views/Tasks/TaskManagerView.axaml | Makes the header section horizontally scrollable to prevent overflow in narrow/docked layouts. |
| src/S7Tools/Views/Tasks/ScheduledTasksView.axaml | Removes redundant “Job” column that duplicated JobName. |
| src/S7Tools/Views/Tasks/HistoryTasksView.axaml | Removes redundant “Job” column that duplicated JobName. |
| src/S7Tools/Views/Tasks/ActiveTasksView.axaml | Removes redundant column and fixes the per-row action to open logs (with command parameter). |
| src/S7Tools/Views/Profiles/SocatProfilesView.axaml | Enables horizontal scrolling and restructures toolbar layout for better docking behavior. |
| src/S7Tools/Views/Profiles/SerialPortProfilesView.axaml | Enables horizontal scrolling and restructures toolbar layout for better docking behavior. |
| src/S7Tools/Views/Profiles/PowerSupplyProfilesView.axaml | Enables horizontal scrolling and restructures toolbar layout for better docking behavior. |
| src/S7Tools/Views/Profiles/MemoryRegionProfilesView.axaml | Enables horizontal scrolling; adjusts layout sizing and selected-profile details section. |
| src/S7Tools/Views/Pages/StreamedMemoryDumpView.axaml | Adds horizontal scrolling to the connection toolbar and memory blocks list. |
| src/S7Tools/Views/Pages/MemoryDumpSidebarView.axaml | Removes an invalid icon binding/converter usage that produced a broken icon. |
| src/S7Tools/Views/Pages/LogViewerView.axaml | Shares horizontal scroll between header + list to prevent overlap in docked layouts. |
| src/S7Tools/Views/Profiles/SerialProfileDetailView.axaml | Removes unused/orphaned view markup. |
| src/S7Tools/Views/Profiles/SerialProfileDetailView.axaml.cs | Removes unused/orphaned code-behind. |
| src/S7Tools/ViewModels/Layout/TabViewModel.cs | Removes unused/orphaned ViewModel. |
| src/S7Tools/ViewModels/Layout/NavigationItemViewModel.cs | Removes unused/orphaned ViewModel. |
| src/S7Tools/Resources/AppSettings/AppSettings.json | Changes memory dump default folder setting. |
| }, | ||
| "memoryDump": { | ||
| "defaultFolder": "", | ||
| "defaultFolder": "Resources/Dumps", |
There was a problem hiding this comment.
memoryDump.defaultFolder is now set to Resources/Dumps, but this directory does not appear to exist in the repo (and FileMemoryDumpViewModel only uses the setting if Directory.Exists), so the new default will be ignored and may confuse users. Either add/create this folder as part of the app’s resources on first run, or keep the default empty / point it at an existing writable directory.
| "defaultFolder": "Resources/Dumps", | |
| "defaultFolder": "Resources/Logs/Exported", |
There was a problem hiding this comment.
Must create the folder on init on first run add others for profiles or logs, it will be the default start point for file memory dump viewer file tree
There was a problem hiding this comment.
The Resources/Dumps directory is already created on first run by PathService.InitializeAsync() (it's in the directoriesToCreate array). The separate issue was that FileMemoryDumpViewModel called Directory.Exists on the raw relative string from settings, which is unreliable depending on the process CWD. Fixed in commit 2adfd5b — the ViewModel now resolves the path via IPathService.ResolvePath() before the existence check, so the pre-created Dumps directory is correctly found.
| <Grid ColumnDefinitions="120,*" RowDefinitions="Auto,Auto,Auto" RowSpacing="4"> | ||
| <TextBlock Grid.Row="0" Grid.Column="0" Text="Name:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
| <TextBlock Grid.Row="0" Grid.Column="1" Text="{Binding SelectedProfile.Name}" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
|
|
||
| <TextBlock Grid.Row="0" Grid.Column="2" Text="Total Segments:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" Margin="16,0,8,0" /> | ||
| <TextBlock Grid.Row="0" Grid.Column="3" Text="{Binding SelectedProfile.SegmentCount}" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
|
|
||
| <TextBlock Grid.Row="1" Grid.Column="0" Text="Description:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" Margin="0,4,8,0" /> | ||
| <TextBlock Grid.Row="1" Grid.Column="1" Grid.ColumnSpan="3" Text="{Binding SelectedProfile.Description}" Foreground="{DynamicResource TextForegroundSecondaryBrush}" Margin="0,4,0,0" /> | ||
|
|
||
| <TextBlock Grid.Row="2" Grid.Column="0" Text="Selected:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" Margin="0,4,8,0" /> | ||
| <TextBlock Grid.Row="2" Grid.Column="1" Foreground="{DynamicResource TextForegroundSecondaryBrush}" Margin="0,4,0,0"> | ||
| <TextBlock.Text> | ||
| <MultiBinding StringFormat="{}{0} of {1} segments"> | ||
| <Binding Path="SelectedProfile.Segments" Converter="{x:Static conv:CollectionConverters.CountSelectedSegments}" /> | ||
| <Binding Path="SelectedProfile.Segments.Count" /> | ||
| </MultiBinding> | ||
| </TextBlock.Text> | ||
| </TextBlock> | ||
| <TextBlock Grid.Row="1" Grid.Column="0" Text="Total Segments:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
| <TextBlock Grid.Row="1" Grid.Column="1" Text="{Binding SelectedProfile.SegmentCount}" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
|
|
||
| <TextBlock Grid.Row="2" Grid.Column="0" Text="Description:" FontWeight="SemiBold" Foreground="{DynamicResource TextForegroundSecondaryBrush}" /> | ||
| <TextBlock Grid.Row="2" Grid.Column="1" Text="{Binding SelectedProfile.Description}" Foreground="{DynamicResource TextForegroundSecondaryBrush}" TextWrapping="Wrap" /> | ||
| </Grid> |
There was a problem hiding this comment.
The selected-profile details grid was simplified and no longer shows the previously-present “Selected: {x} of {y} segments” summary for the profile. This functional UI change isn’t mentioned in the PR description; please confirm it’s intentional or re-add the selected-segments summary in the details section.
There was a problem hiding this comment.
It must show selected-segments summary as before.
The descripción must be limited to maxwidth if the text is too long, in that case have to be represented in multiple lines.
The order must be 1 Name, 2 Description, 3.1 total segments count 3.2 selected segments count (3.1 and 3.2 in same row to optimize space)
There was a problem hiding this comment.
Fixed in commit 2adfd5b. The "Selected Profile Details" section now shows:
- Name
- Description (with
TextWrapping="Wrap"so long text spans multiple lines) - Total Segments | Selected (both in the same row using a 4-column grid)
The grid uses ColumnDefinitions="Auto,*,Auto,Auto" so the description column expands to fill available width while the label columns size to content.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot look my replies to the comments and fix it as i said |
…mp folder via PathService Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/f18150e6-e586-4305-ac84-b5bf7c8131c4 Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
…sistency and usability across various settings and task views.
|
@copilot audit all source code, find dead code, unused code, duplicities, ensure best practices in dotnet development and Avalonia UI, optimizations, memory usage, garbage collector, io operations, UI progress updates, |
…use, fix Process.Start handles Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/f92ad28a-1fc1-4d93-a309-e2dbca0b42f8 Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Completed a full audit in commit Dead code removed:
Security: CodeQL found 0 alerts. All 184 tests pass (pre-existing failures in |
…configured minimum level Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/38ef2aef-91ed-41e4-befc-f81ed77b700c Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…le-size calculation (review #4032247855) Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/32ae571b-e33c-4409-b539-b9fbe2322d62 Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Applied in commit
Build: 0 warnings, 0 errors. All 185 tests pass. |
| // Mark task as completed | ||
| long totalLength = result.SavedFiles?.Sum(x => (long)x.Length) ?? 0; | ||
| long totalLength = result.SavedFiles? | ||
| .Sum(path => { try { return new System.IO.FileInfo(path).Length; } catch { return 0L; } }) ?? 0; | ||
| taskExecution.MarkAsCompleted(outputFilePath, totalLength); |
There was a problem hiding this comment.
The file-size aggregation swallows all exceptions and silently treats unreadable/missing dump files as 0 bytes. That can hide real failures (e.g., output path misconfiguration) and make troubleshooting harder. Consider catching only expected I/O exceptions and at least logging a warning/debug entry when a file can’t be stat’d, or failing the task if the output file(s) are missing after a reported success.
| using var waitCts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); | ||
| while (!stateChanges.Any(s => s.NewState == JobState.Failed) && !waitCts.Token.IsCancellationRequested) | ||
| { | ||
| await Task.Delay(50); | ||
| } |
There was a problem hiding this comment.
This test waits for the scheduler to reach the Failed state using a polling loop with Task.Delay. That makes the test timing-sensitive and potentially flaky under load/slow CI. Prefer using a TaskCompletionSource<JobStateChangedEventArgs> completed by the JobStateChanged event and awaiting it with a timeout, so the test is deterministic and finishes faster when the transition happens quickly.
|
@copilot apply changes based on the comments in this thread also fix the next issues i've encountered:
here attached ther logs from the session, forget about modbus connection fails, they are intentional: Task ID: 1d9bf641-c0c7-488c-ab75-df258521c755 Would you like to navigate to the Task Manager to view and start the task? Format: yyyy-MM-dd HH:mm (24-hour format) Task ID: a55a3677-8a4b-4ed0-be5a-01f54654ffad The task will run automatically at the scheduled time. Format: yyyy-MM-dd HH:mm (24-hour format) Task ID: 8cf1e154-f48b-4baf-9e39-24e561d8f8da The task will run automatically at the scheduled time. Format: yyyy-MM-dd HH:mm (24-hour format) Task ID: d0c87852-e472-4fd7-bbe2-096d5fecbba6 The task will run automatically at the scheduled time. |
…uled state, history refresh, icon buttons, dock navigation Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/0482f695-a7bc-4112-8128-4393e513826f Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Applied in commit From review thread #4033433154:
Reported bugs:
|
…cross all views. add expanders and icons to categories on views
…dardization, and removal of obsolete logging test views.
…ode consistency across services and view models
|
@copilot most of the changes are about format want explicit variable types, a party of this review the rest changes about layout and logging |
…sh() for deterministic tests Agent-Logs-Url: https://github.com/efargas/S7-Tools/sessions/30e12e8d-0abb-409b-88dd-ecb0f36c09d4 Co-authored-by: efargas <9705611+efargas@users.noreply.github.com>
Applied in commit
Build: 0 warnings, 0 errors. All 405 tests pass (198 + 185 + 22). CodeQL: 0 alerts. |
|
@gemini-code-assist /review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs a comprehensive cleanup of unused using directives across the project and introduces a new PartialDumpException to handle and preserve data from interrupted memory dump operations. Key UI enhancements include a redesigned Log Viewer and updated Home and About pages, alongside a refactored navigation system that improves sidebar behavior for document-centric views. Feedback was provided regarding a regression in the Log Viewer UI where header-based sorting functionality was omitted during the redesign, with a suggestion to re-implement it using the existing view model commands.
| <Border Grid.Row="0" Background="{DynamicResource ControlBackgroundBrush}" BorderBrush="{DynamicResource ControlBorderBrush}" BorderThickness="0,0,0,1"> | ||
| <Grid ColumnDefinitions="180,100,150,*" Margin="12,4"> | ||
| <TextBlock Grid.Column="0" Text="TIMESTAMP" FontSize="11" FontWeight="Bold" Opacity="0.6" /> | ||
| <TextBlock Grid.Column="1" Text="LEVEL" FontSize="11" FontWeight="Bold" Opacity="0.6" /> | ||
| <TextBlock Grid.Column="2" Text="CATEGORY" FontSize="11" FontWeight="Bold" Opacity="0.6" /> | ||
| <TextBlock Grid.Column="3" Text="MESSAGE" FontSize="11" FontWeight="Bold" Opacity="0.6" /> | ||
| </Grid> | ||
| </Border> |
There was a problem hiding this comment.
The new UI for the log viewer looks great! However, it seems the ability to sort the log entries by clicking on the column headers has been removed. The previous implementation had sort buttons next to each header title.
The SortCommand still exists in the LogViewerViewModel, so it seems this might be an unintentional omission. Could you please consider re-adding the sorting functionality to the new header design?
For example, you could wrap each TextBlock in a Button to make the headers clickable:
<Grid ColumnDefinitions="180,100,150,*" Margin="12,4">
<Button Grid.Column="0" Command="{Binding SortCommand}" CommandParameter="Timestamp" Classes="HeaderButton">
<TextBlock Text="TIMESTAMP" />
</Button>
<Button Grid.Column="1" Command="{Binding SortCommand}" CommandParameter="Level" Classes="HeaderButton">
<TextBlock Text="LEVEL" />
</Button>
<Button Grid.Column="2" Command="{Binding SortCommand}" CommandParameter="Category" Classes="HeaderButton">
<TextBlock Text="CATEGORY" />
</Button>
<Button Grid.Column="3" Command="{Binding SortCommand}" CommandParameter="Message" Classes="HeaderButton">
<TextBlock Text="MESSAGE" />
</Button>
</Grid>You would need to define a style for HeaderButton to make it look like a plain header but act like a button.
TaskSchedulerfailure-path message to include saved-file count (like cancelled branch)ScheduleTaskAsyncimmediate-promotion branch to clearProgressData["ScheduledTime"]DumpWithTaskTrackingAsyncfile-size calculation (useFileInfo.Length, not string.Length)BootloaderServicefile-size aggregation to log warnings for unreadable filesJobSchedulerTeststo useTaskCompletionSourcefor deterministic state detectiondotnet format: add braces to if-statements, fix whitespace (NavigationViewModel, TestSettingsViewModel, LogViewerView, etc.)DialogParentNotFoundExceptionTests.cs:.Subjectas statement;ExceptionTests.cs:.Should().Be()→.BeEquivalentTo()for collections)Flush()toILogDataStore+LogDataStoreso tests can synchronously flush the 100ms-timer-batched queue; fixes 9 failingLogDataStoreTests🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.