Skip to content

[java] increasing of properties scope for better appium compatibility #14183

Merged
pujagani merged 9 commits intoSeleniumHQ:trunkfrom
iampopovich:13949-appium-compatibility
Oct 1, 2024
Merged

[java] increasing of properties scope for better appium compatibility #14183
pujagani merged 9 commits intoSeleniumHQ:trunkfrom
iampopovich:13949-appium-compatibility

Conversation

@iampopovich
Copy link
Copy Markdown
Contributor

@iampopovich iampopovich commented Jun 24, 2024 •
edited by qodo-code-review Bot
Loading

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

according to request in #13949 scopes of properties were increased
here's a part of request

Increase the scope of private properties of the below classes to 'protected':
RemoteWebDriver -> capabilities

HttpCommandExecutor -> commandCodec

HttpCommandExecutor -> responseCodec

FluentWait -> clock
FluentWait -> timeout
FluentWait -> interval
FluentWait -> sleeper
FluentWait -> ignoredExceptions
FluentWait -> messageSupplier
FluentWait -> input

Make public accessor for the property:
HttpCommandExecutor -> client

DriverService.Builder -> exe

Motivation and Context

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

Description

  • Changed the access level of several fields in HttpCommandExecutor to improve compatibility with Appium:
    • client field changed to public.
    • commandCodec and responseCodec fields changed to protected.
  • Changed the access level of the capabilities field in RemoteWebDriver to protected.
  • Changed the access level of the exe field in DriverService.Builder to public.
  • Changed the access level of multiple fields in FluentWait to protected:
    • input, clock, sleeper, timeout, interval, messageSupplier, ignoredExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
HttpCommandExecutor.java
Modify access levels for HttpCommandExecutor fields           

java/src/org/openqa/selenium/remote/HttpCommandExecutor.java

  • Changed client field to public.
  • Changed commandCodec and responseCodec fields to protected.
    		•  +3/-3     
    RemoteWebDriver.java
    Modify access level for RemoteWebDriver capabilities field

    java/src/org/openqa/selenium/remote/RemoteWebDriver.java

    • Changed capabilities field to protected.
    		 +1/-1     
    DriverService.java
    Modify access level for DriverService.Builder exe field   

    java/src/org/openqa/selenium/remote/service/DriverService.java

    • Changed exe field to public.
    		 +1/-1     
    FluentWait.java
    Modify access levels for FluentWait fields                             

    java/src/org/openqa/selenium/support/ui/FluentWait.java

  • Changed multiple fields to protected: input, clock, sleeper, timeout,
    interval, messageSupplier, ignoredExceptions.
    		•  +7/-7     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-code-review qodo-code-review Bot added the P-enhancement PR with a new feature label Jun 24, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Jun 24, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Access Level Changes:
    The PR changes the access levels of several fields in multiple classes. This could potentially expose internal implementation details and should be carefully reviewed to ensure that it does not violate encapsulation principles or introduce unintended side effects.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Jun 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Change the access modifier of the client field from public to protected to improve encapsulation

    Consider making the client field protected instead of public to restrict access to
    subclasses only, maintaining encapsulation and reducing the risk of unintended
    modifications.

    java/src/org/openqa/selenium/remote/HttpCommandExecutor.java [47]

    -public final HttpClient client;
+protected final HttpClient client;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Changing the access modifier from public to protected for the client field enhances encapsulation and limits access to subclasses, which is a significant improvement in design.

    		8
    Make the capabilities field final to prevent it from being reassigned after initialization

    Consider making the capabilities field final to indicate that it should not be reassigned
    after being initialized, which can help prevent bugs.

    java/src/org/openqa/selenium/remote/RemoteWebDriver.java [117]

    -protected Capabilities capabilities;
+protected final Capabilities capabilities;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Making the capabilities field final is a good practice to ensure it is not modified after initialization, which can help in maintaining immutability and preventing bugs.

    		7
    Enhancement
    Initialize the messageSupplier field with a more meaningful default message

    Consider initializing the messageSupplier field with a more meaningful default message to
    improve debugging and error reporting.

    java/src/org/openqa/selenium/support/ui/FluentWait.java [75]

    -protected Supplier<String> messageSupplier = () -> null;
+protected Supplier<String> messageSupplier = () -> "Condition not met within timeout";
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Providing a more meaningful default message for messageSupplier can enhance debugging and error reporting, which improves the usability of the class.

    		6

    @iampopovich iampopovich changed the title 13949 better appium compatibility [java] increasing of properties scope for better appium compatibility Jun 24, 2024
    pujagani
    pujagani approved these changes Oct 1, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you @iampopovich!

    @pujagani pujagani merged commit e455840 into SeleniumHQ:trunk Oct 1, 2024
    @iampopovich
    Copy link
    Copy Markdown
    Contributor Author

    iampopovich commented Oct 1, 2024 •
    edited
    Loading

    thanks god . it took a bit more time than usual 🙂
    guess you have a lot of work with bidi

    @iampopovich iampopovich deleted the 13949-appium-compatibility branch October 1, 2024 11:54
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @iampopovich @pujagani @M1troll
    [java] increasing of properties scope for better appium compatibility (…
    6b9f07f 
    …SeleniumHQ#14183)

Co-authored-by: Puja Jagani <[email protected]>
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @pujagani pujagani pujagani approved these changes

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @iampopovich @pujagani