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

Fix PrintOptions size example to replace setScale with setPageSize for A4 dimensions #2118

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

yvsvarma
Copy link
Contributor

@yvsvarma yvsvarma commented Jan 1, 2025

User description

Description

This pull request updates the PrintOptions size example in the Selenium documentation to use the correct API method. The previous example incorrectly demonstrated setScale, which is unrelated to setting the page size. The updated example now uses the setPageSize method with the PageSize class, leveraging its constructor to set dimensions for A4 size (27.94 x 21.59 cm).

This example also demonstrates how to retrieve the page height using getPageSize().getHeight(), and a comment has been added to guide users on retrieving the width with getWidth() if needed.

Motivation and Context

This change addresses the issue raised in #2095, where the documentation incorrectly demonstrated the setScale method instead of showcasing the correct setPageSize functionality for the size example.

The updated example:

Fixes the incorrect API usage (setScale) in the documentation.
Aligns with the current Selenium 4.27.0 API, providing users with an accurate and functional reference.
Demonstrates the ability to set and retrieve dimensions for A4 size using the existing PageSize class.

Note:

I have tested the example code locally, and it runs successfully. This commit addresses the fix for the Java example only. I will update the examples for other languages in the same PR once this review is complete.


PR Type

Bug fix, Documentation


Description

  • Replace incorrect setScale with proper setPageSize method

  • Implement A4 page dimensions using PageSize class

  • Add documentation for width/height retrieval methods


Changes walkthrough 📝

Relevant files
Bug fix
PrintOptionsTest.java
Fix PrintOptions size example with correct PageSize implementation

examples/java/src/test/java/dev/selenium/interactions/PrintOptionsTest.java

  • Replaced incorrect setScale method with setPageSize for A4 dimensions
  • Added PageSize constructor with A4 dimensions (27.94 x 21.59 cm)
  • Updated variable to get page height instead of scale
  • Added comment about retrieving width using getWidth()
  • +2/-2     

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

    Copy link

    netlify bot commented Jan 1, 2025

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit df49414

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 1, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2095 - Fully compliant

    Compliant requirements:

    • Fix incorrect documentation example for setting page size in PrintOptions
    • Show proper usage of page size setting instead of scale setting
    • Demonstrate how to set paper size in PrintOptions
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Unused Variable

    The currentHeight variable is assigned but never used in the test method. Consider either using this value in an assertion or removing it if not needed.

    double currentHeight = printOptions.getPageSize().getHeight(); // use getWidth() to retrieve width

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Correct the dimensions for A4 paper size to ensure accurate printing output

    The A4 dimensions are incorrect. Standard A4 dimensions are 21.0 x 29.7 cm. Update
    the values to ensure correct page sizing.

    examples/java/src/test/java/dev/selenium/interactions/PrintOptionsTest.java [34]

    -printOptions.setPageSize(new PageSize(27.94, 21.59)); // A4 size in cm
    +printOptions.setPageSize(new PageSize(21.0, 29.7)); // A4 size in cm
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies and fixes incorrect A4 paper dimensions (27.94 x 21.59 cm) to the standard A4 dimensions (21.0 x 29.7 cm), which is crucial for proper printing functionality.

    9
    General
    Add test assertions to validate the expected dimensions are set correctly

    Add assertions to verify the page size is set correctly, as this is a test method.

    examples/java/src/test/java/dev/selenium/interactions/PrintOptionsTest.java [35]

    -double currentHeight = printOptions.getPageSize().getHeight(); // use getWidth() to retrieve width
    +double currentHeight = printOptions.getPageSize().getHeight();
    +assertEquals(29.7, currentHeight, 0.1); // Verify A4 height in cm
    +assertEquals(21.0, printOptions.getPageSize().getWidth(), 0.1); // Verify A4 width in cm
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves the test method by adding essential assertions to verify both height and width dimensions, which is crucial for a proper test case implementation.

    8

    Copy link
    Member

    @harsha509 harsha509 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 @yvsvarma !

    @yvsvarma
    Copy link
    Contributor Author

    yvsvarma commented Jan 2, 2025

    Thank you very much @harsha509 for your time and effort in reviewing 😊

    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    HI @yvsvarma ,

    looks like tests are failing due to missing import import org.openqa.selenium.print.PageSize;

    Could you please check and fix the test

    @yvsvarma
    Copy link
    Contributor Author

    yvsvarma commented Jan 2, 2025

    HI @yvsvarma ,

    looks like tests are failing due to missing import import org.openqa.selenium.print.PageSize;

    Could you please check and fix the test

    Yes @harsha509, I too just noticed that. Will check and fix that Sri.

    @yvsvarma yvsvarma force-pushed the fix-pagesize-example-doc branch from 0da6868 to 34da705 Compare January 3, 2025 04:03
    @yvsvarma
    Copy link
    Contributor Author

    yvsvarma commented Jan 3, 2025

    HI @yvsvarma ,

    looks like tests are failing due to missing import import org.openqa.selenium.print.PageSize;

    Could you please check and fix the test

    Hi @harsha509 , I have added the missing import and ran all the test cases and attached the logs here. Please review.
    TestResults_PrintOptionsTest.txt

    Thanks,
    Vinay

    @harsha509 harsha509 merged commit 22f0293 into SeleniumHQ:trunk Jan 3, 2025
    7 of 9 checks passed
    @yvsvarma yvsvarma deleted the fix-pagesize-example-doc branch January 3, 2025 05:23
    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