Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet] Annotate nullable reference types on input devices #14804

Merged
merged 13 commits into from
Jan 24, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 25, 2024

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

Annotates nullable reference types on InputDevice and derived types. Implement modernization, simplification, and more XML documentation along the way.

Motivation and Context

Contributes to #14640

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)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement, documentation


Description

  • Enabled nullable reference types across multiple input device classes to improve null safety.
  • Modernized code by using auto-properties and expression-bodied members for simplicity.
  • Added XML documentation for constructors to specify exceptions thrown.
  • Improved exception handling and documentation for better code clarity and maintenance.

Changes walkthrough 📝

Relevant files
Enhancement
InputDevice.cs
Annotate nullable types and modernize InputDevice class   

dotnet/src/webdriver/Interactions/InputDevice.cs

  • Enabled nullable reference types.
  • Replaced field with auto-property for DeviceName.
  • Simplified method implementations using expression-bodied members.
  • Added XML documentation for exceptions.
  • +8/-16   
    KeyInputDevice.cs
    Annotate nullable types and simplify KeyInputDevice           

    dotnet/src/webdriver/Interactions/KeyInputDevice.cs

  • Enabled nullable reference types.
  • Simplified property and method implementations.
  • Added XML documentation for exceptions.
  • +10/-14 
    PointerInputDevice.cs
    Enhance PointerInputDevice with nullable annotations         

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs

  • Enabled nullable reference types.
  • Converted fields to readonly where applicable.
  • Simplified property and method implementations.
  • Improved exception handling and documentation.
  • +60/-114
    WheelInputDevice.cs
    Enhance WheelInputDevice with nullable annotations             

    dotnet/src/webdriver/Interactions/WheelInputDevice.cs

  • Enabled nullable reference types.
  • Converted fields to readonly where applicable.
  • Simplified property and method implementations.
  • Improved exception handling and documentation.
  • +23/-50 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Reference
    The ToString() method assumes target is not null when origin is CoordinateOrigin.Element. This could cause NullReferenceException if target is null.

    Code Duplication
    The ConvertElement() method is duplicated between PointerInputDevice and WheelInputDevice classes. Consider extracting to a shared helper class.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 25, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add parameter validation to prevent null reference exceptions

    Add null check for properties parameter in the constructor to avoid potential
    NullReferenceException when accessing eventProperties later.

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs [427-432]

     public PointerDownInteraction(InputDevice sourceDevice, MouseButton button, PointerEventProperties properties)
         : base(sourceDevice)
     {
         this.button = button;
    -    this.eventProperties = properties;
    +    this.eventProperties = properties ?? throw new ArgumentNullException(nameof(properties));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding null validation for the properties parameter is crucial as it prevents potential NullReferenceException in the ToDictionary method where eventProperties is used.

    8
    ✅ Add null check for required parameter to prevent runtime errors
    Suggestion Impact:The commit added checks to handle cases where the target is null or cannot be converted to IWebDriverObjectReference, aligning with the suggestion to prevent runtime errors.

    code diff:

    +                IWebDriverObjectReference? elementReference = this.target as IWebDriverObjectReference;
    +                if (elementReference == null)
    +                {
    +                    IWrapsElement? elementWrapper = this.target as IWrapsElement;
    +                    if (elementWrapper != null)
    +                    {
    +                        elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference;
    +                    }
    +                }
    +
    +                if (elementReference == null)
    +                {
    +                    throw new ArgumentException("Target element cannot be converted to IWebElementReference");
    +                }
    +
    +                Dictionary<string, object> elementDictionary = elementReference.ToDictionary();
    +                return elementDictionary;

    Add null check for target parameter in ConvertElement() since it's marked as
    nullable but the method assumes it's non-null.

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs [584-588]

     private Dictionary<string, object> ConvertElement()
     {
    +    if (this.target == null)
    +    {
    +        throw new ArgumentNullException(nameof(target));
    +    }
         if (this.target is IWebDriverObjectReference element)
         {
             return element.ToDictionary();
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The ConvertElement method assumes target is non-null but it's marked as nullable. Adding an explicit null check improves error handling and prevents runtime exceptions.

    7

    💡 Need additional feedback ? start a PR chat

    @RenderMichael RenderMichael merged commit 78e1029 into SeleniumHQ:trunk Jan 24, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the input-device branch January 24, 2025 18:58
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants