Scrollable screen-shot support added for mobile device#4232
Conversation
WalkthroughThis update enhances screenshot functionality for mobile automation by introducing full-page screenshot capabilities in the Changes
Sequence Diagram(s)sequenceDiagram
participant VRTAnalyzer
participant Driver (GenericAppiumDriver or Other)
participant AppiumDriver
VRTAnalyzer->>Driver: TrackVRT()
alt Driver is GenericAppiumDriver and full-page needed
Driver->>Driver: CaptureFullPageCroppedScreenshot()
loop While page is scrollable and no duplicate
Driver->>AppiumDriver: ScrollDown()
Driver->>Driver: Capture and crop screenshot
end
Driver->>Driver: MergeScreenshotsVertically()
Driver->>VRTAnalyzer: Return full-page screenshot
else
Driver->>Driver: GetScreenShot()
Driver->>VRTAnalyzer: Return screenshot
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(38 hunks)
🧰 Additional context used
🧠 Learnings (1)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (4)
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:33.780Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
🧬 Code Graph Analysis (1)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
GenericAppiumDriver(82-5323)GenericAppiumDriver(258-261)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
1708-1720: Guard clause must come before usingimages[0]
width = images[0].Widthis executed before verifying thatimagesis non-null and non-empty, leading to anIndexOutOfRangeExceptionwhenCaptureFullPageCroppedScreenshotreturns zero frames.- int totalHeight = images.Sum(img => img.Height); - int width = images[0].Width; - if (images == null || images.Count == 0) + if (images == null || images.Count == 0) throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images)); + int totalHeight = images.Sum(img => img.Height); + int width = images[0].Width;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(38 hunks)
🧰 Additional context used
🧠 Learnings (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (6)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:219-226
Timestamp: 2025-06-16T10:13:01.319Z
Learning: In VRTAnalyzer.cs for the GenericAppiumDriver class, the mAct.IsFullPageScreenshot flag is not applicable and should not be used to control screenshot behavior. GenericAppiumDriver always uses CaptureFullPageCroppedScreenshot() method regardless of the IsFullPageScreenshot flag setting.
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T09:52:40.854Z
Learning: The `GenericAppiumDriver` class is located in the `Amdocs.Ginger.CoreNET` namespace, not in a namespace that matches its file path structure. File paths don't always correspond to namespace declarations in this codebase.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:33.780Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
undefined
<retrieved_learning>
Learnt from: prashelke
PR: #4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:219-226
Timestamp: 2025-06-16T10:13:01.319Z
Learning: In VRTAnalyzer.cs for the GenericAppiumDriver class, the mAct.IsFullPageScreenshot flag is not applicable and should not be used to control screenshot behavior. GenericAppiumDriver always uses CaptureFullPageCroppedScreenshot() method regardless of the IsFullPageScreenshot flag setting.
</retrieved_learning>
🧬 Code Graph Analysis (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
GenericAppiumDriver(82-5278)GenericAppiumDriver(258-261)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (3)
Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (1)
ActMobileDevice(30-668)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (8)
Bitmap(2003-2035)Bitmap(9544-9625)Bitmap(9627-9660)Bitmap(9701-9705)Bitmap(9848-9876)Bitmap(9877-9881)Bitmap(9883-9964)Size(10876-10879)Ginger/GingerCoreCommon/Actions/Act.cs (1)
AddOrUpdateReturnParamActual(1305-1332)
🔇 Additional comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
1731-1740:HideStickyElementssilently no-ops on native views
IJavaScriptExecutoronly works when the driver context is web; on native pages the cast throws and the catch swallows the error.
Verifydriver.Context.StartsWith("WEBVIEW")(Android) /driver.Context == "WEBVIEW"(iOS) before executing the script, or skip this step for non-web contexts to avoid pointless try/catch overhead.Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (1)
219-230: Nice cleanup – duplicated screenshot logic centralisedSwitching to
GetScreenshot()eliminates the two previously diverging code blocks and keeps baseline/non-baseline paths in sync. 👍
Please just double-check that every call site copes with a possiblenullreturn (see follow-up comment).
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
4324-4330: Parameter shadowing & path logic inSetFilePathLocal variable
fileNameshadows method parameterFileName, making the ternary
expression hard to follow and producing unexpected paths when a directory is
passed. Consider simplifying:- string fileName = Path.GetFileName(FilePath); - string targetFile = string.IsNullOrEmpty(fileName) ? Path.Combine(FilePath, $"{FileName}_{timestamp}.{FileType}") : $"{FilePath}.{FileType}"; + string baseName = Path.GetFileName(FilePath); + string targetFile = string.IsNullOrEmpty(baseName) + ? Path.Combine(FilePath, $"{FileName}_{timestamp}.{FileType}") + : Path.Combine(Path.GetDirectoryName(FilePath)!, $"{baseName}_{timestamp}.{FileType}");
♻️ Duplicate comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
1683-1689: Heavy Base-64 hashing was already flagged earlierRe-encoding every crop to PNG + Base64 is CPU/memory expensive on long pages.
Previous review suggested a raw-pixel MD5/SHA-1 or lightweight checksum.
No functional change was made; consider addressing before merge to avoid slow tests.
1709-1715: Null-guard placed after first use – can crash on empty list
width = images[0].Widthexecutes before the empty-collection check,
soimages.Count == 0still throwsIndexOutOfRangeException.- int totalHeight = images.Sum(img => img.Height); - int width = images[0].Width; - if (images == null || images.Count == 0) + if (images == null || images.Count == 0) + throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images)); + + int totalHeight = images.Sum(img => img.Height); + int width = images[0].Width;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(38 hunks)
🧰 Additional context used
🧠 Learnings (2)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (7)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:219-226
Timestamp: 2025-06-16T10:13:01.319Z
Learning: In VRTAnalyzer.cs for the GenericAppiumDriver class, the mAct.IsFullPageScreenshot flag is not applicable and should not be used to control screenshot behavior. GenericAppiumDriver always uses CaptureFullPageCroppedScreenshot() method regardless of the IsFullPageScreenshot flag setting.
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T09:52:40.854Z
Learning: The `GenericAppiumDriver` class is located in the `Amdocs.Ginger.CoreNET` namespace, not in a namespace that matches its file path structure. File paths don't always correspond to namespace declarations in this codebase.
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.024Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:33.780Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
undefined
<retrieved_learning>
Learnt from: prashelke
PR: #4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:219-226
Timestamp: 2025-06-16T10:13:01.319Z
Learning: In VRTAnalyzer.cs for the GenericAppiumDriver class, the mAct.IsFullPageScreenshot flag is not applicable and should not be used to control screenshot behavior. GenericAppiumDriver always uses CaptureFullPageCroppedScreenshot() method regardless of the IsFullPageScreenshot flag setting.
</retrieved_learning>
🧬 Code Graph Analysis (1)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (2)
GenericAppiumDriver(83-5354)GenericAppiumDriver(259-262)
🔇 Additional comments (1)
Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs (1)
219-230: Helper reduces duplication – good moveReplacing the two location-specific screenshot blocks with a single
GetScreenshot()call tightens the logic and prevents drift between baseline and non-baseline paths.
No further issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (1)
1664-1717: Make full-page capture more robust & configurable
maxScrollsis hard-coded to 10. Long pages on modern devices frequently exceed this. Expose it as an optional parameter or compute it from page height.- Comment says “~5% for gesture bar” but you subtract 7 %. Align the comment or the calculation to avoid confusion.
- Duplicate detection still base-64 encodes every frame (heavy). A lightweight hash (MD5/SHA-1 over raw bytes) would cut CPU & memory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(38 hunks)
🧰 Additional context used
🧠 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (3)
undefined
<retrieved_learning>
Learnt from: prashelke
PR: #4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:219-226
Timestamp: 2025-06-16T10:13:01.319Z
Learning: In VRTAnalyzer.cs for the GenericAppiumDriver class, the mAct.IsFullPageScreenshot flag is not applicable and should not be used to control screenshot behavior. GenericAppiumDriver always uses CaptureFullPageCroppedScreenshot() method regardless of the IsFullPageScreenshot flag setting.
</retrieved_learning>
<retrieved_learning>
Learnt from: prashelke
PR: #4232
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:1637-1655
Timestamp: 2025-06-16T12:22:07.984Z
Learning: The driver.ReleaseActions() method is deprecated in current versions of Appium and should not be used for managing action sequences.
</retrieved_learning>
<retrieved_learning>
Learnt from: prashelke
PR: #4232
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:1637-1655
Timestamp: 2025-06-16T12:22:07.984Z
Learning: In Appium 2.x, driver.ReleaseActions() is deprecated. The current best practice is to use the W3C Actions API with fresh action sequences for each gesture, treating them as single-use objects. Action sequences should not be reused across multiple operations.
</retrieved_learning>
🧬 Code Graph Analysis (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (3)
Ginger/GingerCoreNET/ActionsLib/UI/Mobile/ActMobileDevice.cs (1)
ActMobileDevice(30-692)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (19)
Bitmap(2003-2035)Bitmap(9544-9625)Bitmap(9627-9660)Bitmap(9701-9705)Bitmap(9848-9876)Bitmap(9877-9881)Bitmap(9883-9964)List(2296-2347)List(4893-4968)List(5207-5241)List(6033-6054)List(6124-6176)List(8284-8296)List(10157-10160)List(10204-10223)List(10297-10304)List(10306-10367)List(10712-10715)List(10737-10740)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
| private static void ScrollDown(AppiumDriver driver) | ||
| { | ||
| try | ||
| { | ||
| var windowSize = driver.Manage().Window.Size; | ||
|
|
||
| int startX = windowSize.Width / 2; | ||
| int startY = (int)(windowSize.Height * 0.8); | ||
| int endY = (int)(windowSize.Height * 0.2); | ||
|
|
||
| var touch = new PointerInputDevice(PointerKind.Touch); | ||
| var sequence = new ActionSequence(touch, 0); | ||
|
|
||
| sequence.AddAction(touch.CreatePointerMove(CoordinateOrigin.Viewport, startX, startY, TimeSpan.Zero)); | ||
| sequence.AddAction(touch.CreatePointerDown(0)); | ||
| sequence.AddAction(touch.CreatePause(TimeSpan.FromMilliseconds(300))); | ||
| sequence.AddAction(touch.CreatePointerMove(CoordinateOrigin.Viewport, startX, endY, TimeSpan.FromMilliseconds(500))); | ||
| sequence.AddAction(touch.CreatePointerUp(0)); | ||
|
|
||
| driver.PerformActions(new List<ActionSequence> { sequence }); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, "Error occurred while scrolling for screen-shot", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use the same touch-action API everywhere
ScrollDown builds the gesture with OpenQA.Selenium.Interactions.PointerInputDevice, while the rest of the driver (e.g. TapXY, SwipeByXY) uses OpenQA.Selenium.Appium.Interactions.PointerInputDevice.
Mixing the two classes works today, but generates two different JSON payload formats and may break when the Appium client is upgraded. Stick to Appium.Interactions.PointerInputDevice for consistency across all gestures.
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
between lines 1637 and 1662, the ScrollDown method uses
OpenQA.Selenium.Interactions.PointerInputDevice instead of
OpenQA.Selenium.Appium.Interactions.PointerInputDevice like the rest of the
driver. To fix this, replace the usage of
OpenQA.Selenium.Interactions.PointerInputDevice with
OpenQA.Selenium.Appium.Interactions.PointerInputDevice in the ScrollDown method
to ensure consistent gesture building and avoid potential issues with different
JSON payload formats.
| private static Bitmap MergeScreenshotsVertically(List<Bitmap> images) | ||
| { | ||
| int totalHeight = images.Sum(img => img.Height); | ||
| int width = images[0].Width; | ||
|
|
||
| Bitmap finalImage = new Bitmap(width, totalHeight); | ||
| using (Graphics g = Graphics.FromImage(finalImage)) | ||
| { | ||
| try | ||
| { | ||
| int offset = 0; | ||
| if (images == null || images.Count == 0) | ||
| { | ||
| throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images)); | ||
| } | ||
| foreach (var image in images) | ||
| { | ||
| g.DrawImage(image, new Rectangle(0, offset, image.Width, image.Height)); | ||
| offset += image.Height; | ||
| image.Dispose(); // free as soon as we are done | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, "Error occurred while merging screen-shots", ex); | ||
| } | ||
| } | ||
| return finalImage; | ||
| } |
There was a problem hiding this comment.
Null / empty list guard is after the first access
MergeScreenshotsVertically dereferences images[0] before checking if the list is empty, so an empty collection raises ArgumentOutOfRangeException instead of your friendly ArgumentException.
- int totalHeight = images.Sum(img => img.Height);
- int width = images[0].Width;
- if (images == null || images.Count == 0)
- throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images));
+ if (images == null || images.Count == 0)
+ throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images));
+
+ int totalHeight = images.Sum(img => img.Height);
+ int width = images[0].Width;📝 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 static Bitmap MergeScreenshotsVertically(List<Bitmap> images) | |
| { | |
| int totalHeight = images.Sum(img => img.Height); | |
| int width = images[0].Width; | |
| Bitmap finalImage = new Bitmap(width, totalHeight); | |
| using (Graphics g = Graphics.FromImage(finalImage)) | |
| { | |
| try | |
| { | |
| int offset = 0; | |
| if (images == null || images.Count == 0) | |
| { | |
| throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images)); | |
| } | |
| foreach (var image in images) | |
| { | |
| g.DrawImage(image, new Rectangle(0, offset, image.Width, image.Height)); | |
| offset += image.Height; | |
| image.Dispose(); // free as soon as we are done | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, "Error occurred while merging screen-shots", ex); | |
| } | |
| } | |
| return finalImage; | |
| } | |
| private static Bitmap MergeScreenshotsVertically(List<Bitmap> images) | |
| { | |
| // guard against null or empty input before any dereferences | |
| if (images == null || images.Count == 0) | |
| throw new ArgumentException("images collection must contain at least one Bitmap", nameof(images)); | |
| int totalHeight = images.Sum(img => img.Height); | |
| int width = images[0].Width; | |
| Bitmap finalImage = new Bitmap(width, totalHeight); | |
| using (Graphics g = Graphics.FromImage(finalImage)) | |
| { | |
| try | |
| { | |
| int offset = 0; | |
| foreach (var image in images) | |
| { | |
| g.DrawImage(image, new Rectangle(0, offset, image.Width, image.Height)); | |
| offset += image.Height; | |
| image.Dispose(); // free as soon as we are done | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, "Error occurred while merging screen-shots", ex); | |
| } | |
| } | |
| return finalImage; | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
around lines 1719 to 1747, the method MergeScreenshotsVertically accesses
images[0] before checking if the images list is null or empty, which can cause
an ArgumentOutOfRangeException. To fix this, move the null or empty check for
the images list to the very beginning of the method before any access to images
elements, and throw the ArgumentException if the list is null or empty. This
ensures the guard is effective and prevents invalid access.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements