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] [bidi] Make input Actions as not nested #15437

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Mar 16, 2025

User description

Motivation and Context

Contributes to #15407

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, Tests


Description

  • Replaced nested DTO types with standalone types for better extensibility.

    • Updated Pointer and Key nested types to standalone types like DownPointer, UpPointer, etc.
    • Adjusted JSON serialization attributes to reflect new type structure.
  • Updated test cases to use new standalone types.

    • Modified CombinedInputActionsTest to replace nested type references.

Changes walkthrough 📝

Relevant files
Enhancement
BiDiJsonSerializerContext.cs
Removed JSON serialization for nested types                           

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs

  • Removed JSON serialization attributes for nested Pointer and Key
    types.
  • Retained serialization for other input-related commands and
    interfaces.
  • +0/-5     
    SourceActions.cs
    Refactored nested types to standalone types                           

    dotnet/src/webdriver/BiDi/Modules/Input/SourceActions.cs

  • Replaced nested Pointer and Key types with standalone types like
    DownPointer, UpPointer, etc.
  • Updated JSON polymorphic attributes to use new standalone types.
  • Adjusted properties and methods to align with new type structure.
  • +40/-43 
    Tests
    CombinedInputActionsTest.cs
    Updated tests for standalone input types                                 

    dotnet/test/common/BiDi/Input/CombinedInputActionsTest.cs

  • Updated test cases to use new standalone types instead of nested
    types.
  • Adjusted input actions to reference DownPointer, UpPointer, DownKey,
    and UpKey.
  • +12/-12 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15407 - Fully compliant

    Compliant requirements:

    • Replace nested DTO types with standalone types for better extensibility
    • Apply this pattern to all similar nested types

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

    Naming Convention

    The new standalone types like DownKey and UpKey might benefit from more descriptive names like KeyDownAction and KeyUpAction to better indicate their purpose as actions.

    public record DownKey(char Value) : Key;
    
    public record UpKey(char Value) : Key;

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix interface type mismatch

    The NoneActions class should inherit from SourceActions to match the pattern
    used by other action types and to properly implement the interface. Currently,
    it's using None which doesn't match the interface type.

    dotnet/src/webdriver/BiDi/Modules/Input/SourceActions.cs [88]

    -public record NoneActions : SourceActions<None>;
    +public record NoneActions : SourceActions<INoneSourceAction>;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies an inconsistency in the type hierarchy. The NoneActions class should use INoneSourceAction instead of None to match the pattern used by other action types (KeyActions, PointerActions, WheelActions). This would ensure type consistency and proper interface implementation.

    Medium
    • More

    @nvborisenko nvborisenko merged commit 68bfaf9 into SeleniumHQ:trunk Mar 17, 2025
    8 of 10 checks passed
    @nvborisenko nvborisenko deleted the bidi-actions branch March 17, 2025 16:07
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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