Mobile enhancements and changes#4005
Merged
Merged
Conversation
Contributor
WalkthroughThe pull request includes modifications to several XAML and C# files related to mobile device configuration and UI elements. Key changes involve updating the swipe scale range in Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml (1)
76-78: Document rationale for increased swipe scale rangeThe expansion of the swipe scale range to 2 represents a significant change in gesture control capabilities. Consider:
- Adding code comments explaining the rationale for this change
- Documenting any device-specific considerations
- Including test scenarios that validate behavior across the new range, especially near the boundaries (0.1, 1.0, and 2.0)
Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml (2)
58-60: Consider standardizing input field widthsThe width value of 122px seems arbitrary. Consider standardizing input field widths across the application, preferably using round numbers (e.g., 120px or 125px) for consistency.
-<Actions:UCValueExpression x:Name="xLoadTimeoutTxtbox" Width="122" Margin="17,0,0,0"/> +<Actions:UCValueExpression x:Name="xLoadTimeoutTxtbox" Width="120" Margin="17,0,0,0"/>
70-77: LGTM: Well-structured screenshot dimension configurationThe new screenshot height and width configuration fields are well-organized with consistent styling and spacing. The implementation aligns with the backend changes in GenericAppiumDriver.cs.
Consider adding input validation tooltips to indicate acceptable value ranges for screenshot dimensions.
-<Actions:UCValueExpression x:Name="xScreenshotHeightTextBox" Width="120" Margin="5,0,0,0"/> +<Actions:UCValueExpression x:Name="xScreenshotHeightTextBox" Width="120" Margin="5,0,0,0" + ToolTip="Enter a value between 1 and 9999 pixels"/> -<Actions:UCValueExpression x:Name="xScreenshotWidthTextBox" Width="120" Margin="7,0,0,0"/> +<Actions:UCValueExpression x:Name="xScreenshotWidthTextBox" Width="120" Margin="7,0,0,0" + ToolTip="Enter a value between 1 and 9999 pixels"/>Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml.cs (3)
108-116: Extract validation rules to constants.Consider extracting validation messages to a constants class for better maintainability and reusability.
Create a constants class:
public static class ValidationMessages { public const string URL_EMPTY = "URL cannot be empty"; public const string LOAD_TIMEOUT_EMPTY = "Load Timeout cannot be empty"; public const string HEIGHT_EMPTY = "Height cannot be empty"; public const string WIDTH_EMPTY = "Width cannot be empty"; }Then update the validation rules:
-xServerURLTextBox.ValueTextBox.AddValidationRule(new ValidateEmptyValue("URL cannot be empty")); +xServerURLTextBox.ValueTextBox.AddValidationRule(new ValidateEmptyValue(ValidationMessages.URL_EMPTY));
118-125: Improve method naming and documentation.The method name and comment could be more descriptive about the purpose of triggering property changes.
Consider this improvement:
-// need in order to trigger the validation's rules -private void CallConfigPropertyChange() +/// <summary> +/// Triggers property change notifications to validate configuration fields. +/// This ensures that validation rules are re-evaluated after initial binding. +/// </summary> +private void TriggerConfigurationValidation()
Line range hint
479-479: Investigate binding issue.There's a TODO comment indicating that binding is lost for an unknown reason. This could lead to unexpected behavior and should be investigated.
Would you like me to help investigate the binding issue or create a GitHub issue to track this?
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
1679-1690: Minor improvements for validation messages and comments.
- Fix typo in comment: "valide" should be "validate"
- Make validation messages more consistent by using similar format
- //valide swipeScale is between 0.1 and 2 and if not set it to 1 and write warn messaage + //validate swipeScale is between 0.1 and 2 and if not set it to 1 and write warn message if (swipeScale < 0.1 || swipeScale > 2) { - Reporter.ToLog(eLogLevel.WARN, "Mobile action swipeScreen(): swipeScale: '" + swipeScale + "' is out of range (0.1-2), setting it to 1"); + Reporter.ToLog(eLogLevel.WARN, $"Mobile action swipeScreen(): swipeScale '{swipeScale}' is out of range (0.1-2), setting it to 1"); } - //validate swipeDuration is bigger than 0 and if not set it to 200 and write warn messaage + //validate swipeDuration is greater than 0 and if not set it to 200 and write warn message if (swipeDuration.TotalMilliseconds < 0) { - Reporter.ToLog(eLogLevel.WARN, "Mobile action swipeScreen(): swipeDuration: '" + swipeDuration + "' is out of range, setting it to 200"); + Reporter.ToLog(eLogLevel.WARN, $"Mobile action swipeScreen(): swipeDuration '{swipeDuration}' is out of range (must be positive), setting it to 200"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml(1 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml(3 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(3 hunks)
🔇 Additional comments (4)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml (1)
76-76: Verify validation logic for the expanded swipe scale range
The swipe scale range has been expanded from (0.1-1) to (0.1-2). While the UI label has been updated, we should ensure:
- The validation logic in the code-behind file has been updated to accept values up to 2
- The change is documented in the Help Library
- Existing automation scripts using swipe actions are reviewed for compatibility
Let's verify the validation logic:
Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml (2)
39-39: LGTM: Improved vertical spacing
The increased top margin enhances the visual separation between controls, improving the overall layout hierarchy.
56-56: Verify width consistency across similar input fields
The width increase to 190px for the proxy textbox should be validated against other similar input fields in the application for consistency.
✅ Verification successful
Based on the search results, I can now generate the final response:
Width change appears consistent with similar input fields in driver configurations
The width increase to 190px for the proxy textbox aligns with the width patterns found in driver configuration pages:
- Other proxy textboxes: 250px (WebServicesDriverEditPage)
- Similar configuration textboxes in AppiumDriverEditPage: 120-190px range
- Load timeout textbox in same file: 122px
The 190px width is within the reasonable range for input fields in driver configuration pages and maintains UI consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other UCValueExpression controls with explicit width settings
rg -l "UCValueExpression.*Width=\"\d+" | xargs ast-grep --pattern 'Actions:UCValueExpression Width="$_"'
Length of output: 274
Script:
#!/bin/bash
# Search for UCValueExpression controls with width settings using a more flexible pattern
rg -l "UCValueExpression" | grep "\.xaml$" | xargs rg "UCValueExpression.*Width=\"\d+"
Length of output: 29477
Script:
#!/bin/bash
# Search for any input-like controls with explicit width settings in XAML files
rg -l "TextBox|ComboBox|UCValueExpression" | grep "\.xaml$" | xargs rg "Width=\"\d+".*"(TextBox|ComboBox|UCValueExpression)"
Length of output: 54504
Script:
#!/bin/bash
# Find all XAML files in the Drivers configuration section
fd ".*EditPage\.xaml$" "Ginger/Ginger/Drivers"
Length of output: 446
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
112-121: LGTM! Well-structured property declarations.
The new screenshot dimension properties are properly documented with appropriate attributes and default values.
Comment on lines
+80
to
+86
| DriverConfigParam screenshotHeight = mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotHeight)); | ||
| xScreenshotHeightTextBox.Init(null, screenshotHeight, nameof(DriverConfigParam.Value)); | ||
| BindingHandler.ObjFieldBinding(xScreenshotHeightTextBox, TextBox.ToolTipProperty, mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotHeight)), nameof(DriverConfigParam.Description)); | ||
|
|
||
| DriverConfigParam screenshotWidth = mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotWidth)); | ||
| xScreenshotWidthTextBox.Init(null, screenshotWidth, nameof(DriverConfigParam.Value)); | ||
| BindingHandler.ObjFieldBinding(xScreenshotWidthTextBox, TextBox.ToolTipProperty, mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotWidth)), nameof(DriverConfigParam.Description)); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for screenshot dimensions.
While the implementation follows the existing pattern, consider adding validation to ensure the screenshot dimensions are positive numbers greater than zero.
Add numeric validation rules:
DriverConfigParam screenshotHeight = mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotHeight));
xScreenshotHeightTextBox.Init(null, screenshotHeight, nameof(DriverConfigParam.Value));
+xScreenshotHeightTextBox.ValueTextBox.AddValidationRule(new ValidatePositiveNumber("Height must be a positive number"));
DriverConfigParam screenshotWidth = mAgent.GetOrCreateParam(nameof(GenericAppiumDriver.ScreenshotWidth));
xScreenshotWidthTextBox.Init(null, screenshotWidth, nameof(DriverConfigParam.Value));
+xScreenshotWidthTextBox.ValueTextBox.AddValidationRule(new ValidatePositiveNumber("Width must be a positive number"));Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+3202
to
+3245
|
|
||
| //override with user configured sizes | ||
| if (ScreenshotHeight.ToLower().Trim() != "auto") | ||
| { | ||
| //convert from int | ||
| int userConfiguredHeight; | ||
| if (int.TryParse(ScreenshotHeight, out userConfiguredHeight)) | ||
| { | ||
| //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message | ||
| if (userConfiguredHeight < 300) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 300 pixels, using the calculated height '" + mWindowHeight.ToString() +"' instead."); | ||
| } | ||
| else | ||
| { | ||
| mWindowHeight = userConfiguredHeight; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height"); | ||
| } | ||
| } | ||
| if (ScreenshotWidth.ToLower().Trim() != "auto") | ||
| { | ||
| //convert from int | ||
| int userConfiguredWidth; | ||
| if (int.TryParse(ScreenshotWidth, out userConfiguredWidth)) | ||
| { | ||
| //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message | ||
| if (userConfiguredWidth < 300) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 300 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead."); | ||
| } | ||
| else | ||
| { | ||
| mWindowWidth = userConfiguredWidth; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width."); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring screenshot dimension handling for better maintainability.
The current implementation has duplicate code and magic numbers. Consider these improvements:
- Extract the minimum dimension validation to a method
- Define constants for magic numbers and strings
- Simplify the parsing logic
+ private const int MIN_SCREENSHOT_DIMENSION = 300;
+ private const string AUTO_VALUE = "auto";
+
+ private int ValidateScreenshotDimension(string configuredValue, int calculatedValue, string dimensionName)
+ {
+ if (configuredValue.ToLower().Trim() == AUTO_VALUE)
+ {
+ return calculatedValue;
+ }
+
+ if (!int.TryParse(configuredValue, out int userConfigured))
+ {
+ Reporter.ToUser(eUserMsgKey.StaticWarnMessage, $"The user configured screenshot {dimensionName} is not valid, using the calculated {dimensionName}");
+ return calculatedValue;
+ }
+
+ if (userConfigured < MIN_SCREENSHOT_DIMENSION)
+ {
+ Reporter.ToUser(eUserMsgKey.StaticWarnMessage, $"The user configured screenshot {dimensionName} is less than {MIN_SCREENSHOT_DIMENSION} pixels, using the calculated {dimensionName} '{calculatedValue}'");
+ return calculatedValue;
+ }
+
+ return userConfigured;
+ }
private void CalculateMobileDeviceScreenSizes()
{
try
{
var windowSize = Driver.Manage().Window.Size;
if (DevicePlatformType == eDevicePlatformType.iOS)
{
var nativeSize = (Dictionary<string, object>)Driver.ExecuteScript("mobile: viewportRect");
mWindowScaleFactor = Convert.ToInt32(nativeSize["width"]) / windowSize.Width;
}
else
{
mWindowScaleFactor = 1;
}
mWindowWidth = (int)(windowSize.Width * mWindowScaleFactor);
mWindowHeight = (int)(windowSize.Height * mWindowScaleFactor);
- //override with user configured sizes
- if (ScreenshotHeight.ToLower().Trim() != "auto")
- {
- //convert from int
- int userConfiguredHeight;
- if (int.TryParse(ScreenshotHeight, out userConfiguredHeight))
- {
- //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message
- if (userConfiguredHeight < 300)
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 300 pixels, using the calculated height '" + mWindowHeight.ToString() +"' instead.");
- }
- else
- {
- mWindowHeight = userConfiguredHeight;
- }
- }
- else
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height");
- }
- }
- if (ScreenshotWidth.ToLower().Trim() != "auto")
- {
- //convert from int
- int userConfiguredWidth;
- if (int.TryParse(ScreenshotWidth, out userConfiguredWidth))
- {
- //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message
- if (userConfiguredWidth < 300)
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 300 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead.");
- }
- else
- {
- mWindowWidth = userConfiguredWidth;
- }
- }
- else
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width.");
- }
- }
+ // Override with user configured sizes
+ mWindowHeight = ValidateScreenshotDimension(ScreenshotHeight, mWindowHeight, "height");
+ mWindowWidth = ValidateScreenshotDimension(ScreenshotWidth, mWindowWidth, "width");
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, "Failed to get Mobile Device Screen Sizes", ex);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| //override with user configured sizes | |
| if (ScreenshotHeight.ToLower().Trim() != "auto") | |
| { | |
| //convert from int | |
| int userConfiguredHeight; | |
| if (int.TryParse(ScreenshotHeight, out userConfiguredHeight)) | |
| { | |
| //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message | |
| if (userConfiguredHeight < 300) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 300 pixels, using the calculated height '" + mWindowHeight.ToString() +"' instead."); | |
| } | |
| else | |
| { | |
| mWindowHeight = userConfiguredHeight; | |
| } | |
| } | |
| else | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height"); | |
| } | |
| } | |
| if (ScreenshotWidth.ToLower().Trim() != "auto") | |
| { | |
| //convert from int | |
| int userConfiguredWidth; | |
| if (int.TryParse(ScreenshotWidth, out userConfiguredWidth)) | |
| { | |
| //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message | |
| if (userConfiguredWidth < 300) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 300 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead."); | |
| } | |
| else | |
| { | |
| mWindowWidth = userConfiguredWidth; | |
| } | |
| } | |
| else | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width."); | |
| } | |
| } | |
| private const int MIN_SCREENSHOT_DIMENSION = 300; | |
| private const string AUTO_VALUE = "auto"; | |
| private int ValidateScreenshotDimension(string configuredValue, int calculatedValue, string dimensionName) | |
| { | |
| if (configuredValue.ToLower().Trim() == AUTO_VALUE) | |
| { | |
| return calculatedValue; | |
| } | |
| if (!int.TryParse(configuredValue, out int userConfigured)) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, $"The user configured screenshot {dimensionName} is not valid, using the calculated {dimensionName}"); | |
| return calculatedValue; | |
| } | |
| if (userConfigured < MIN_SCREENSHOT_DIMENSION) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, $"The user configured screenshot {dimensionName} is less than {MIN_SCREENSHOT_DIMENSION} pixels, using the calculated {dimensionName} '{calculatedValue}'"); | |
| return calculatedValue; | |
| } | |
| return userConfigured; | |
| } | |
| private void CalculateMobileDeviceScreenSizes() | |
| { | |
| try | |
| { | |
| var windowSize = Driver.Manage().Window.Size; | |
| if (DevicePlatformType == eDevicePlatformType.iOS) | |
| { | |
| var nativeSize = (Dictionary<string, object>)Driver.ExecuteScript("mobile: viewportRect"); | |
| mWindowScaleFactor = Convert.ToInt32(nativeSize["width"]) / windowSize.Width; | |
| } | |
| else | |
| { | |
| mWindowScaleFactor = 1; | |
| } | |
| mWindowWidth = (int)(windowSize.Width * mWindowScaleFactor); | |
| mWindowHeight = (int)(windowSize.Height * mWindowScaleFactor); | |
| // Override with user configured sizes | |
| mWindowHeight = ValidateScreenshotDimension(ScreenshotHeight, mWindowHeight, "height"); | |
| mWindowWidth = ValidateScreenshotDimension(ScreenshotWidth, mWindowWidth, "width"); | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, "Failed to get Mobile Device Screen Sizes", ex); | |
| } | |
| } |
… still allowing to control the device
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
1349-1350: Fix typo in method name.The method name
GetUserCustomeScreenshotSizecontains a spelling error. It should beGetUserCustomScreenshotSize.- Tuple<int?, int?> customSize = GetUserCustomeScreenshotSize(); + Tuple<int?, int?> customSize = GetUserCustomScreenshotSize();
1732-1743: Improve swipe validation code style.Consider these improvements:
- Define constants for the magic numbers
- Use string formatting for consistent message style
+ private const double MIN_SWIPE_SCALE = 0.1; + private const double MAX_SWIPE_SCALE = 2.0; + private const int DEFAULT_SWIPE_DURATION = 200; + //valide swipeScale is between 0.1 and 2 and if not set it to 1 and write warn messaage - if (swipeScale < 0.1 || swipeScale > 2) + if (swipeScale < MIN_SWIPE_SCALE || swipeScale > MAX_SWIPE_SCALE) { - Reporter.ToLog(eLogLevel.WARN, "Mobile action swipeScreen(): swipeScale: '" + swipeScale + "' is out of range (0.1-2), setting it to 1"); + Reporter.ToLog(eLogLevel.WARN, + string.Format("Mobile action swipeScreen(): swipeScale: '{0}' is out of range ({1}-{2}), setting it to 1", + swipeScale, MIN_SWIPE_SCALE, MAX_SWIPE_SCALE)); swipeScale = 1; } //validate swipeDuration is bigger than 0 and if not set it to 200 and write warn messaage if (swipeDuration.TotalMilliseconds < 0) { - Reporter.ToLog(eLogLevel.WARN, "Mobile action swipeScreen(): swipeDuration: '" + swipeDuration + "' is out of range, setting it to 200"); - swipeDuration = TimeSpan.FromMilliseconds(200); + Reporter.ToLog(eLogLevel.WARN, + string.Format("Mobile action swipeScreen(): swipeDuration: '{0}' is out of range, setting it to {1}", + swipeDuration, DEFAULT_SWIPE_DURATION)); + swipeDuration = TimeSpan.FromMilliseconds(DEFAULT_SWIPE_DURATION); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Ginger/Ginger/Drivers/DriversConfigsEditPages/AppiumDriverEditPage.xaml
🔇 Additional comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
112-121: LGTM! Well-documented screenshot dimension properties.
The new properties for screenshot dimensions are properly configured with appropriate attributes and documentation.
2871-2896: LGTM! Proper handling of custom screenshot dimensions.
The screenshot resizing logic correctly handles the optional width and height parameters.
Comment on lines
+1359
to
+1409
| private Tuple<int?,int?> GetUserCustomeScreenshotSize() | ||
| { | ||
| int? customeWidth = null; | ||
| int? customeHeight = null; | ||
|
|
||
| //override with user configured sizes | ||
| if (ScreenshotWidth.ToLower().Trim() != "auto") | ||
| { | ||
| //convert from int | ||
| int userConfiguredWidth; | ||
| if (int.TryParse(ScreenshotWidth, out userConfiguredWidth)) | ||
| { | ||
| //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message | ||
| if (userConfiguredWidth < 200) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 200 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead."); | ||
| } | ||
| else | ||
| { | ||
| customeWidth = userConfiguredWidth; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width."); | ||
| } | ||
| } | ||
| if (ScreenshotHeight.ToLower().Trim() != "auto") | ||
| { | ||
| //convert from int | ||
| int userConfiguredHeight; | ||
| if (int.TryParse(ScreenshotHeight, out userConfiguredHeight)) | ||
| { | ||
| //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message | ||
| if (userConfiguredHeight < 200) | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 200 pixels, using the calculated height '" + mWindowHeight.ToString() + "' instead."); | ||
| } | ||
| else | ||
| { | ||
| customeHeight = userConfiguredHeight; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height"); | ||
| } | ||
| } | ||
|
|
||
| return new Tuple<int?, int?>(customeWidth, customeHeight); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor screenshot dimension validation for better maintainability.
The current implementation has duplicate code and magic numbers. Consider these improvements:
- Extract the dimension validation to a method
- Define constants for magic numbers
- Use consistent string formatting
+ private const int MIN_SCREENSHOT_DIMENSION = 200;
+ private const string AUTO_VALUE = "auto";
+
+ private int? ValidateScreenshotDimension(string configuredValue, string dimensionName)
+ {
+ if (configuredValue.ToLower().Trim() == AUTO_VALUE)
+ {
+ return null;
+ }
+
+ if (!int.TryParse(configuredValue, out int userConfigured))
+ {
+ Reporter.ToUser(eUserMsgKey.StaticWarnMessage,
+ string.Format("The user configured screenshot {0} is not valid, using the calculated {0}", dimensionName));
+ return null;
+ }
+
+ if (userConfigured < MIN_SCREENSHOT_DIMENSION)
+ {
+ Reporter.ToUser(eUserMsgKey.StaticWarnMessage,
+ string.Format("The user configured screenshot {0} is less than {1} pixels, using the calculated {0}",
+ dimensionName, MIN_SCREENSHOT_DIMENSION));
+ return null;
+ }
+
+ return userConfigured;
+ }
+
private Tuple<int?,int?> GetUserCustomeScreenshotSize()
{
- int? customeWidth = null;
- int? customeHeight = null;
-
- //override with user configured sizes
- if (ScreenshotWidth.ToLower().Trim() != "auto")
- {
- //convert from int
- int userConfiguredWidth;
- if (int.TryParse(ScreenshotWidth, out userConfiguredWidth))
- {
- //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message
- if (userConfiguredWidth < 200)
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 200 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead.");
- }
- else
- {
- customeWidth = userConfiguredWidth;
- }
- }
- else
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width.");
- }
- }
- if (ScreenshotHeight.ToLower().Trim() != "auto")
- {
- //convert from int
- int userConfiguredHeight;
- if (int.TryParse(ScreenshotHeight, out userConfiguredHeight))
- {
- //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message
- if (userConfiguredHeight < 200)
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 200 pixels, using the calculated height '" + mWindowHeight.ToString() + "' instead.");
- }
- else
- {
- customeHeight = userConfiguredHeight;
- }
- }
- else
- {
- Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height");
- }
- }
-
- return new Tuple<int?, int?>(customeWidth, customeHeight);
+ return new Tuple<int?, int?>(
+ ValidateScreenshotDimension(ScreenshotWidth, "width"),
+ ValidateScreenshotDimension(ScreenshotHeight, "height")
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private Tuple<int?,int?> GetUserCustomeScreenshotSize() | |
| { | |
| int? customeWidth = null; | |
| int? customeHeight = null; | |
| //override with user configured sizes | |
| if (ScreenshotWidth.ToLower().Trim() != "auto") | |
| { | |
| //convert from int | |
| int userConfiguredWidth; | |
| if (int.TryParse(ScreenshotWidth, out userConfiguredWidth)) | |
| { | |
| //validate user configured width is at least 300 pixels, if not than use the calculated width and write Warn message | |
| if (userConfiguredWidth < 200) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is less than 200 pixels, using the calculated width '" + mWindowWidth.ToString() + "' instead."); | |
| } | |
| else | |
| { | |
| customeWidth = userConfiguredWidth; | |
| } | |
| } | |
| else | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot width is not valid, using the calculated width."); | |
| } | |
| } | |
| if (ScreenshotHeight.ToLower().Trim() != "auto") | |
| { | |
| //convert from int | |
| int userConfiguredHeight; | |
| if (int.TryParse(ScreenshotHeight, out userConfiguredHeight)) | |
| { | |
| //validate user configured height is at least 300 pixels, if not than use the calculated height and write Warn message | |
| if (userConfiguredHeight < 200) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is less than 200 pixels, using the calculated height '" + mWindowHeight.ToString() + "' instead."); | |
| } | |
| else | |
| { | |
| customeHeight = userConfiguredHeight; | |
| } | |
| } | |
| else | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "The user configured screenshot height is not valid, using the calculated height"); | |
| } | |
| } | |
| return new Tuple<int?, int?>(customeWidth, customeHeight); | |
| } | |
| private const int MIN_SCREENSHOT_DIMENSION = 200; | |
| private const string AUTO_VALUE = "auto"; | |
| private int? ValidateScreenshotDimension(string configuredValue, string dimensionName) | |
| { | |
| if (configuredValue.ToLower().Trim() == AUTO_VALUE) | |
| { | |
| return null; | |
| } | |
| if (!int.TryParse(configuredValue, out int userConfigured)) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, | |
| string.Format("The user configured screenshot {0} is not valid, using the calculated {0}", dimensionName)); | |
| return null; | |
| } | |
| if (userConfigured < MIN_SCREENSHOT_DIMENSION) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, | |
| string.Format("The user configured screenshot {0} is less than {1} pixels, using the calculated {0}", | |
| dimensionName, MIN_SCREENSHOT_DIMENSION)); | |
| return null; | |
| } | |
| return userConfigured; | |
| } | |
| private Tuple<int?,int?> GetUserCustomeScreenshotSize() | |
| { | |
| return new Tuple<int?, int?>( | |
| ValidateScreenshotDimension(ScreenshotWidth, "width"), | |
| ValidateScreenshotDimension(ScreenshotHeight, "height") | |
| ); | |
| } |
Maheshkale447
approved these changes
Nov 28, 2024
Merged
15 tasks
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
Release Notes
New Features
UI Enhancements
Bug Fixes