refactor(sonar): addresses sonarqube issues#21
Conversation
44ef2e5 to
891169a
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the WPF UI layer and the ProcSim.Core simulation engine to address SonarQube findings, mainly via API/name cleanups, extracted types, converter refactors, and improved dispose patterns.
Changes:
- Standardizes core type naming (
CPU→Cpu,PCB→Pcb) across the scheduler/kernel/dispatcher and related IO/syscall flows. - Extracts enums and nested view-model types into standalone files and replaces
Equals-style comparisons withValueEquals. - Refactors WPF converters and applies/adjusts Sonar suppressions + disposal patterns to reduce warnings and improve resource cleanup.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ProcSim/Views/SimulationControlView.xaml.cs | Removes BOM/formatting noise; adds scoped Sonar warning suppression around an event handler. |
| src/ProcSim/Views/ProcessesConfigControl.xaml.cs | Adds click-to-deselect logic and a visual-tree helper; suppresses Sonar S2325 for an event handler. |
| src/ProcSim/Views/ProcessesConfigControl.xaml | Wires PreviewMouseLeftButtonDown handler and names the DataGrid for selection clearing. |
| src/ProcSim/ViewModels/VmConfigViewModel.cs | Updates EnumDescriptionConverter.Convert call to match IValueConverter signature. |
| src/ProcSim/ViewModels/ProcessesConfigViewModel.cs | Changes CanSave() gating logic for SaveCommand. |
| src/ProcSim/ViewModels/ProcessConfigViewModel.cs | Extracts LoopType, seals class, and replaces Equals with ValueEquals. |
| src/ProcSim/ViewModels/OperationType.cs | New extracted OperationType enum with descriptions. |
| src/ProcSim/ViewModels/OperationConfigViewModel.cs | Extracts nested types; seals class; changes equality to ValueEquals. |
| src/ProcSim/ViewModels/LoopType.cs | New extracted LoopType enum with descriptions. |
| src/ProcSim/ViewModels/IoOperationConfigViewModel.cs | New extracted IO operation config VM; adds ValueEquals. |
| src/ProcSim/ViewModels/CpuOperationConfigViewModel.cs | New extracted CPU operation config VM; adds ValueEquals. |
| src/ProcSim/Converters/InvertBooleanConverter.cs | Deduplicates invert logic and implements ConvertBack using the same helper. |
| src/ProcSim/Converters/EnumDescriptionConverter.cs | Aligns Convert signature with IValueConverter and retains ConvertBack logic. |
| src/ProcSim/Converters/BooleanToVisibilityConverter.cs | Simplifies branching; keeps parameter-based inversion behavior. |
| src/ProcSim/Assets/Settings.cs | Removes generated comment boilerplate / unused handlers. |
| src/ProcSim.Core/Syscall/SystemCallDispatcher.cs | Updates API types to Cpu/Pcb. |
| src/ProcSim.Core/Simulation/SimulationController.cs | Marks BuildCpuInstructions as static. |
| src/ProcSim.Core/Scheduler/SchedulerFactory.cs | Updates factory signature to use Pcb. |
| src/ProcSim.Core/Scheduler/RoundRobinScheduler.cs | Updates scheduler types to Cpu/Pcb. |
| src/ProcSim.Core/Scheduler/PriorityScheduler.cs | Updates scheduler types to Cpu/Pcb. |
| src/ProcSim.Core/Scheduler/IScheduler.cs | Updates interface to use Cpu/Pcb. |
| src/ProcSim.Core/Process/PCB.cs | Renames class/ctor to Pcb. |
| src/ProcSim.Core/Process/MicroOp.cs | Updates Action<> generic from CPU to Cpu. |
| src/ProcSim.Core/Process/Factories/MicroOpFactory.cs | Updates dispatcher/scheduler interactions to Pcb. |
| src/ProcSim.Core/Monitoring/MonitoringService.cs | Improves dispose pattern and updates Cpu/Pcb type usage. |
| src/ProcSim.Core/Kernel.cs | Updates Cpu/Pcb types and improves dispose pattern. |
| src/ProcSim.Core/Interruptions/InterruptService.cs | Updates ISR builder signature to Cpu. |
| src/ProcSim.Core/Interruptions/Handlers/TimerInterruptHandler.cs | Updates handler signature to Cpu. |
| src/ProcSim.Core/Interruptions/Handlers/IoInterruptHandler.cs | Updates handler signature to Cpu. |
| src/ProcSim.Core/Interruptions/Handlers/IInterruptHandler.cs | Updates interface signature to Cpu. |
| src/ProcSim.Core/IO/IoRequest.cs | Updates record type from PCB to Pcb. |
| src/ProcSim.Core/IO/IODevice.cs | Updates waiters/submit/pop signatures to Pcb and improves dispose pattern. |
| src/ProcSim.Core/Dispatcher.cs | Updates context switch/save/load APIs to Cpu/Pcb. |
| src/ProcSim.Core/CPU.cs | Renames class/ctor to Cpu. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@casuffitsharp I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@casuffitsharp I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@casuffitsharp I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * refactor: rename CPU.cs to Cpu.cs to match the Cpu type name Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com>
…dProcessChanged (#23) * Initial plan * Use pattern matching instead of unsafe cast in OnSelectedProcessChanged Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com>
* Initial plan * Rename PCB.cs to Pcb.cs to match the Pcb class name Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/ProcSim/Views/ProcessesConfigControl.xaml.cs:1
- The warning suppression is applied at file scope and is not restored, which can mask unrelated
S2325findings in this file. Prefer scoping the suppression to the specific member(s) that trigger it (disable immediately before the method and restore immediately after), or remove it if it no longer applies.
src/ProcSim/Views/ProcessesConfigControl.xaml.cs:1 - This method returns
nullbut the return type is declared as non-nullableT. If nullable reference types are enabled, this will produce warnings and makes the contract unclear. Consider changing the signature toprivate static T? FindAncestor<T>(DependencyObject? element) where T : DependencyObject(and update call sites accordingly).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | ||
| { | ||
| bool back = value is Visibility visibility && visibility == Visibility.Visible; | ||
| if (parameter != null) | ||
| { | ||
| if ((bool)parameter) | ||
| back = !back; | ||
| } | ||
| if (parameter != null && (bool)parameter) | ||
| back = !back; | ||
|
|
||
| return back; | ||
| } |
There was a problem hiding this comment.
In WPF/XAML, ConverterParameter is typically provided as a string. Casting parameter to bool will throw InvalidCastException when ConvertBack is invoked. Make ConvertBack handle the parameter the same way Convert does (e.g., parse string), or ensure both directions use a consistent parameter type.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@casuffitsharp I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Fix ConvertBack to parse parameter as string instead of casting to bool Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #pragma warning disable S2325 // Methods and properties that don't access instance data should be static | ||
| public partial class ProcessesConfigControl : UserControl | ||
| { |
There was a problem hiding this comment.
#pragma warning disable S2325 is applied at the file/class level without a corresponding restore, which suppresses the rule for the entire file and can hide future findings. Prefer scoping the suppression to the specific method(s) that need it (or using a targeted suppression attribute) to keep analyzer suppressions minimal.
|
@casuffitsharp I've opened a new pull request, #26, to work on those changes. Once the pull request is ready, I'll request review from you. |
…tring and bool types (#26) * Initial plan * Fix BooleanToVisibilityConverter to handle parameter as both string and bool Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: casuffitsharp <78829483+casuffitsharp@users.noreply.github.com>
5d8c05e to
8cb7df1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaced direct assignment with null-conditional operator for prev.State to prevent possible NullReferenceException if prev is null.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.



No description provided.