[dotnet] Allow setting of PageDimensions and PageMargins in PrintOptions directly

[shbenzer]

User description

PrintOptions.cs was missing setters for PageMargins and PageDimensions.

Description

added setters in PrintOptions.cs for PageMargins and PageDimensions.

Motivation and Context

fixes issue

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

Bug fix

---

Description

• Added missing setters for PageDimensions and PageMargins in the PrintOptions class to allow modification of these properties.
• This change addresses an issue where these properties were previously read-only, limiting their usability.

---

Changes walkthrough 📝

	Relevant files
Bug fix	PrintOptions.cs Add missing setters for PageDimensions and PageMargins      dotnet/src/webdriver/PrintOptions.cs Added a setter for PageDimensions property. Added a setter for PageMargins property. +2/-0

---

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

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Typo There is a typo in the comment for the PageMargins property. "doucment" should be "document".

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ Add null checks to property setters to prevent null reference exceptions Suggestion Impact: Null checks were added to the property setters for PageDimensions and PageMargins to prevent null reference exceptions, aligning with the suggestion. code diff: + set + { + if (value == null) + { + throw new ArgumentNullException("PageDimensions cannot be set to null"); + } + + pageSize = value; + } + } + + /// <summary> + /// Gets or sets the margins for each page in the doucment. /// </summary> public Margins PageMargins { get { return margins; } - set { margins = value; } + set + { + if (value == null) + { + throw new ArgumentNullException("PageMargins cannot be set to null"); + } + + margins = value; + } Consider adding null checks in the setters to prevent potential null reference exceptions. dotnet/src/webdriver/PrintOptions.cs [106-119] public PageSize PageDimensions { get { return pageSize; } - set { pageSize = value; } + set { pageSize = value ?? throw new ArgumentNullException(nameof(value)); } } ... public Margins PageMargins { get { return margins; } - set { margins = value; } + set { margins = value ?? throw new ArgumentNullException(nameof(value)); } } Suggestion importance[1-10]: 8 Why: Adding null checks in the setters is a valuable enhancement that prevents potential null reference exceptions, increasing the robustness and reliability of the code.	8
Enhancement	Simplify property implementation by using auto-implemented properties Consider making the PageDimensions and PageMargins properties auto-implemented to reduce boilerplate code and improve maintainability. dotnet/src/webdriver/PrintOptions.cs [106-119] -public PageSize PageDimensions -{ - get { return pageSize; } - set { pageSize = value; } -} +public PageSize PageDimensions { get; set; } ... -public Margins PageMargins -{ - get { return margins; } - set { margins = value; } -} +public Margins PageMargins { get; set; } Suggestion importance[1-10]: 7 Why: Using auto-implemented properties reduces boilerplate code and improves maintainability, making the code cleaner and easier to read without changing functionality.	7
	Make properties virtual to allow for overriding in derived classes Consider making the properties virtual to allow for potential overriding in derived classes, enhancing extensibility. dotnet/src/webdriver/PrintOptions.cs [106-119] -public PageSize PageDimensions +public virtual PageSize PageDimensions { get { return pageSize; } set { pageSize = value; } } ... -public Margins PageMargins +public virtual Margins PageMargins { get { return margins; } set { margins = value; } } Suggestion importance[1-10]: 6 Why: Making properties virtual enhances extensibility by allowing derived classes to override them, which can be beneficial in scenarios requiring customization. However, it may not be necessary if no current use case requires it.	6

💡 Need additional feedback ? start a PR chat

[nvborisenko]

Allow properties to be null or not. Nullable properties is very good idea (see inline comment).

[nvborisenko]

Good progress here, I am going to checksum (using CI). If pass, the I vote for removing DefaultPageHeight value on client side, because I hope it should be handled oon server side.

[nvborisenko]

We don't have test for Print command. @shbenzer do you want to add new tests?

[shbenzer]

@nvborisenko sure, I wrote the documentation examples - they should easily port over.

[shbenzer]

Done

[nvborisenko]

I looked at java implementation, it stores default values on client side. So let's be in sync.

• Add check for null when setting value (do not allow private value to be null)
• Add tests that value cannot be null
• Be in sync with java implementation when serializing object to json

[shbenzer]

• Add check for null when setting value (do not allow private value to be null)

Do we want to throw an error when setting as null, or do we want to just set the default in that case and let it continue?

[nvborisenko]

• Add check for null when setting value (do not allow private value to be null)

Do we want to throw an error when setting as null, or do we want to just set the default in that case and let it continue?

Definitely throw argument null exception.

[nvborisenko]

Looks good, thank you

[shbenzer]

@nvborisenko Could you explain the code changes you made?

Particularly PageSizeDictionary()s changes, I can see the logic in the changes to the main sets, as well as refining the test example to just test construction instead of construction inside of the print command.

[nvborisenko]

Only for you..

The same and simpler.

In this case value is double (I don't remember), which is never can be null.

Assert is correct, not assert which even could not be compiled. And the goal of this test is that we cannot set property to null, we don't test Print command.

[shbenzer]

Lol thank you

Those all make sense to me