You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This handles the scenario where you request names clash between different types of devices.
Motivation and Context
Today, this throws an unhelpful InvalidCastException, with no indication that the user did anything wrong. Now, it throws an InvalidOperationException (an exception whose meaning is "user cannot do this") with a better message.
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Exception Handling Verify that the new exception handling for clashing device names is comprehensive and provides clear error messages to users.
Code Duplication Check if the similar code structure in SetActivePointer, SetActiveKeyboard, and SetActiveWheel methods can be refactored to reduce duplication.
Performance Consideration Evaluate if the FindDeviceById method's linear search through sequences can be optimized for better performance, especially with a large number of sequences.
Refactor device activation methods to reduce code duplication
Consider implementing a generic method for setting active devices to reduce code duplication across SetActivePointer, SetActiveKeyboard, and SetActiveWheel methods.
-public Actions SetActivePointer(PointerKind kind, string name)+private T SetActiveDevice<T>(string name, Func<string, T> createDevice) where T : InputDevice
{
InputDevice device = FindDeviceById(name);
if (device == null)
{
- this.activePointer = new PointerInputDevice(kind, name);- }- else- {- this.activePointer = device as PointerInputDevice- ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a pointer. Actual input type: {device.DeviceKind}");+ return createDevice(name);
}
+ return device as T ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a {typeof(T).Name}. Actual input type: {device.DeviceKind}");+}++public Actions SetActivePointer(PointerKind kind, string name)+{+ this.activePointer = SetActiveDevice(name, n => new PointerInputDevice(kind, n));
return this;
}
public Actions SetActiveKeyboard(string name)
{
- InputDevice device = FindDeviceById(name);-- if (device == null)- {- this.activeKeyboard = new KeyInputDevice(name);- }- else- {- this.activeKeyboard = device as KeyInputDevice- ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a keyboard. Actual input type: {device.DeviceKind}");-- }-+ this.activeKeyboard = SetActiveDevice(name, n => new KeyInputDevice(n));
return this;
}
public Actions SetActiveWheel(string name)
{
- InputDevice device = FindDeviceById(name);-- if (device == null)- {- this.activeWheel = new WheelInputDevice(name);- }- else- {- this.activeWheel = device as WheelInputDevice- ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a wheel. Actual input type: {device.DeviceKind}");- }-+ this.activeWheel = SetActiveDevice(name, n => new WheelInputDevice(n));
return this;
}
Apply this suggestion
Suggestion importance[1-10]: 9
Why: This suggestion significantly improves code maintainability by reducing duplication across multiple methods. It introduces a generic method for setting active devices, which makes the code more concise and easier to maintain, especially if new device types are added in the future.
9
Performance
Implement caching for input devices to improve lookup performance
Consider using a dictionary to store and retrieve input devices by their names, instead of searching through the action sequences each time. This would improve performance, especially for repeated operations on the same devices.
+private Dictionary<string, InputDevice> deviceCache = new Dictionary<string, InputDevice>();+
private InputDevice FindDeviceById(string name)
{
- foreach (var sequence in this.actionBuilder.ToActionSequenceList())+ if (deviceCache.TryGetValue(name, out var device))
{
- Dictionary<string, object> actions = sequence.ToDictionary();-- string id = (string)actions["id"];-- if (id == name)- {- return sequence.inputDevice;- }+ return device;
}
- return null;+ device = this.actionBuilder.ToActionSequenceList()+ .FirstOrDefault(sequence => (string)sequence.ToDictionary()["id"] == name)?.inputDevice;++ if (device != null)+ {+ deviceCache[name] = device;+ }++ return device;
}
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion significantly improves performance by implementing a caching mechanism for input devices. It reduces repeated searches through action sequences, which is especially beneficial for operations involving the same devices multiple times.
8
Enhancement
✅ Use pattern matching to simplify device type checking and assignmentSuggestion Impact:The suggestion to use pattern matching instead of if-else statements for setting active devices was implemented in the commit. The code now uses a switch expression for device assignment.
code diff:
@@ -77,15 +77,12 @@
{
InputDevice device = FindDeviceById(name);
- if (device == null)- {- this.activePointer = new PointerInputDevice(kind, name);- }- else- {- this.activePointer = device as PointerInputDevice- ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a pointer. Actual input type: {device.DeviceKind}");- }+ this.activePointer = device switch+ {+ null => new PointerInputDevice(kind, name),+ PointerInputDevice pointerDevice => pointerDevice,+ _ => throw new InvalidOperationException($"Device under the name \"{name}\" is not a pointer. Actual input type: {device.DeviceKind}"),+ };
Consider using a switch statement or pattern matching instead of multiple if-else statements when setting active devices. This would make the code more readable and easier to maintain, especially if more device types are added in the future.
public Actions SetActivePointer(PointerKind kind, string name)
{
InputDevice device = FindDeviceById(name);
- if (device == null)+ this.activePointer = device switch
{
- this.activePointer = new PointerInputDevice(kind, name);- }- else- {- this.activePointer = device as PointerInputDevice- ?? throw new InvalidOperationException($"Device under the name \"{name}\" is not a pointer. Actual input type: {device.DeviceKind}");- }+ null => new PointerInputDevice(kind, name),+ PointerInputDevice pointer => pointer,+ _ => throw new InvalidOperationException($"Device under the name \"{name}\" is not a pointer. Actual input type: {device.DeviceKind}")+ };
return this;
}
Apply this suggestion
Suggestion importance[1-10]: 6
Why: The suggestion enhances code readability and maintainability by using pattern matching instead of if-else statements. While it's a good improvement, it's not as critical as the performance enhancement in the first suggestion.
The pattern matching is actually a good suggestion! The maintainability one seems like overkill, especially with the lambda expression. Let me implement the pattern matching
Tests coverage is another topic, we even don't know what is covered. But please at least for your "simple" changes, given that you already added a test, let's keep it workable in this PR.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This handles the scenario where you request names clash between different types of devices.
Motivation and Context
Today, this throws an unhelpful
InvalidCastException, with no indication that the user did anything wrong. Now, it throws anInvalidOperationException(an exception whose meaning is "user cannot do this") with a better message.Types of changes
Checklist
PR Type
Bug fix, Tests
Description
InvalidCastExceptionwithInvalidOperationExceptionfor better clarity.FindDeviceByIdto streamline device lookup and reduce code duplication.Changes walkthrough 📝
Actions.cs
Handle clashing device names with improved exception handlingdotnet/src/webdriver/Interactions/Actions.cs
FindDeviceByIdmethod for device lookup.CombinedInputActionsTest.cs
Add tests for clashing device name handlingdotnet/test/common/Interactions/CombinedInputActionsTest.cs