Add dropdown-based node and edge editors#4
Conversation
Move node role editing into a dedicated window with dropdown selection for nodes, traffic entries, traffic types, and roles, and constrain edge endpoints to existing nodes.
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated node editor window and a Node Roles tab to manage node properties and traffic profiles. Key changes include the addition of the NodeEditorWindow, UI updates for node selection, and view model enhancements for role-based configuration. Feedback identifies a redundant property assignment when adding nodes, potential logic issues leading to duplicate traffic profiles, code duplication in error handling methods, and opportunities to optimize property setters in the view models.
| SelectedNode = node; | ||
| SelectedNodeTrafficProfile = initialProfile; |
There was a problem hiding this comment.
The assignment to SelectedNodeTrafficProfile on line 326 is redundant. Setting SelectedNode = node on the previous line already triggers the SelectedNode property setter (see line 144), which automatically selects the first traffic profile of the node. Since initialProfile was just added as the first profile, it will be selected automatically.
SelectedNode = node;| var trafficName = TrafficDefinitions | ||
| .Select(definition => definition.Name) | ||
| .Where(name => !string.IsNullOrWhiteSpace(name)) | ||
| .FirstOrDefault(name => SelectedNode.TrafficProfiles.All(profile => !Comparer.Equals(profile.TrafficType, name))) | ||
| ?? primaryTrafficDefinition.Name; |
There was a problem hiding this comment.
This logic can lead to duplicate traffic profiles for the same traffic type on a single node. If all defined traffic types are already present in the node's profiles, FirstOrDefault returns null and trafficName defaults to the primary traffic definition's name. If that primary definition is also already present, a duplicate profile will be added. Consider preventing the addition or generating a new unique traffic type if no unused types are available.
| private void ExecuteWithErrorHandling(Action action) | ||
| { | ||
| try | ||
| { | ||
| action(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| MessageBox.Show( | ||
| this, | ||
| ex.Message, | ||
| "MedW Network Simulator", | ||
| MessageBoxButton.OK, | ||
| MessageBoxImage.Error); | ||
| } | ||
| } |
| set | ||
| { | ||
| ArgumentNullException.ThrowIfNull(value); | ||
|
|
||
| switch (value) | ||
| { | ||
| case ProducerRole: | ||
| ApplyRoleSelection(isProducer: true, isConsumer: false, canTransship: false); | ||
| break; | ||
| case ConsumerRole: | ||
| ApplyRoleSelection(isProducer: false, isConsumer: true, canTransship: false); | ||
| break; | ||
| case TransshipRole: | ||
| ApplyRoleSelection(isProducer: false, isConsumer: false, canTransship: true); | ||
| break; | ||
| case ProducerConsumerRole: | ||
| ApplyRoleSelection(isProducer: true, isConsumer: true, canTransship: false); | ||
| break; | ||
| case ProducerTransshipRole: | ||
| ApplyRoleSelection(isProducer: true, isConsumer: false, canTransship: true); | ||
| break; | ||
| case ConsumerTransshipRole: | ||
| ApplyRoleSelection(isProducer: false, isConsumer: true, canTransship: true); | ||
| break; | ||
| case ProducerConsumerTransshipRole: | ||
| ApplyRoleSelection(isProducer: true, isConsumer: true, canTransship: true); | ||
| break; | ||
| default: | ||
| ApplyRoleSelection(isProducer: false, isConsumer: false, canTransship: false); | ||
| break; | ||
| } | ||
|
|
||
| OnPropertyChanged(nameof(SelectedRoleName)); | ||
| } |
There was a problem hiding this comment.
The SelectedRoleName setter should check if the value has actually changed before proceeding to avoid unnecessary processing. Additionally, the explicit call to OnPropertyChanged(nameof(SelectedRoleName)) at the end is redundant because ApplyRoleSelection modifies properties whose setters already trigger a notification for SelectedRoleName.
set
{
ArgumentNullException.ThrowIfNull(value);
if (string.Equals(value, SelectedRoleName, StringComparison.Ordinal))
{
return;
}
switch (value)
{
case ProducerRole:
ApplyRoleSelection(isProducer: true, isConsumer: false, canTransship: false);
break;
case ConsumerRole:
ApplyRoleSelection(isProducer: false, isConsumer: true, canTransship: false);
break;
case TransshipRole:
ApplyRoleSelection(isProducer: false, isConsumer: false, canTransship: true);
break;
case ProducerConsumerRole:
ApplyRoleSelection(isProducer: true, isConsumer: true, canTransship: false);
break;
case ProducerTransshipRole:
ApplyRoleSelection(isProducer: true, isConsumer: false, canTransship: true);
break;
case ConsumerTransshipRole:
ApplyRoleSelection(isProducer: false, isConsumer: true, canTransship: true);
break;
case ProducerConsumerTransshipRole:
ApplyRoleSelection(isProducer: true, isConsumer: true, canTransship: true);
break;
default:
ApplyRoleSelection(isProducer: false, isConsumer: false, canTransship: false);
break;
}
}
Move node role editing into a dedicated window with dropdown selection for nodes, traffic entries, traffic types, and roles, and constrain edge endpoints to existing nodes.