Add Appium Operations #4074
Add Appium Operations #4074
Conversation
WalkthroughThis PR updates the mobile device editing functionality. The changes include major UI revisions in the XAML file for the mobile device edit page with the addition of several new StackPanels and controls, and corresponding code-behind modifications to bind new input elements and support additional mobile actions. Enhancements extend to the mobile device class with new properties, enums, and actions, coupled with expanded methods in the Appium driver for operations such as rotation, screen recording, file management, and clipboard handling. Additional updates include improvements in the mobile driver window and project resource reorganization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditPage
participant Driver
participant MobileDriverWindow
User->>EditPage: Initiate mobile action (e.g., rotate simulation)
EditPage->>Driver: Call corresponding driver method (RotateSimulation, etc.)
Driver-->>MobileDriverWindow: Send RotateEvent message
MobileDriverWindow->>MobileDriverWindow: Update orientation icon (UpdateRotateIcon)
MobileDriverWindow-->>User: Reflect updated device orientation in UI
Possibly related PRs
Poem
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 (
|
… app state, simulate device rotation, check app is installed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (3)
1445-1456: Guard repeated user prompts.
The calls toReporter.ToUser(eUserMsgKey.StaticWarnMessage, ...)with similar warnings could potentially appear multiple times if screenshot dimensions are invalid. If the user sets both width and height incorrectly, consider consolidating these warnings or gating them so the user sees fewer repeated prompts.Also applies to: 1478-1478
3006-3010: Check for null locator.
AssigningLocateBytoByXPathand retrieving the first XPath locator fromelemInfo.Locatorscan potentially return null if no XPath locator is present. A quick null check before accessingLocateValuecan avoid a runtime exception.- configArgs.LocateValue = elemInfo.Locators.FirstOrDefault(l => l.LocateBy == eLocateBy.ByXPath).LocateValue; + var xpathLocator = elemInfo.Locators.FirstOrDefault(l => l.LocateBy == eLocateBy.ByXPath); + if (xpathLocator == null) + { + // Fallback or set error + configArgs.LocateValue = "//body"; + } + else + { + configArgs.LocateValue = xpathLocator.LocateValue; + }
3042-3042: Expand documentation for drag-and-drop.
The newPerformDrag()method is straightforward, but consider adding a short doc comment to clarify usage, especially regarding the coordinate system and recommended durations. This helps maintain consistency with otherPerform*methods in this class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml(1 hunks)Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs(6 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(11 hunks)
🔇 Additional comments (7)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (7)
333-334: Synchronize StopProcess in Selenium driver.
Line 334 propagates theStopProcessflag to the embedded Selenium driver. This is a good addition for ensuring consistency of driver stopping logic across the hybrid scenario.
998-998: New TapXY case recognized.
This line adds a case label forActMobileDevice.eMobileDeviceAction.TapXY. No issues spotted. It properly delegates to the relevant method.
2990-2991: Validate resizing logic in screenshot.
This new methodGetScreenshotImageFromDriver(int? width, int? height)resizes the native screenshot. The usage of GDI+ in ausingblock is good; just ensure that extremely large or zero dimensions are handled gracefully (e.g., preventing out-of-memory conditions if someone passes extremely large numbers).
3031-3031: Improved pattern matching.
Using pattern matching (if (LocateElementByLocator(LocatorToTest) is IWebElement elem)) is concise and clear. No issues noticed here.
3102-3102: Check for screenshot bytes validity.
Always ensureGetScreenshotImageFromDriver(...)does not return null. If it does, handle or log an error instead of silently returning a null reference to be used downstream.
3426-3430: Good fallback for resource-id-based locators.
This snippet correctly adds a short-circuit to build resource-id locators only if they differ from the main XPath. This approach helps reduce duplication. Nicely done.
3844-3855: Separate distinct actions or rename.
GetTypeMobileIdandOpenDeeplinkare introduced as new methods. They look fine and read cleanly. Make sure that “deeplink” is validated or sanitized if user-provided. Otherwise, no issues seen.
[approve]
| @@ -53,6 +53,7 @@ limitations under the License. | |||
| using OpenQA.Selenium; | |||
| using OpenQA.Selenium.Appium; | |||
| using OpenQA.Selenium.Appium.Android; | |||
| using OpenQA.Selenium.Appium.Enums; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Check usage of newly added import.
The import OpenQA.Selenium.Appium.Enums was added here. If you are not using any enums defined in this namespace, consider removing this import to reduce clutter.
| @@ -67,6 +68,7 @@ | |||
| using System.Linq; | |||
| using System.Net; | |||
| using System.Text; | |||
| using System.Text.RegularExpressions; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Check usage of Text.RegularExpressions.
The System.Text.RegularExpressions namespace was added, but it doesn't appear to be used in the changed lines. If it is unused, removing it might keep the file lean.
| @@ -112,7 +114,7 @@ | |||
| [UserConfigured] | |||
| [UserConfiguredDefault("Auto")] | |||
| [UserConfiguredDescription("Define the Height to set for the mobile device screenshot")] | |||
| public String ScreenshotHeight { get; set;} | |||
| public String ScreenshotHeight { get; set; } | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Maintain consistent property types.
screenshotHeight is stored as a string, which requires parsing later for numeric comparisons. This can lead to potential parsing exceptions and user confusion. Consider strongly typing this property as an integer (or nullable integer) if only numeric values are allowed.
| case ActMobileDevice.eMobileDeviceAction.OpenDeeplink: | ||
| OpenDeeplink(act.URLName.ValueForDriver, GetAppPackage(act), GetTypeMobileId()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.IsKeyboardVisible: | ||
| act.AddOrUpdateReturnParamActual("Is Keyboard Visible", IsKeyboardVisible().ToString()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.IsLocked: | ||
| act.AddOrUpdateReturnParamActual("Is App Locked", IsLocked().ToString()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.IsAppInstalled: | ||
| act.AddOrUpdateReturnParamActual("Is App Installed", IsAppInstalled(GetAppPackage(act)).ToString()); | ||
|
|
||
| break; | ||
| case ActMobileDevice.eMobileDeviceAction.RemoveApp: | ||
| Driver.RemoveApp(GetAppPackage(act)); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.QueryAppState: | ||
| act.AddOrUpdateReturnParamActual("App State", QueryAppState(GetAppPackage(act)).ToString()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.RotateSimulation: | ||
| RotateSimulation(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.RunScript: | ||
| RunScript(act.ActionInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.StartRecordingScreen: | ||
| StartRecordingScreen(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.StopRecordingScreen: | ||
| StopRecordingScreen(act.PathRecording.ValueForDriver); | ||
|
|
||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.HideKeyboard: | ||
| HideKeyboard(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.PushFileToDevice: | ||
| PushFileToDevice(act.LocalFile.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.PullFileFromDevice: | ||
| PullFileFromDevice(act.LocalFile.ValueForDriver, act.ActionInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.SetClipboardText: | ||
| SetClipboardText(act.ActionInput.ValueForDriver); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Evaluate exception-handling consistency for new mobile actions.
These lines introduce several new cases in the MobileDeviceActionHandler, each calling helper methods (e.g., OpenDeeplink, HideKeyboard, PushFileToDevice, etc.). While this expands functionality, note that some methods throw exceptions internally but are not wrapped here. If any of these calls can fail at runtime (e.g., invalid file paths, unavailable device features), consider capturing those exceptions and updating act.Error consistently to provide better test result reporting.
| private Tuple<int?, int?> GetUserCustomeScreenshotSize() | ||
| { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Method name spelling.
GetUserCustomeScreenshotSize() has a small spelling inconsistency ("Custome"). Consider renaming it to GetUserCustomScreenshotSize() for clarity.
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | ||
| <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageDeepLinkVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | ||
| </StackPanel> | ||
| <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | ||
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Save File Recording to Folder :"/> | ||
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | ||
| </StackPanel> | ||
| </StackPanel> | ||
| <StackPanel x:Name="xRunScriptPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Script:"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageVE2" VerticalAlignment="Top" Width="400"/> | ||
| </StackPanel> | ||
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | ||
| <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | ||
| </StackPanel> | ||
| </StackPanel> | ||
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Enter File Name From Device:" Margin="0,5,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Choose Folder:" Margin="0,5,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE2" Width="600" /> | ||
| </StackPanel> | ||
| <StackPanel x:Name="xSetClipboardTextPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Set value to Clipboard Text:" Margin="0,5,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/> | ||
|
|
||
| </StackPanel> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Standardize margin values across panels.
The margin values are inconsistent across different panels, which may lead to misaligned UI elements.
Apply consistent margins:
- Use "5,10,0,0" for panel margins
- Use "5,5,0,0" for control margins within panels
- <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed">
+ <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed">
- <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed">
+ <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed">
- <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/>
+ <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,5,0,0" Width="200"/>
- <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/>
+ <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,5,0,0" Width="200"/>📝 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.
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | |
| <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageDeepLinkVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Save File Recording to Folder :"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xRunScriptPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Script:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE2" VerticalAlignment="Top" Width="400"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Enter File Name From Device:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose Folder:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE2" Width="600" /> | |
| </StackPanel> | |
| <StackPanel x:Name="xSetClipboardTextPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Set value to Clipboard Text:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | |
| <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageDeepLinkVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Save File Recording to Folder :"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xRunScriptPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Script:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE2" VerticalAlignment="Top" Width="400"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Enter File Name From Device:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,5,0,0" Width="200"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose Folder:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE2" Width="600" /> | |
| </StackPanel> | |
| <StackPanel x:Name="xSetClipboardTextPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Set value to Clipboard Text:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,5,0,0" Width="200"/> | |
| </StackPanel> |
🛠️ Refactor suggestion
Standardize naming conventions and fix spelling.
The control naming needs standardization:
- Use consistent suffixes ('VE' vs 'TxtBox' vs 'EV')
- Avoid numbered suffixes (VE2, VE3, VE4)
- Fix spelling of 'Sumilation' to 'Simulation' in control names
Apply this diff to standardize the naming:
- <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/>
+ <Activities:UCValueExpression x:Name="xURLNameVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/>
- <Actions:UCValueExpression x:Name="xPhotoSumilationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" />
+ <Actions:UCValueExpression x:Name="xPhotoSimulationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" />
- <Actions:UCValueExpression x:Name="xAppPackageVE2" VerticalAlignment="Top" Width="400"/>
+ <Actions:UCValueExpression x:Name="xScriptVE" VerticalAlignment="Top" Width="400"/>
- <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" />
+ <Actions:UCValueExpression x:Name="xPushFileVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" />
- <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/>
+ <Actions:UCValueExpression x:Name="xPullFileNameVE" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/>
- <Actions:UCValueExpression x:Name="xPhotoSumilationVE2" Width="600" />
+ <Actions:UCValueExpression x:Name="xPullFolderVE" Width="600" />
- <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/>
+ <Actions:UCValueExpression x:Name="xClipboardTextVE" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/>📝 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.
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | |
| <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageDeepLinkVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Save File Recording to Folder :"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xRunScriptPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Script:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE2" VerticalAlignment="Top" Width="400"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Enter File Name From Device:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE3" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose Folder:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSumilationVE2" Width="600" /> | |
| </StackPanel> | |
| <StackPanel x:Name="xSetClipboardTextPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Set value to Clipboard Text:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE4" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | |
| <Activities:UCValueExpression x:Name="xURLNameVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageDeepLinkVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xRecordingScreenPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Save File Recording to Folder :"/> | |
| <Actions:UCValueExpression x:Name="xPhotoSimulationVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xRunScriptPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Script:"/> | |
| <Actions:UCValueExpression x:Name="xScriptVE" VerticalAlignment="Top" Width="400"/> | |
| </StackPanel> | |
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | |
| <Actions:UCValueExpression x:Name="xPushFileVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> | |
| </StackPanel> | |
| </StackPanel> | |
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Enter File Name From Device:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xPullFileNameVE" VerticalAlignment="Top" Margin="5,8,30,8" Width="200"/> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Choose Folder:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xPullFolderVE" Width="600" /> | |
| </StackPanel> | |
| <StackPanel x:Name="xSetClipboardTextPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |
| <Label Style="{StaticResource $LabelStyle}" Content="Set value to Clipboard Text:" Margin="0,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xClipboardTextVE" VerticalAlignment="Top" Margin="5,7,30,8" Width="200"/> | |
| </StackPanel> |
| xDeepLinkPnl.Visibility = Visibility.Collapsed; | ||
| xRunScriptPnl.Visibility = Visibility.Collapsed; | ||
| xRecordingScreenPnl.Visibility = Visibility.Collapsed; | ||
| xPushToDevicePnl.Visibility = Visibility.Collapsed; | ||
| xPullFromDevicePnl.Visibility = Visibility.Collapsed; | ||
| xSetClipboardTextPnl.Visibility = Visibility.Collapsed; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve maintainability of panel visibility management.
The current approach of managing panel visibility through multiple individual statements could be refactored for better maintainability.
Consider using a dictionary-based approach to manage panel visibility:
+ private readonly Dictionary<eMobileDeviceAction, StackPanel[]> actionPanels;
+
+ public ActMobileDeviceEditPage(ActMobileDevice Act)
+ {
+ InitializeComponent();
+ InitializeActionPanels();
+ mAct = Act;
+ mContext = Context.GetAsContext(Act.Context);
+ BindControls();
+ SetControlsView();
+ }
+
+ private void InitializeActionPanels()
+ {
+ actionPanels = new Dictionary<eMobileDeviceAction, StackPanel[]>
+ {
+ { eMobileDeviceAction.OpenDeeplink, new[] { xDeepLinkPnl } },
+ { eMobileDeviceAction.RunScript, new[] { xRunScriptPnl } },
+ { eMobileDeviceAction.StopRecordingScreen, new[] { xRecordingScreenPnl } },
+ { eMobileDeviceAction.PushFileToDevice, new[] { xPushToDevicePnl } },
+ { eMobileDeviceAction.PullFileFromDevice, new[] { xPullFromDevicePnl } },
+ { eMobileDeviceAction.SetClipboardText, new[] { xSetClipboardTextPnl } }
+ };
+ }
+
+ private void SetControlsView()
+ {
+ // Hide all panels
+ foreach (var panels in actionPanels.Values)
+ {
+ foreach (var panel in panels)
+ {
+ panel.Visibility = Visibility.Collapsed;
+ }
+ }
+
+ // Show panels for current action
+ if (actionPanels.TryGetValue(mAct.MobileDeviceAction, out var visiblePanels))
+ {
+ foreach (var panel in visiblePanels)
+ {
+ panel.Visibility = Visibility.Visible;
+ }
+ }
+ }Also applies to: 282-316
| xPhotoSumilationTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.GetOrCreateInputParam(nameof(ActMobileDevice.SimulatedPhotoPath)), true, true, UCValueExpression.eBrowserType.File, "*"); | ||
|
|
||
| xAppPackageDeepLinkVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionAppPackage, nameof(ActInputValue.Value)); | ||
|
|
||
| xURLNameTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value)); | ||
|
|
||
| xPhotoSumilationVE2.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); | ||
|
|
||
| xAppPackageVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionAppPackage, nameof(ActInputValue.Value)); | ||
| xPhotoSumilationEV.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*"); | ||
|
|
||
| xAppPackageVE3.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | ||
| xAppPackageVE2.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | ||
| xAppPackageVE4.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | ||
|
|
||
| xPhotoSumilationVE.Init(Context.GetAsContext(mAct.Context), mAct.PathRecording, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update control initialization to match standardized naming.
The control initialization code should be updated to match the standardized naming conventions proposed for the XAML file.
Apply this diff to update the initialization code:
- xPhotoSumilationTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.GetOrCreateInputParam(nameof(ActMobileDevice.SimulatedPhotoPath)), true, true, UCValueExpression.eBrowserType.File, "*");
+ xPhotoSimulationTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.GetOrCreateInputParam(nameof(ActMobileDevice.SimulatedPhotoPath)), true, true, UCValueExpression.eBrowserType.File, "*");
- xURLNameTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value));
+ xURLNameVE.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value));
- xPhotoSumilationVE2.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*");
+ xPullFolderVE.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*");
- xPhotoSumilationEV.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*");
+ xPushFileVE.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*");
- xAppPackageVE3.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
+ xPullFileNameVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
- xAppPackageVE2.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
+ xScriptVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
- xAppPackageVE4.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
+ xClipboardTextVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value));
- xPhotoSumilationVE.Init(Context.GetAsContext(mAct.Context), mAct.PathRecording, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*");
+ xPhotoSimulationVE.Init(Context.GetAsContext(mAct.Context), mAct.PathRecording, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*");📝 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.
| xPhotoSumilationTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.GetOrCreateInputParam(nameof(ActMobileDevice.SimulatedPhotoPath)), true, true, UCValueExpression.eBrowserType.File, "*"); | |
| xAppPackageDeepLinkVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionAppPackage, nameof(ActInputValue.Value)); | |
| xURLNameTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value)); | |
| xPhotoSumilationVE2.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); | |
| xAppPackageVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionAppPackage, nameof(ActInputValue.Value)); | |
| xPhotoSumilationEV.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*"); | |
| xAppPackageVE3.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xAppPackageVE2.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xAppPackageVE4.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xPhotoSumilationVE.Init(Context.GetAsContext(mAct.Context), mAct.PathRecording, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); | |
| xPhotoSimulationTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.GetOrCreateInputParam(nameof(ActMobileDevice.SimulatedPhotoPath)), true, true, UCValueExpression.eBrowserType.File, "*"); | |
| xAppPackageDeepLinkVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionAppPackage, nameof(ActInputValue.Value)); | |
| xURLNameVE.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value)); | |
| xPullFolderVE.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); | |
| xPushFileVE.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*"); | |
| xPullFileNameVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xScriptVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xClipboardTextVE.Init(Context.GetAsContext(mAct.Context), mAct.ActionInput, nameof(ActInputValue.Value)); | |
| xPhotoSimulationVE.Init(Context.GetAsContext(mAct.Context), mAct.PathRecording, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); |
| public ActInputValue URLName | ||
| { | ||
| get | ||
| { | ||
| return GetOrCreateInputParam(nameof(URLName), "your-deep-link-url"); | ||
| } | ||
| set | ||
| { | ||
| AddOrUpdateInputParamValue(nameof(URLName), value.ToString()); | ||
| OnPropertyChanged(nameof(URLName)); | ||
| } | ||
| } | ||
| public ActInputValue LocalFile | ||
| { | ||
| get | ||
| { | ||
| return GetOrCreateInputParam(nameof(LocalFile), "/local/path/to/file.txt"); | ||
| } | ||
| set | ||
| { | ||
| AddOrUpdateInputParamValue(nameof(LocalFile), value.ToString()); | ||
| OnPropertyChanged(nameof(LocalFile)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve default values for properties.
The default values in the properties could be more descriptive and follow a consistent pattern.
Apply this diff to improve default values:
- return GetOrCreateInputParam(nameof(URLName), "your-deep-link-url");
+ return GetOrCreateInputParam(nameof(URLName), "scheme://host/path");
- return GetOrCreateInputParam(nameof(LocalFile), "/local/path/to/file.txt");
+ return GetOrCreateInputParam(nameof(LocalFile), "C:\\path\\to\\file");
- return GetOrCreateInputParam(nameof(PathRecording), "~\\FolderName\\fileName");
+ return GetOrCreateInputParam(nameof(PathRecording), "~\\Documents\\Recordings\\recording.mp4");Also applies to: 218-229
| //public ActInputValue Script | ||
| //{ | ||
| // get | ||
| // { | ||
| // return GetOrCreateInputParam(nameof(Script)); | ||
| // } | ||
| // set | ||
| // { | ||
| // AddOrUpdateInputParamValue(nameof(Script), value.ToString()); | ||
| // OnPropertyChanged(nameof(Script)); | ||
| // } | ||
| //} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
Dead code should be removed rather than commented out. If this code needs to be referenced later, it can be found in version control history.
Remove the commented-out Script property:
- //public ActInputValue Script
- //{
- // get
- // {
- // return GetOrCreateInputParam(nameof(Script));
- // }
- // set
- // {
- // AddOrUpdateInputParamValue(nameof(Script), value.ToString());
- // OnPropertyChanged(nameof(Script));
- // }
- //}📝 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.
| //public ActInputValue Script | |
| //{ | |
| // get | |
| // { | |
| // return GetOrCreateInputParam(nameof(Script)); | |
| // } | |
| // set | |
| // { | |
| // AddOrUpdateInputParamValue(nameof(Script), value.ToString()); | |
| // OnPropertyChanged(nameof(Script)); | |
| // } | |
| //} |
| @@ -3780,7 +3840,151 @@ public ObservableList<ElementLocator> GetElementFriendlyLocators(ElementInfo Ele | |||
| { | |||
| throw new NotImplementedException(); | |||
| } | |||
| } | |||
| public string GetTypeMobileId() | |||
There was a problem hiding this comment.
rename to "GetAppPackageNameByOs()"
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="0 0 0 0"/> | ||
| <Activities:UCValueExpression x:Name="xURLNameTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="250"/> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> |
There was a problem hiding this comment.
check option to re-use exoecitng App package control
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <StackPanel HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Choose File:"/> | ||
| <Actions:UCValueExpression x:Name="xPhotoSumilationEV" HorizontalAlignment="Left" VerticalAlignment="Center" Width="600" /> |
There was a problem hiding this comment.
check for re-use file path control
|
|
||
| xURLNameTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.URLName, nameof(ActInputValue.Value)); | ||
|
|
||
| xPhotoSumilationVE2.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); |
There was a problem hiding this comment.
have meaningful names for the controlers and Action fields
There was a problem hiding this comment.
Actionable comments posted: 30
🔭 Outside diff range comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (4)
80-81: Consider splitting SeleniumDriver class into smaller focused classesThe SeleniumDriver class is implementing multiple interfaces and has grown too large (11000+ lines). This violates the Single Responsibility Principle. Consider splitting it into smaller, focused classes like:
- SeleniumElementFinder
- SeleniumWindowManager
- SeleniumActionExecutor
8447-8456: Add input validation for JavaScript executionThe RunJavaScript method executes arbitrary JavaScript without proper input validation or sanitization. This could be a security risk if malicious code is injected.
Consider:
- Adding input validation
- Restricting allowed JavaScript operations
- Using a whitelist of allowed commands
8197-8199: Replace Thread.Sleep with WebDriverWaitUsing Thread.Sleep for fixed delays is not optimal as it:
- Doesn't account for varying load times
- Can make tests run slower than necessary
- May still fail if the delay is too short
Replace with WebDriverWait:
var wait = new WebDriverWait(Driver, TimeSpan.FromSeconds(2)); wait.Until(d => /* condition */);
7373-7380: Improve exception handlingThe catch block swallows exceptions without proper logging or handling. This can make debugging difficult.
Consider:
catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Failed to handle browser alert: {ex.Message}", ex); throw new BrowserOperationException("Failed to handle browser alert", ex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml(4 hunks)Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs(6 hunks)Ginger/Ginger/Drivers/DriversWindows/MobileDriverWindow.xaml.cs(7 hunks)Ginger/Ginger/Ginger.csproj(1 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs(5 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(17 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(1 hunks)Ginger/GingerCoreNET/RunLib/DriverBase.cs(1 hunks)
🔇 Additional comments (45)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs (3)
21-22: No issues with new using statements.Also applies to: 25-26, 29-29
299-364: Refactor panel visibility logic for maintainability.A dictionary-based approach or a consolidated method can reduce duplicated code blocks in this switch statement. This approach was suggested in a previous review comment. Consider unifying the logic to avoid scattered
Visibilitychanges across multiple cases.
78-78: 🧹 Nitpick (assertive)Fix naming typo for the photo simulation text box.
Currently, the control is named “xPhotoSumilationTxtBox,” but the intended word seems to be “Simulation.”
Consider updating the control name to maintain clarity and consistency:- xPhotoSumilationTxtBox + xPhotoSimulationTxtBoxLikely invalid or redundant comment.
Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (3)
78-89: Property for RotateDeviceState looks correct.Initialization aligns with existing patterns, and using
GetOrCreateInputParamensures a default value ofeRotateDeviceState.Portrait. The code is coherent and straightforward.
91-102: Property for PerformanceTypes is consistent with existing structure.The approach correctly follows the input-parameter pattern. No issues found with the default assignment.
447-482: Add usage validation or unit testing for newly added actions.Multiple actions have been added (e.g.,
OpenDeeplink,RunScript, etc.). Verify they are exercised by unit or integration tests to ensure coverage and prevent regressions. Would you like to generate a script to search for corresponding usage/tests throughout the codebase?Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (24)
51-51: Check usage of Microsoft.Graph
If references from Microsoft.Graph are not used, consider removing this import.
57-57: Check usage of newly added import
75-75: Usage of File alias looks valid
No issues found with this usage. It's referencing System.IO.File methods (e.g., WriteAllBytes).
76-76: Usage of Image alias looks valid
No issues found with your usage. It's referencing System.Drawing.Image for screenshot resizing operations.
1248-1251: Check for potential exception
The call to IsKeyboardVisible() could fail in certain environments or unsupportive device platforms. Verify that exceptions are gracefully handled.
1252-1255: Expose device lock status
This case retrieves the lock status. Consider adding logging if you need real-time monitoring.
1256-1260: App installation check
Implementation looks correct. Confirm that iOS and Android package/bundle naming is fully consistent.
1264-1267: Return value usage
AppState is added to act’s ReturnParam. Logging the final state might help in troubleshooting.
1290-1293: Check recording constraints
Ensure that the selected bit rate/duration are valid on all targeted devices.
1294-1301: Confirm file path logic
Double-check that the final file name is unique or doesn’t cause collisions if the user calls this action repeatedly.
1302-1305: Error handling
Ensure that calling HideKeyboard when the keyboard is already hidden will not raise an unhandled exception.
1310-1313: Validate local file path
Confirm that the directory exists or create it before writing.
1363-1365: Return param usage
The text is fetched from the clipboard. If needed, store it in a named return parameter for clarity.
3929-3937: Cross-platform check
This calls different scripts for Android vs iOS. Ensure unknown platforms gracefully fail or log messages.
3938-3941: Potential exceptions
Driver.IsKeyboardShown() might fail on certain iOS builds. Consider catching or ignoring exceptions for unsupported versions.
3942-3945: Code clarity
Using ExecuteScript("mobile:isLocked") is straightforward. Confirm it’s supported across your targeted OS versions.
3946-3949: Valid usage
IsAppInstalled aligns with standard driver methods.
3950-3953: Confirm app-specific usage
Check if iOS removal expects a bundle ID instead of a path.
3954-3957: Ensure consistent usage
QueryAppState uses driver-specific logic. Double-check for edge cases on older OS versions.
3980-3994: Bitrate and time limit
Check older devices’ constraints; some may struggle with high bit rates or 30-minute recordings.
4007-4026: Driver differences
Hiding the keyboard is handled differently on iOS and Android. Catch or log exceptions if the keyboard is already hidden.
4070-4103: Data structure
Performance data is stored in a dictionary. For advanced scenarios, consider a typed model for clarity.
4137-4140: Method usage
If called on Android, consider throwing an “unsupported” exception or ignoring it.
4147-4150: Expand cross-platform
GetClipboardText is Android-only. iOS might require a different approach if needed.Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml (4)
6-6: Check newly added namespaces
If some of these namespaces are unused, consider removing them to keep the file clean.
20-20: Value expression usage
Verify that “xInputVE” is properly bound in the code-behind to update the action’s data.
105-108: Performance data UI
The combo box is a good addition. Ensure it’s bound to the newly introduced “PerformanceTypes” property.
109-112: Device Rotation Panel
Make sure it’s shown only for “RotateSimulation.” Good addition for user-friendly orientation changes.Ginger/GingerCoreNET/RunLib/DriverBase.cs (1)
202-203: LGTM! The new enum value for rotation events is correctly added.The addition of
RotateEventto theeDriverMessageTypeenum is well-placed and follows the existing pattern. This enum value will be used to handle device rotation events in the driver.Ginger/Ginger/Drivers/DriversWindows/MobileDriverWindow.xaml.cs (3)
206-212: LGTM! The new method correctly handles UI thread synchronization.The
UpdateRotateIcon()method properly usesDispatcher.Invoketo ensure UI updates occur on the UI thread.
278-283: LGTM! The rotation event handler is correctly implemented.The
RotateEventcase in the switch statement follows the established pattern and properly callsUpdateRotateIcon().
1319-1339: LGTM! The method has proper error handling and logging.The
SetOrientationButton()method:
- Correctly handles device orientation changes
- Includes appropriate error handling and logging
- Changed from private to public to support external calls
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
353-356: Approve change of UnhandledPromptBehavior default valueThe change from "Ignore" to "DismissAndNotify" as the default value for UnhandledPromptBehavior is a good improvement. This makes the behavior more explicit and helps with debugging by notifying when unhandled prompts occur.
Ginger/Ginger/Ginger.csproj (6)
15-23: Obsolete Resource Removal and Cleanup
The removal of compile and embedded resource entries for
"Reports\GingerExecutionReport\HTMLReports\**"and
"SolutionWindows\TreeViewItems\NewFolder\**"
appears to be an intentional cleanup of obsolete files. Please verify that none of these files/resources are referenced elsewhere in the solution.
25-31: Confirm Removal of Legacy Code Files
The project file now omits references to files such as"ALM\Repository\JiraRepository.cs"and several POM wizard pages. Ensure that these files are truly deprecated and that no dependent functionality still requires them.
33-41: Documentation and Resource Cleanup Verification
A number of markdown files and additional resource entries (for example, in theNoneitems) have been removed. Double-check that relevant documentation (such as Help Library updates) is adjusted accordingly and that no UI or runtime components inadvertently rely on these assets.
743-754: New Project Reference for GingerWPFDriverWindow
At line 753, a new project reference has been added:
<ProjectReference Include="..\GingerWPFDriverWindow\GingerWPFDriverWindow.csproj" />
This addition aligns with the broader restructuring efforts (and likely supports the Appium operations enhancements). Please verify that this referenced project is current, compiles without issues, and that its dependency versions are compatible with the main Ginger solution.
835-2538: Extensive Resource Entries Review
There is an extensive section of<Resource>entries (from line 835 to 2538) that set many image and asset files with<CopyToOutputDirectory>Never</CopyToOutputDirectory>. This cleanup considerably reduces the build output by excluding unnecessary assets. Please double-check that none of these removed or modified resources are still used by the application (e.g. in the UI or help documentation) to avoid runtime issues due to missing assets.
2540-2617: Validation of None Update Configurations
The final item group (lines 2540–2617) updates a number of files (including reports, settings, and help documents) with specificCopyToOutputDirectorysettings (e.g.Always,PreserveNewest). Confirm that these configurations ensure that all necessary runtime resources are correctly available during execution, especially given the new integration points introduced by the Appium operations.
| xPulltoFolderTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActInputValue.Value), true, true, UCValueExpression.eBrowserType.Folder, "*"); | ||
|
|
||
| xPushtoDeviceFolderTxtBox.Init(Context.GetAsContext(mAct.Context), mAct.LocalFile, nameof(ActMobileDevice.Value), true, true, UCValueExpression.eBrowserType.File, "*"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve naming consistency for folder textboxes.
The current names “xPulltoFolderTxtBox” and “xPushtoDeviceFolderTxtBox” partially use lowercase “to.” Renaming them to “xPullToFolderTxtBox” and “xPushToDeviceFolderTxtBox” would align better with typical CamelCase conventions.
| private void BrowseButton_Click(object sender, RoutedEventArgs e) | ||
| { | ||
| string filePath = UpdateBaseLineImage(); | ||
| ImportPhotoToSolutionFolder(filePath); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling for BrowseButton_Click.
Currently, the method silently fails if filePath is invalid or unreadable. Consider adding basic error handling or user feedback to manage such scenarios more gracefully:
private void BrowseButton_Click(object sender, RoutedEventArgs e)
{
string filePath = UpdateBaseLineImage();
+ if (string.IsNullOrEmpty(filePath))
+ {
+ Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "File path is invalid. Please recheck.");
+ return;
+ }
ImportPhotoToSolutionFolder(filePath);
}📝 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.
| private void BrowseButton_Click(object sender, RoutedEventArgs e) | |
| { | |
| string filePath = UpdateBaseLineImage(); | |
| ImportPhotoToSolutionFolder(filePath); | |
| } | |
| private void BrowseButton_Click(object sender, RoutedEventArgs e) | |
| { | |
| string filePath = UpdateBaseLineImage(); | |
| if (string.IsNullOrEmpty(filePath)) | |
| { | |
| Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "File path is invalid. Please recheck."); | |
| return; | |
| } | |
| ImportPhotoToSolutionFolder(filePath); | |
| } |
| public enum ePerformanceTypes | ||
| { | ||
| [EnumValueDescription("cpuinfo")] | ||
| Cpuinfo, | ||
| [EnumValueDescription("memoryinfo")] | ||
| Memoryinfo, | ||
| [EnumValueDescription("batteryinfo")] | ||
| Batteryinfo, | ||
| [EnumValueDescription("networkinfo")] | ||
| Networkinfo, | ||
| [EnumValueDescription("diskinfo")] | ||
| Diskinfo, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider naming consistency for new enums.
Members like “Cpuinfo” and “Batteryinfo” use lowercase within the enum. To improve readability, consider renaming them with a consistent pattern (e.g., “CPUInfo,” “BatteryInfo”). Similarly, ensure that eRotateDeviceState’s members consistently match your casing conventions.
Also applies to: 343-349
|
|
||
|
|
||
| public ActInputValue LocalFile | ||
| { | ||
| get | ||
| { | ||
| return GetOrCreateInputParam(nameof(LocalFile), ""); | ||
| } | ||
| set | ||
| { | ||
| AddOrUpdateInputParamValue(nameof(LocalFile), value.ToString()); | ||
| OnPropertyChanged(nameof(LocalFile)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against potential null reference in LocalFile property setter.
If consumers of the property set LocalFile to null, .ToString() may cause a NullReferenceException. A small safety check can improve robustness:
public ActInputValue LocalFile
{
get
{
return GetOrCreateInputParam(nameof(LocalFile), "");
}
set
{
- AddOrUpdateInputParamValue(nameof(LocalFile), value.ToString());
+ AddOrUpdateInputParamValue(nameof(LocalFile), value?.ToString() ?? string.Empty);
OnPropertyChanged(nameof(LocalFile));
}
}📝 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.
| public ActInputValue LocalFile | |
| { | |
| get | |
| { | |
| return GetOrCreateInputParam(nameof(LocalFile), ""); | |
| } | |
| set | |
| { | |
| AddOrUpdateInputParamValue(nameof(LocalFile), value.ToString()); | |
| OnPropertyChanged(nameof(LocalFile)); | |
| } | |
| } | |
| public ActInputValue LocalFile | |
| { | |
| get | |
| { | |
| return GetOrCreateInputParam(nameof(LocalFile), ""); | |
| } | |
| set | |
| { | |
| AddOrUpdateInputParamValue(nameof(LocalFile), value?.ToString() ?? string.Empty); | |
| OnPropertyChanged(nameof(LocalFile)); | |
| } | |
| } |
| case ActMobileDevice.eMobileDeviceAction.OpenDeeplink: | ||
| OpenDeeplink(act.ActionInput.ValueForDriver, act.ActionAppPackage.ValueForDriver, GetAppPackageNameByOs()); | ||
| break; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure traceability for the OpenDeeplink operation
This new case properly calls OpenDeeplink. Consider logging or setting “act.Error” if the method fails.
| @@ -61,9 +61,9 @@ | |||
| <UserControlsLib1:UcLabel Content="Note: Andoid device will simulate FingerPrint, iOS devices will simulate FaceID" FontSize="9"></UserControlsLib1:UcLabel> | |||
| </StackPanel> | |||
|
|
|||
| <StackPanel x:Name="xAppPnl" HorizontalAlignment="Left" Orientation="Horizontal" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | |||
| <StackPanel x:Name="xAppPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="6,10,0,0" Visibility="Collapsed"> | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
AppPnl container
Make sure the panel’s margin and visibility logic remain consistent with other panels.
| <StackPanel x:Name="xDeepLinkPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="URL:" Margin="2,0,0,0"/> | ||
| <Activities:UCValueExpression x:Name="xURLPathTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Margin="5,0,0,0" Width="330"/> | ||
| </StackPanel> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New Deep Link panel
This panel is well-labeled. Confirm that it only appears for “OpenDeeplink” action.
| <StackPanel x:Name="xPushToDevicePnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Device Target Folder:" Margin="-4,0,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xPushFileNameTxtBox" HorizontalAlignment="Left" Margin="0,0,0,0" Width="330"/> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Local File Path:" Margin="-2,2,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xPushtoDeviceFolderTxtBox" HorizontalAlignment="Left" VerticalAlignment="Center" Width="340" /> | ||
| </StackPanel> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PushToDevice panel
Consider standardizing the control names (e.g., “xPushFileNameTxtBox” vs “xPushtoDeviceFolderTxtBox”) for clarity.
| <StackPanel x:Name="xPullFromDevicePnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="5,10,0,0" Visibility="Collapsed"> | ||
| <Label x:Name="xFileNamebl" Style="{StaticResource $LabelStyle}" Content="Device File Path:" Margin="-4,0,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xFileNameTxtBox" HorizontalAlignment="Left" Margin="0,0,0,0" Width="330"/> | ||
| <Label x:Name="xFilePathbl" Style="{StaticResource $LabelStyle}" Content="Local Target Folder:" Margin="-3,0,0,0"/> | ||
| <Actions:UCValueExpression x:Name="xPulltoFolderTxtBox" Width="340" /> | ||
| </StackPanel> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
PullFromDevice panel
Check margin consistency and reevaluate naming (e.g., “xFileNameTxtBox” vs “xPulltoFolderTxtBox”).
| <Label Style="{StaticResource $LabelStyle}" Content="Application package:"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageVE" VerticalAlignment="Top" Width="205"/> | ||
| <Actions:UCValueExpression x:Name="xAppPackageVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="330" Margin="-1,5,0,0"/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine negative margin
Using a negative margin can lead to unpredictable layout shifts.
- Margin="-1,5,0,0"
+ Margin="5,5,0,0"📝 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.
| <Actions:UCValueExpression x:Name="xAppPackageVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="330" Margin="-1,5,0,0"/> | |
| <Actions:UCValueExpression x:Name="xAppPackageVE" HorizontalAlignment="Left" VerticalAlignment="Center" Width="330" Margin="5,5,0,0"/> |
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs(5 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(17 hunks)
🔇 Additional comments (41)
Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (5)
78-89: Property implementation looks consistent
TheRotateDeviceStateproperty follows the typical pattern of usingGetOrCreateInputParamin its getter and ensures the property is observable viaOnPropertyChanged.
91-102: No issues with the new PerformanceTypes property
This also follows the same pattern of property definitions in the class. Just ensure references toPerformanceTypesthroughout the codebase are correct and tested.
145-156: Prevent potential NullReferenceException in LocalFile setter
Ifvalueis null, callingvalue.ToString()will cause a crash. This is the same feedback mentioned previously.- AddOrUpdateInputParamValue(nameof(LocalFile), value.ToString()); + AddOrUpdateInputParamValue(nameof(LocalFile), value?.ToString() ?? string.Empty);
327-339: Maintain naming consistency in ePerformanceTypes
Enum members likeCpuinfoandBatteryinfomight read more clearly asCPUInfoandBatteryInfofor consistent .NET naming.
343-349: Enum eRotateDeviceState is named consistently
Your approach to using PascalCase forLandscapeandPortraitaligns well with .NET conventions.Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (36)
57-57: Import is actively used by ScreenOrientation.
This import, once flagged in a past review, is now valid becauseScreenOrientationfromOpenQA.Selenium.Appium.Enumsis referenced in the code.
180-180: No functional change.
This blank line addition has no functional impact.
339-341: Synchronize StopProcess with mSeleniumDriver.
This ensures that the Selenium driver’sStopProcessproperty remains in sync with the parent driver.
1248-1251: Check cross-platform compatibility.
WhileIsKeyboardVisible()works for most Android/iOS contexts, older versions or alternative Appium drivers might not fully support it.
1252-1255: No issues identified for IsLocked().
The call toDriver.ExecuteScript("mobile:isLocked")is valid for appium-based drivers.
1260-1263: Catch exceptions when removing an app.
If removing the app fails (e.g., not installed), consider wrapping in try/catch and settingact.Error.
1264-1267: QueryAppState usage looks good.
This code properly delegates calls to Android or iOS.
1268-1287: Add a default case for device rotation.
Currently only “Landscape” and “Portrait” are handled. Consider a default to guard against invalid enumerations.
1288-1291: Validate arbitrary script input.
Running arbitrary scripts can pose security concerns. Restrict or sanitize untrusted inputs if relevant.
1308-1311: Review parameter usage for PushFileToDevice.
Local vs. target folder may be reversed; the method signature is(string localFilePath, string DeviceTargerFolder), but it’s called with(ActionInput, LocalFile).
1312-1315: Potential confusion for PullFileFromDevice.
As with push, confirm that the arguments match their intended usage.
1316-1319: Clipboard text is Android-only.
If iOS is not supported, consider documenting or throwing a more descriptive exception.
1320-1325: Validate file path & handle IO exceptions.
This code appends logs to a file but does not validate path existence. If path is invalid or locked, it may crash.
1326-1359: Performance data extraction for Android only.
If iOS support is not required, add a clarifying comment or fallback path.
1360-1363: Clipboard text retrieval is Android-only.
If iOS is needed, consider a fallback or explicit “not supported” message.
1501-1551: Correct method name spelling.
There’s a small spelling inconsistency (“Custome”). Consider renaming toGetUserCustomScreenshotSize().
3020-3065: Resize logic is correct.
The screenshot resizing approach is valid. No immediate concerns.
3137-3162: Handle orientation differences gracefully.
BothSwitchToLandscape()andSwitchToPortrait()look fine. Consider verifying device capabilities.
3429-3430: Check for zero or negative dimensions.
IfwindowSize.WidthornativeSize["width"]is unexpectedly 0, scaling could fail. Consider verifying safe ranges.
3923-3925: GetAppPackageNameByOs naming is consistent.
The method is used for the deep link logic, no concerns.
3944-3946: No issues identified for IsKeyboardVisible().
This is a straightforward call toDriver.IsKeyboardShown().
3956-3958: Catch uninstallation exceptions.
Users might attempt removal of a non-existing package.
3960-3962: QueryAppState logic is correct.
Both Android and iOS branches are handled.
3964-3973: RotateSimulation includes only two states.
Consider a default/fallback if an unexpected string is provided.
3974-3984: RunScript can pose security risks.
As previously noted, treat untrusted script input with caution.
3985-3999: Android-only screen recording.
If iOS is needed, add parallel logic or clarify in docs.
4000-4009: StopRecordingScreen method is Android-only.
Explaining or handling iOS usage can prevent confusion.
4030-4044: PushFileToDevice parameter usage.
As noted, confirm that “localFilePath” and “DeviceTargerFolder” are in the correct order.
4046-4066: PullFileFromDevice naming is reversed.
Similar to push; watch for local vs. device folder naming.
4068-4072: Android-only for SetClipboardText.
If iOS support is needed, consider implementing or returning “Not supported.”
4073-4097: GetSpecificPerformanceData for Android only.
If iOS is needed, add a fallback or consider a cross-platform library.
4099-4116: Device logs rely on “logcat.”
No fallback for iOS, which lacks logcat.
4122-4126: ClearAppdata can remove user data.
Ensure testers are aware of possible data loss before calling.
4128-4130: GetClipboardText is Android-only.
Similar advice applies if iOS is required.
4133-4146: InstallApp: verify existence of app files.
If the file doesn’t exist, it should be caught before calling.
1244-1247: 🧹 Nitpick (assertive)Add exception handling for deep-link failures.
Consider wrappingOpenDeeplinkin try/catch to capture any exceptions (e.g., invalid URL). Then setact.Errorto help with test result reporting.try { OpenDeeplink(...); } catch(Exception ex) { act.Error = ex.Message; }Likely invalid or redundant comment.
| @@ -48,11 +48,13 @@ limitations under the License. | |||
| using GingerCore.Actions.VisualTesting; | |||
| using GingerCore.Drivers; | |||
| using GingerCoreNET.SolutionRepositoryLib.RepositoryObjectsLib.PlatformsLib; | |||
| using Microsoft.Graph; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing the unused import.
It appears that nothing in this file references any types from Microsoft.Graph. If confirmed unused, removing it can help keep the file lean.
| case ActMobileDevice.eMobileDeviceAction.IsAppInstalled: | ||
| act.AddOrUpdateReturnParamActual("Is App Installed", IsAppInstalled(act.ActionAppPackage.ValueForDriver).ToString()); | ||
|
|
||
| break; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Check for driver exceptions.
IsAppInstalled can throw if the driver is in a bad state. Optionally, catch exceptions here and set act.Error.
| case ActMobileDevice.eMobileDeviceAction.StartRecordingScreen: | ||
| StartRecordingScreen(); | ||
| break; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Recording screen for Android only.
If iOS support is needed, consider adding fallback or clarifying that it’s not currently supported.
| case ActMobileDevice.eMobileDeviceAction.StopRecordingScreen: | ||
|
|
||
| string pathRecording = StopRecordingScreen(act.LocalFile.ValueForDriver).ToString(); | ||
| act.AddOrUpdateReturnParamActual("ScreenRecordingFilePath", pathRecording); | ||
| Act.AddArtifactToAction(Path.GetFileName(pathRecording), act, pathRecording); | ||
|
|
||
| break; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Expose exceptions to test reporting.
If stopping screen recording fails, it throws an exception. Consider catching it here to set act.Error.
| case ActMobileDevice.eMobileDeviceAction.HideKeyboard: | ||
| HideKeyboard(); | ||
| break; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Propagate exceptions to act.Error.
The HideKeyboard() method throws if the keyboard is not visible or if it fails. Instead of rethrowing, consider capturing the error in act.Error.
| { | ||
| return (bool)Driver.ExecuteScript("mobile:isLocked"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Method IsLocked() is concise.
Consider capturing exceptions if “mobile:isLocked” is unsupported.
| { | ||
| return Driver.IsAppInstalled(bundleId); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add optional error logging.
If IsAppInstalled fails, you could log the exception in act.Error.
| { | ||
|
|
||
| try | ||
| { | ||
| if (Driver is AndroidDriver) | ||
| { | ||
| ((AndroidDriver)Driver).HideKeyboard("Done"); | ||
| } | ||
| else if (Driver is IOSDriver) | ||
| { | ||
| ((IOSDriver)Driver).HideKeyboard(); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new($"Failed to hide the keyboard: {ex.Message}"); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
HideKeyboard exceptions.
Similar logic: rethrowing might abruptly end the test.
| public string GetTragetFile(string fileType, string path,string FileName) | ||
| { | ||
|
|
||
| string timestamp = DateTime.Now.ToString("yyyyMMddHHmmss"); | ||
| string fileName = Path.GetFileName(path); | ||
| string targetFile = string.IsNullOrEmpty(fileName) ? Path.Combine(path, $"{FileName}_{timestamp}.{fileType}") : $"{path}.{fileType}"; | ||
| return targetFile; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix minor spelling in method name.
GetTragetFile should be GetTargetFile for clarity.
-public string GetTragetFile(string fileType, string path,string FileName)
+public string GetTargetFile(string fileType, string path, string FileName)📝 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.
| public string GetTragetFile(string fileType, string path,string FileName) | |
| { | |
| string timestamp = DateTime.Now.ToString("yyyyMMddHHmmss"); | |
| string fileName = Path.GetFileName(path); | |
| string targetFile = string.IsNullOrEmpty(fileName) ? Path.Combine(path, $"{FileName}_{timestamp}.{fileType}") : $"{path}.{fileType}"; | |
| return targetFile; | |
| } | |
| public string GetTargetFile(string fileType, string path, string FileName) | |
| { | |
| string timestamp = DateTime.Now.ToString("yyyyMMddHHmmss"); | |
| string fileName = Path.GetFileName(path); | |
| string targetFile = string.IsNullOrEmpty(fileName) ? Path.Combine(path, $"{FileName}_{timestamp}.{fileType}") : $"{path}.{fileType}"; | |
| return targetFile; | |
| } |
| using File = System.IO.File; | ||
| using Image = System.Drawing.Image; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify the usage of file aliases.
You’re mixing direct usage of System.IO.File.ReadAllBytes (line 4033) and the File alias (line 4006). Consider consistently using one approach for clarity.
- byte[] fileContent = System.IO.File.ReadAllBytes(DeviceTargerFolder);
+ byte[] fileContent = File.ReadAllBytes(DeviceTargerFolder);📝 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.
| using File = System.IO.File; | |
| using Image = System.Drawing.Image; | |
| using File = System.IO.File; | |
| using Image = System.Drawing.Image; | |
| // ... other code ... | |
| // Somewhere down in the file, the file content is read. | |
| // Original code: | |
| // byte[] fileContent = System.IO.File.ReadAllBytes(DeviceTargerFolder); | |
| // Updated code: | |
| byte[] fileContent = File.ReadAllBytes(DeviceTargerFolder); | |
| // ... rest of the file ... |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(17 hunks)
🔇 Additional comments (44)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (44)
36-46: All imported namespaces appear valid.
These newly addedusingdirectives seem appropriate for the added functionality (image handling, I/O, etc.). No further issues.
67-67: Confirmed usage ofOpenQA.Selenium.Appium.Enums.
This namespace is indeed used forAppStateand other enums, so retaining it is correct.
179-179: No issues found.
This line change appears to be an empty or formatting modification, so there's nothing actionable here.
1235-1235: Return parameter indexing is fine.
Using"Context " + (i + 1)is clear enough to label each returned context.
1243-1246: Ensure traceability for OpenDeeplink.
OpenDeeplink might throw if the driver is invalid. Optionally wrap in try/catch and setact.Errorfor robust error reporting.
1248-1248: Querying keyboard visibility is fine.
No immediate issues spotted.
1252-1252: Consider handling unsupported “mobile:isLocked” scenarios.
If the device or driver doesn’t recognize this, an exception may occur. Optionally catch and setact.Error.
1255-1258: Ensure exceptions are caught if IsAppInstalled fails.
If the driver is in a bad state,IsAppInstalledmight throw. Consider capturing the exception inact.Error.
1259-1262: Handle potential exceptions when removing an app.
If the app removal fails (e.g., missing package), you could capture any exception to improve debugging.
1264-1264: Reporting app state is straightforward.
Implementation looks good.
1267-1285: Refine orientation switching.
Consider adding a default case if the state is neither “Landscape” nor “Portrait,” or produce a meaningful warning.
1287-1290: Validate user-supplied scripts.
Running arbitrary scripts can pose security concerns. Restrict or sanitize as needed.
1291-1294: Recording screen is Android-only.
If iOS support is desired, consider adding fallback or clarifying it’s unsupported there.
1295-1302: Consider capturing exceptions when stopping screen recording.
A failure here could use better test reporting viaact.Error.
1303-1306: Propagate HideKeyboard exceptions.
If the keyboard isn’t visible or it fails to hide, capturing the exception inact.Errorimproves clarity.
1308-1309: Watch parameter usage for pushing files to device.
The naming might be reversed or confusing (localFilePathvs.DeviceTargerFolder). See prior feedback for clarifying usage.
1311-1313: Handling overwrites when pulling files.
Currently no check for existing local files. Confirm whether overwriting is acceptable or if a safeguard is needed.
1315-1317: SetClipboardText is Android-specific.
If needed on iOS, consider a fallback or note it’s not supported.
1319-1323: Retrieving device logs looks good.
Implementation is clear. No immediate concerns.
1325-1358: Performance data extraction is fine.
For clarity and maintainability, you could label the returned metrics more descriptively or use typed objects.
1359-1361: Clipboard retrieval is straightforward.
No issues.
1500-1501: Fix spelling in method name.
RenamingGetUserCustomeScreenshotSize()toGetUserCustomScreenshotSize()would improve clarity.
3428-3428: No problem with updated mobile window-size scaling.
Implementation is consistent with prior usage.
3921-3924: Method name can be more descriptive.
Consider naming itGetPackageIdentifierByPlatform()or similar.
3925-3932: Correct spelling ofGetTragetFile.
Should beGetTargetFile.
3933-3940: Gracefully handle non-Android/iOS drivers for deepLink.
If neither Android nor iOS, this cast fails. Add a fallback error for clarity.
3943-3945: Keyboard visibility method is succinct.
Looks good; no further concerns.
3947-3949: Capture exceptions if “mobile:isLocked” is unsupported.
Keep previous feedback in mind to avoid unhandled errors.
3951-3953: Log or handle driver issues for IsAppInstalled.
If the driver is invalid, an exception may be thrown.
3954-3957: RemoveApp might fail if the app is missing.
Optionally catch and store inact.Error.
3959-3961: Querying app state looks fine.
Implementation is straightforward.
3962-3972: Add default handling for unknown "RotateSimulation" states.
This is a repeat recommendation from prior feedback.
3973-3983: Restrict or sanitize user scripts.
Executing arbitrary code is a security risk.
3984-3997: Android-only screen recording.
If iOS support is needed, consider a fallback or clarify it’s not supported.
3999-4007: StopRecordingScreen: check for errors.
Handle exceptions or invalid base64 data to avoid silent failure.
4010-4028: HideKeyboard can fail if the keyboard is not present.
Consider capturing exceptions inact.Error.
4029-4043: PushFileToDevice parameter mismatch.
Double-check “localFilePath” vs “DeviceTargerFolder.” Also handle potential read errors.
4044-4065: Overwrite caution when pulling files.
If the file exists locally, confirm or skip overwriting.
4066-4071: Add iOS check for SetClipboardText if needed.
Currently only calls Android’s method.
4072-4096: Performance data retrieval is fine.
As noted before, typed structures can improve clarity.
4097-4115: Retrieving device logs is clear.
No immediate issues. The append mode is a straightforward approach.
4120-4125: Clearing app data can lead to data loss.
Ensure testers acknowledge the potential side effects.
4126-4129: GetClipboardText for Android only.
Implementation is straightforward unless iOS support is needed.
4131-4147: Validate app path before InstallApp.
If the file is missing or invalid, consider raising an error.
| }; | ||
| mSeleniumDriver.StopProcess = this.StopProcess; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove duplicated assignment.
Line 339 and line 340 both set mSeleniumDriver.StopProcess = this.StopProcess;. Removing the redundant statement is recommended.
338 };
339 mSeleniumDriver.StopProcess = this.StopProcess;
-340 mSeleniumDriver.StopProcess = this.StopProcess;📝 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.
| }; | |
| mSeleniumDriver.StopProcess = this.StopProcess; | |
| }; | |
| mSeleniumDriver.StopProcess = this.StopProcess; |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (5)
80-89: Consider extracting network monitoring functionality into a separate classThe network monitoring related fields and state are mixed in with the core driver functionality. Consider extracting these into a dedicated NetworkMonitor class to improve separation of concerns and maintainability.
+ public class NetworkMonitor { + private IDevToolsSession Session; + private DevToolsSession devToolsSession; + private DevToolsDomains devToolsDomains; + private IDevTools devTools; + private List<Tuple<string, object>> networkResponseLogList; + private List<Tuple<string, object>> networkRequestLogList; + private INetwork interceptor; + private bool isNetworkLogMonitoringStarted = false; + }
1136-1149: Add error handling for custom driver path validationThe custom driver path validation could be improved by adding more specific error handling and path validation.
private void AddCustomDriverPath(DriverService driverService) { if (!string.IsNullOrWhiteSpace(DriverFilePath)) { + try { + if (!Path.GetExtension(DriverFilePath).Equals(".exe", StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException("Driver file must be an executable (.exe)"); + } if (File.Exists(DriverFilePath)) { driverService.DriverServicePath = Path.GetDirectoryName(DriverFilePath); driverService.DriverServiceExecutableName = Path.GetFileName(DriverFilePath); } else { throw new FileNotFoundException($"Invalid path provided in Custom Driver File in {nameof(DriverFilePath)} in Agent Configuration. Please provide valid path or consider removing it if not needed."); } + } + catch (Exception ex) { + throw new DriverServiceException($"Failed to configure custom driver path: {ex.Message}", ex); + } } }
6731-6742: Potential security risk in handling JavaScript executionThe RunJavaScript method executes arbitrary JavaScript code without any validation or sanitization. This could be a security risk if the input is not properly validated.
private void HandleActUIElement(ActUIElement act) { // ... existing code ... case ActUIElement.eElementAction.RunJavaScript: string script = act.GetInputParamCalculatedValue("Value"); try { + // Validate and sanitize JavaScript input + if (!IsJavaScriptSafe(script)) { + throw new SecurityException("Potentially unsafe JavaScript detected"); + } if (string.IsNullOrEmpty(script)) { act.Error = "Script is empty"; } // ... rest of the code } } + private bool IsJavaScriptSafe(string script) { + // Add validation logic here + return true; + }
575-588: Consider implementing retry mechanism with exponential backoffThe ImplicitWait setting uses a fixed timeout value. Consider implementing a retry mechanism with exponential backoff for better reliability.
+ private async Task<bool> WaitWithRetry(Func<bool> condition, int maxAttempts = 3, int initialWaitMs = 1000) + { + for (int i = 0; i < maxAttempts; i++) + { + if (condition()) + return true; + await Task.Delay(initialWaitMs * (int)Math.Pow(2, i)); + } + return false; + }
1-17: Update copyright year in license headerThe copyright year in the license header should be updated to reflect the current year (2025).
- Copyright © 2014-2024 European Support Limited + Copyright © 2014-2025 European Support Limited
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/Ginger/Ginger.csproj(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(1 hunks)
🔇 Additional comments (6)
Ginger/Ginger/Ginger.csproj (5)
14-23: Resource Removal in ItemGroup
The removal of resource and code items (Compile, EmbeddedResource, None, and Page) for the Reports and SolutionWindows folders appears to be a comprehensive cleanup measure. Please verify that none of these resources are still in use elsewhere in the project or referenced by other parts of the solution.
25-31: Removal of Specific Source Files
The removal of specific source files (e.g., JiraRepository.cs, wizard page code-behind files, and related action reports) is consistent with eliminating obsolete or unused code. Double-check that these files are not referenced in other projects or build steps to avoid breaking dependencies.
33-42: Cleanup of Documentation and Support Files
Removing various markdown files and associated documentation (e.g., ActionConversionIntro.md, AddAgentIntro.md, etc.) helps reduce clutter. Ensure that any important documentation has been migrated to the Help Library or another centralized location as noted in the PR objectives.
760-761: New Project Reference Added
A new project reference toGingerWPFDriverWindow.csprojhas been added. This inclusion should support the integration of new UI functionalities (likely related to Appium operations or driver windows). Please confirm that this project builds correctly, its dependencies are managed appropriately, and that no conflicts arise with other references.
1-2626: Overall Project File Reorganization
This csproj file exhibits significant reorganization—including the removal of obsolete resources and source files, updates to resource copy directives, and the integration of a new project reference. These changes align well with the PR objectives for streamlining the project and integrating new functionalities. It is important to run a full build and verify that all runtime resources and dependencies are intact.Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
353-356: Changed default UnhandledPromptBehavior from "Ignore" to "DismissAndNotify"The default value for UnhandledPromptBehavior has been changed from "Ignore" to "DismissAndNotify", which is a safer default as it ensures users are notified of any unhandled prompts while automatically dismissing them. This aligns better with Selenium's recommended behavior.
|
|
||
| Act.AddArtifactToAction(Path.GetFileName(responsePath), act, responsePath); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unnecessary empty line with tilde marker
There is an unnecessary empty line marked with a tilde (~) that should be removed.
-~Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs (3)
78-86: 🧹 Nitpick (assertive)Correct spelling and naming consistency for xPhotoSumilationTxtBox.
“Sumilation” is still misspelled and was flagged in prior reviews. Renaming to “xPhotoSimulationTxtBox” aligns with the recommended naming.Apply this diff to rename:
- xPhotoSumilationTxtBox + xPhotoSimulationTxtBox
149-153: 🧹 Nitpick (assertive)Add basic error handling for invalid file paths.
This method still lacks error handling for null or invalid paths, which was noted previously. Consider showing an error message if the path is invalid.private void BrowseButton_Click(object sender, RoutedEventArgs e) { string filePath = UpdateBaseLineImage(); + if (string.IsNullOrEmpty(filePath)) + { + Reporter.ToUser(eUserMsgKey.StaticWarnMessage, "File path is invalid. Please recheck."); + return; + } ImportPhotoToSolutionFolder(filePath); }
283-345: 🧹 Nitpick (assertive)Consider refactoring panel visibility logic using a dictionary-based approach.
Past comments suggested consolidating panel visibility settings into a data-driven structure for maintainability. Currently, new actions continue to expand the switch statement, making it more complex.Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (4)
91-102: 🧹 Nitpick (assertive)Use consistent naming for ePerformanceTypes.
Enums like “Cpuinfo” and “Batteryinfo” are partially lowercase, which was noted in past reviews. Prefer “CPUInfo,” “BatteryInfo,” etc.
145-156:⚠️ Potential issueAdd null checks for FilePathInput.
When calling value.ToString(), if value is null, it could result in a NullReferenceException. Safely handle nulls to improve robustness.public ActInputValue FilePathInput { get { return GetOrCreateInputParam(nameof(FilePathInput)); } set { - AddOrUpdateInputParamValue(nameof(FilePathInput), value.ToString()); + AddOrUpdateInputParamValue(nameof(FilePathInput), value?.ToString() ?? string.Empty); OnPropertyChanged(nameof(FilePathInput)); } }
158-169: 🧹 Nitpick (assertive)Same null-check recommendation for FolderPathInput.
Apply the same safety check here as for FilePathInput.
341-353: 🧹 Nitpick (assertive)Ensure consistent casing for ePerformanceTypes members.
In prior comments, a recommendation was made to rename members like “Cpuinfo”→“CPUInfo” for clarity.Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml (2)
18-21: 🧹 Nitpick (assertive)Avoid negative margins for consistent layout.
Line 20’s margin of "-1,5,0,0" can lead to misalignments. Revisit the standardized margin approach discussed in past reviews.- <Actions:UCValueExpression x:Name="xAppPackageVE" ... Margin="-1,5,0,0"/> + <Actions:UCValueExpression x:Name="xAppPackageVE" ... Margin="5,5,0,0"/>
90-95: 🧹 Nitpick (assertive)Eliminate negative margins in xFileTransferPnl.
Lines 91 and 93 specify “-4,0,0,0” and “-3,0,0,0,” which can cause layout issues.- <Label x:Name="xFilePathLbl" ... Margin="-4,0,0,0"/> + <Label x:Name="xFilePathLbl" ... Margin="5,0,0,0"/>Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
3901-4152: 🧹 Nitpick (assertive)🛠️ Refactor suggestion
⚠️ Potential issueExtensive new methods for device operations.
- The method “GetTragetFile” is misspelled. Rename to “GetTargetFile” for clarity.
- “PushFileToDevice” and “PullFileFromDevice” usage is reversed, as localFilePath is used as the remote path (and vice versa).
- Validate user-supplied script in “RunScriptOnDevice” to avoid potential security issues.
- For “RemoveDeviceApp” and “IsDeviceAppInstalled,” consider capturing exceptions and setting act.Error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml(3 hunks)Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs(6 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs(5 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(17 hunks)
🔇 Additional comments (15)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs (2)
21-29: Ensure usage references are required.
The newly added using directives look correct at a glance. Verify that all are indeed necessary for the added or modified functionality. If any are unused, removing them will reduce clutter.
230-233: Looks good.
Collapsing the newly introduced panels by default is a clean approach that helps avoid unwanted clutter.Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (2)
78-89: Check naming consistency for eRotateDeviceState property.
The property and default value are well-defined. Ensure references to “RotateSimulation” in the switch statements align with “RotateDeviceState.”
357-363: RotateDeviceState enum specification looks good.
Naming is clear, and the default selection in the property is well-handled.Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml (1)
101-104: xDeviceRotationPnl addition looks good.
Implementation is straightforward, with no apparent concerns regarding naming or control usage.Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (10)
36-46: Imports are consistent with functionality.
These added namespaces appear to be used for new functionality such as image processing (System.Drawing) and XML operations (System.Xml). No immediate concerns.
67-67: This import is actively used for orientation references.
Confirmed usage of ScreenOrientation from OpenQA.Selenium.Appium.Enums, so this import is valid.
114-114: Consider using a strongly typed property.
Storing ScreenshotHeight as a string could lead to parsing errors. Reusing the suggestion from a previous review.
171-171: No functional change introduced.
This appears to be a whitespace update, no action needed.
330-332: No duplication remains for StopProcess assignment.
Previously, there was duplication when setting StopProcess. This is now resolved.
746-746: Trivial whitespace change.
No functional impact.
994-994: Trivial formatting change.
No action needed.
2720-2723: Rotation event notification.
Method is concise; consider including additional context about orientation if needed.
3118-3118: Refined orientation handling.
Ensuring the correct orientation is established after SwitchToLandscape or SwitchToPortrait is beneficial. No major concerns.Also applies to: 3120-3120, 3135-3135
3408-3408: Device scaling.
Applying the scale factor for the window height is consistent with the approach for the width.
| [EnumValueDescription("Open Deep Link")] | ||
| OpenDeeplink, | ||
| [EnumValueDescription("Is Keyboard Visible")] | ||
| IsKeyboardVisible, | ||
| [EnumValueDescription("Is Device Locked")] | ||
| IsLocked, | ||
| [EnumValueDescription("Is App Installed")] | ||
| IsAppInstalled, | ||
| [EnumValueDescription("Remove App")] | ||
| RemoveApp, | ||
| [EnumValueDescription("Get App State")] | ||
| QueryAppState, | ||
| [EnumValueDescription("Simulate Device Rotation")] | ||
| RotateSimulation, | ||
| [EnumValueDescription("Run Script")] | ||
| RunScript, | ||
| [EnumValueDescription("Start Recording Screen")] | ||
| StartRecordingScreen, | ||
| [EnumValueDescription("Stop Recording Screen")] | ||
| StopRecordingScreen, | ||
| [EnumValueDescription("Hide Keyboard")] | ||
| HideKeyboard, | ||
| [EnumValueDescription("Push File to Device")] | ||
| PushFileToDevice, | ||
| [EnumValueDescription("Pull File From Device")] | ||
| PullFileFromDevice, | ||
| [EnumValueDescription("Set Clipboard Text")] | ||
| SetClipboardText, | ||
| [EnumValueDescription("Get Specific Performance Data")] | ||
| GetSpecificPerformanceData, | ||
| [EnumValueDescription("Get Device Logs")] | ||
| GetDeviceLogs, | ||
| [EnumValueDescription("Get Clipboard Text")] | ||
| GetClipboardText, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm consistency of newly added eMobileDeviceAction members.
All new actions appear logically grouped. Double-check naming like “OpenDeeplink” vs. “OpenDeepLink” to align with your standard.
| @@ -3,7 +3,7 @@ | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" | |||
| xmlns:d="http://schemas.microsoft.com/expression/blend/2008" | |||
| xmlns:UserControlsLib1="clr-namespace:Ginger.UserControlsLib" xmlns:Activities="clr-namespace:Ginger.Activities" | |||
| xmlns:UserControlsLib1="clr-namespace:Ginger.UserControlsLib" xmlns:Activities="clr-namespace:Ginger.Activities" xmlns:UserControls="clr-namespace:Ginger.UserControls" | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Namespace usage seems correct.
Ensure “UserControlsLib1” is the final intended name and isn’t leftover from a temporary reference.
| <StackPanel x:Name="xInputPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="7,10,0,0" Visibility="Collapsed"> | ||
| <Label x:Name="xInputLabelVE" Style="{StaticResource $LabelStyle}" Content="Input:"/> | ||
| <Actions:UCValueExpression x:Name="xInputVE" VerticalAlignment="Top" Width="320"/> | ||
| <Actions:UCValueExpression x:Name="xInputVE" VerticalAlignment="Top" Width="330"/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Check margin consistency for xInputPnl.
Margin is set to “7,10,0,0,” differing slightly from other panels (often “5,10,0,0”). Consider standardizing for a uniform UI.
| <StackPanel x:Name="xSpecificPerformanceDataPnl" HorizontalAlignment="Left" Orientation="Vertical" DockPanel.Dock="Top" Margin="0,2,0,0" Visibility="Collapsed"> | ||
| <Label Style="{StaticResource $LabelStyle}" Content="Data Type:" Margin="0,0,0,0"/> | ||
| <UserControlsLib1:UCComboBox x:Name="xDataTypeComboBox" HorizontalAlignment="Left" VerticalAlignment="Center" Margin="4,-6,0,0" Width="250"/> | ||
| </StackPanel> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use consistent margins for xSpecificPerformanceDataPnl.
Currently set to “0,2,0,0,” which is slightly out of step with other default “5,10,0,0” or “5,5,0,0” usage.
| case ActMobileDevice.eMobileDeviceAction.IsAppInstalled: | ||
| act.AddOrUpdateReturnParamActual("Is App Installed", IsDeviceAppInstalled(act.ActionAppPackage.ValueForDriver).ToString()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.RemoveApp: | ||
| RemoveDeviceApp(act.ActionAppPackage.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.QueryAppState: | ||
| act.AddOrUpdateReturnParamActual("App State", QueryAppState(act.ActionAppPackage.ValueForDriver).ToString()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.RotateSimulation: | ||
| switch (act.RotateDeviceState) | ||
| { | ||
| case ActMobileDevice.eRotateDeviceState.Landscape: | ||
| { | ||
| SwitchToLandscape(); | ||
| break; | ||
| } | ||
| case ActMobileDevice.eRotateDeviceState.Portrait: | ||
| { | ||
| SwitchToPortrait(); | ||
| break; | ||
| } | ||
| } | ||
| NotifyDeviceRotation(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.RunScript: | ||
| RunScriptOnDevice(act.ActionInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.StartRecordingScreen: | ||
| StartDeviceScreenRecording(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.StopRecordingScreen: | ||
| string pathRecording = StopRecordingScreen(act.FilePathInput.ValueForDriver).ToString(); | ||
| act.AddOrUpdateReturnParamActual("ScreenRecordingFilePath", pathRecording); | ||
| Act.AddArtifactToAction(Path.GetFileName(pathRecording), act, pathRecording); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.HideKeyboard: | ||
| HideKeyboard(); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.PushFileToDevice: | ||
| PushFileToDevice(act.FilePathInput.ValueForDriver, act.FolderPathInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.PullFileFromDevice: | ||
| PullFileFromDevice(act.FilePathInput.ValueForDriver, act.FolderPathInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.SetClipboardText: | ||
| SetClipboardText(act.ActionInput.ValueForDriver); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.GetClipboardText: | ||
| act.AddOrUpdateReturnParamActual("Get Clipboard Text", GetClipboardText()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.GetDeviceLogs: | ||
| string deviceLogsPath = GetDeviceLogs(act.FolderPathInput.ValueForDriver); | ||
| act.AddOrUpdateReturnParamActual("DeviceLogFilePath", deviceLogsPath); | ||
| Act.AddArtifactToAction(Path.GetFileName(deviceLogsPath), act, deviceLogsPath); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.GetSpecificPerformanceData: | ||
| switch (act.PerformanceTypes) | ||
| { | ||
| case ActMobileDevice.ePerformanceTypes.Cpuinfo: | ||
| { | ||
| GetSpecificPerformanceData(act.ActionAppPackage.ValueForDriver, act.PerformanceTypes.ToString(), act); | ||
| break; | ||
| } | ||
| case ActMobileDevice.ePerformanceTypes.Memoryinfo: | ||
| { | ||
| GetSpecificPerformanceData(act.ActionAppPackage.ValueForDriver, act.PerformanceTypes.ToString(), act); | ||
| break; | ||
| } | ||
| case ActMobileDevice.ePerformanceTypes.Batteryinfo: | ||
| { | ||
| GetSpecificPerformanceData(act.ActionAppPackage.ValueForDriver, act.PerformanceTypes.ToString(), act); | ||
| break; | ||
| } | ||
| case ActMobileDevice.ePerformanceTypes.Networkinfo: | ||
| { | ||
| GetSpecificPerformanceData(act.ActionAppPackage.ValueForDriver, act.PerformanceTypes.ToString(), act); | ||
| break; | ||
| } | ||
| case ActMobileDevice.ePerformanceTypes.Diskinfo: | ||
| { | ||
| GetSpecificPerformanceData(act.ActionAppPackage.ValueForDriver, act.PerformanceTypes.ToString(), act); | ||
| break; | ||
| } | ||
| } | ||
| break; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Comprehensive new actions (deeplink, installation checks, rotation, etc.).
- Consider adding try/catch blocks or “act.Error” assignments for calls like OpenDeeplink, RemoveApp, QueryAppState, which may throw exceptions.
- The “PushFileToDevice” and “PullFileFromDevice” parameter usage is reversed (local path vs. device folder), likely breaking file transfers.
- Hiding the keyboard, checking installs, and rotating the device all expand coverage. Watch for exceptions in disconnected driver scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs(6 hunks)Ginger/Ginger/Drivers/DriversWindows/MobileDriverWindow.xaml.cs(6 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(16 hunks)
🔇 Additional comments (46)
Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs (20)
31-31: No significant change detected.
43-43: No significant change detected.
53-53: No significant change detected.
60-61: No significant change detected.
73-73: Rename “xPhotoSumilationTxtBox” for consistency.
The control name contains a typographical error ("Sumilation"). This was noted in previous review comments and remains valid.
75-75: Initialization of new combo box.
Looks good. The approach is consistent with other controls.
77-77: Use of xDataTypeComboBox.
Implementation appears fine. Ensure that “PerformanceTypes” is properly validated and tested.
79-81: File path and folder path textboxes.
These controls are initialized consistently and match the existing pattern for path-based fields.
225-228: Collapsing new panels by default.
All new panels are hidden before switching on specific actions. This is consistent with the existing approach in SetControlsView().
278-283: Handling “OpenDeeplink” action.
Makes sense to reuse xAppPnl and xInputPnl for capturing deeplink input.
286-291: Showing xAppPnl for new app-related actions.
Actions “IsAppInstalled”, “RemoveApp”, and “QueryAppState” reasonably share xAppPnl.
296-296: No significant change detected.
297-300: Adding “RunScript” action.
Please recall the security caution when running scripts. A similar concern was previously raised about untrusted inputs.
302-306: Supporting “StartRecordingScreen” action.
No issues found. Hides the input expression editor and shows a note about maximum duration.
308-314: Handling device logs and stopping screen recording.
Hiding file path fields and letting the user specify a folder location is clear.
316-320: PushFileToDevice action.
Labels are updated for local file vs. device folder. Implementation is consistent with other file transfer actions.
322-324: PullFileFromDevice action.
Straightforward approach. Only the folder panel is shown.
326-329: SetClipboardText action.
Assigning the “Text:” label is aligned with the rest of the UI.
331-334: Show performance data panels.
This block sets xAppPnl and xSpecificPerformanceDataPnl visibility. Looks good.
336-339: RotateSimulation action panel.
Device rotation panel is now visible for the “RotateSimulation” action. Implementation is consistent with the existing pattern.Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (23)
56-56: Check usage of newly added import.
The import "OpenQA.Selenium.Appium.Enums" was added in a past revision. If enums from this namespace are no longer used, consider removing it.
114-114: Type-safety concern for ScreenshotHeight property.
Storing the screenshot height as a string can lead to parsing issues. Converting it to an integer (or a nullable int) would be safer.
170-171: No notable change.
This appears to be a minor or whitespace addition for webDriver.
330-332: StopProcess assignment.
This logic unifies the Selenium driver’s StopProcess with the base property. No functional issue found.
994-994: No functional change.
Likely whitespace or formatting.
1227-1227: Adding context index for clarity.
"Context " + i + 1 clarifies the returned results. Implementation is correct.
1240-1242: IsKeyboardVisible.
Action is straightforward. Ensure user flows account for iOS vs. Android coverage.
1243-1245: IsLocked.
Retrieves locked state. No issues found.
1251-1253: RemoveApp.
Consider catching driver exceptions to report status more gracefully.
1255-1257: QueryAppState.
Provides a quick check on the app’s run state. Looks good.
1259-1274: RotateSimulation.
Consider adding a default case if “state” is neither “Landscape” nor “Portrait.”
1276-1278: RunScriptOnDevice.
Running raw scripts can be a security risk if inputs aren’t trusted.
1280-1289: StartDeviceScreenRecording.
Implementation covers both Android and iOS with a maximum 30 minutes limit.
1290-1293: HideKeyboard.
Potential exceptions can occur if the keyboard is already hidden. Consider safe handling in higher-level logic.
1294-1297: PushFileToDevice reversed parameters.
LocalFilePath and DeviceTargerFolder usage seem swapped. This was flagged previously and remains valid.
1298-1301: PullFileFromDevice reversed path usage.
Similar confusion about local vs. remote parameters.
1302-1304: SetClipboardText.
Applies to both iOS and Android. Implementation looks fine.
1306-1309: GetClipboardText.
No issues found, the method checks for iOS or Android at runtime.
1310-1314: GetDeviceLogs.
If logs become large, consider chunking or limiting.
1316-1346: Retrieving performance data.
A strongly typed structure or column labels may improve clarity when returning CPU or memory metrics.
1483-1483: Method name – “GetUserCustomeScreenshotSize.”
Minor spelling issue noted in previous reviews (“Custome” → “Custom”).
3099-3119: PerformDrag method.
Implementing drag with press and drop durations is consistent with related device actions.
3399-3408: Refresh screen scaling in CalculateMobileDeviceScreenSizes.
Dynamically recalculating the window scale factor is good for orientation changes.Ginger/Ginger/Drivers/DriversWindows/MobileDriverWindow.xaml.cs (3)
67-67: LGTM! Clean collection initialization.The code uses the new C# 12 collection expression syntax for a cleaner initialization of
mDeviceDetails.
206-212: LGTM! Well-structured UI update method.The
UpdateRotateIcon()method correctly uses the Dispatcher to ensure UI updates happen on the UI thread, following WPF best practices.
278-280: LGTM! Clean event handling.The
RotateEventcase is properly integrated into the existing message handling switch statement and maintains consistency with the codebase.
| case ActMobileDevice.eMobileDeviceAction.OpenDeeplink: | ||
| OpenDeeplink(act.ActionInput.ValueForDriver, act.ActionAppPackage.ValueForDriver, GetAppPackageNameByOs()); | ||
| break; | ||
|
|
||
| case ActMobileDevice.eMobileDeviceAction.IsKeyboardVisible: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New “OpenDeeplink” action.
Ensure that any exceptions at runtime (e.g., invalid link) are handled if needed.
| case ActMobileDevice.eMobileDeviceAction.IsAppInstalled: | ||
| act.AddOrUpdateReturnParamActual("Is App Installed", IsDeviceAppInstalled(act.ActionAppPackage.ValueForDriver).ToString()); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
IsAppInstalled.
Checks device-level installation. Consider a fallback if the package name is null or empty.
MeniKadosh1
changed the base branch from
master
to
Releases/Official-Release
|
|
||
| case ActMobileDevice.eMobileDeviceAction.GetDeviceLogs: | ||
| string deviceLogsPath = GetDeviceLogs(act.LocalFile.ValueForDriver); | ||
| act.AddOrUpdateReturnParamActual("GetDeviceLogPath", deviceLogsPath); |
There was a problem hiding this comment.
change to "DeviceLogFilePath"
| case ActMobileDevice.eMobileDeviceAction.GetDeviceLogs: | ||
| string deviceLogsPath = GetDeviceLogs(act.LocalFile.ValueForDriver); | ||
| act.AddOrUpdateReturnParamActual("GetDeviceLogPath", deviceLogsPath); | ||
| Act.AddArtifactToAction(Path.GetFileName(GetDeviceLogs(act.LocalFile.ValueForDriver).ToString()), act, deviceLogsPath); |
There was a problem hiding this comment.
Need to use deviceLogsPath instead of calling GetDeviceLogs again
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit