Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Refactor snapshotting mechanism #970

Merged
merged 11 commits into from
Jan 16, 2025
Merged

feat: Refactor snapshotting mechanism #970

merged 11 commits into from
Jan 16, 2025

Conversation

mykola-mokhnach
Copy link

@mykola-mokhnach mykola-mokhnach commented Jan 14, 2025

BREAKING CHANGE: snapshotTimeout and customSnapshotTimeout settings have been removed as a result of the custom snapshotting logic removal

Snapshotting is complicated. And requires lots of time.
This PR tries to optimize snapshots by performing the following optimizations:

  • Reduce the amount of snapshotting calls. We do not invoke element snapshots every time it is fetched from the cache, but instead only when it's necessary to either fetch element properties or verify if it is stalled.
  • Only fetch required element attributes where needed in snapshots without fetching the whole hierarchy or custom attributes. Basically we use the below 2 main snapshotting methods interchangeably:
    • fb_takeSnapshot:NO -> the fastest one, - fetches only standard attributes without tree structure
    • fb_takeSnapshot:YES -> the slower one, - fetches standard attributes plus the tree structure
  • Custom visibility and accessibility attributes are now fetched dynamically using a customized timeout value.

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

lgtm in my best knowledge...
I like this simplification. Hopefully, the fb_isVisible's removals will work well

@mykola-mokhnach mykola-mokhnach changed the title feat: Refactor snapshotting mechnism feat: Refactor snapshotting mechanism Jan 15, 2025
@KazuCocoa
Copy link
Member

Just to leave a finding:

I saw a missing value attribute case in the XML source for Settings app on a simulator. This was one line only, while other elements properly had value attributes. Should not be an issue with this PR change.

Current master

<XCUIElementTypeSearchField type="XCUIElementTypeSearchField" value="Search" name="Search" label="Search" enabled="true" visible="true" accessible="true" x="20" y="150" width="390" height="37" index="2">

With this PR

<XCUIElementTypeSearchField type="XCUIElementTypeSearchField" name="Search" label="Search" enabled="true" visible="true" accessible="true" x="20" y="150" width="390" height="37" index="2">

@mykola-mokhnach
Copy link
Author

mykola-mokhnach commented Jan 16, 2025

Just to leave a finding:

I saw a missing value attribute case in the XML source for Settings app on a simulator. This was one line only, while other elements properly had value attributes. Should not be an issue with this PR change.

Current master

<XCUIElementTypeSearchField type="XCUIElementTypeSearchField" value="Search" name="Search" label="Search" enabled="true" visible="true" accessible="true" x="20" y="150" width="390" height="37" index="2">

With this PR

<XCUIElementTypeSearchField type="XCUIElementTypeSearchField" name="Search" label="Search" enabled="true" visible="true" accessible="true" x="20" y="150" width="390" height="37" index="2">

I did not change any logic for the value fetching. Nevertheless added this field type to the list of placeholder owners to align these behaviours

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

yea, i know of. That was interesting.

lgtm

@@ -42,11 +42,11 @@ - (BOOL)fb_isAccessibilityElement
if (nil != attributeValue) {
NSMutableDictionary *updatedValue = [NSMutableDictionary dictionaryWithDictionary:self.additionalAttributes ?: @{}];
[updatedValue setObject:attributeValue forKey:FB_XCAXAIsElementAttribute];
self.additionalAttributes = updatedValue.copy;
self.snapshot.additionalAttributes = updatedValue.copy;
Copy link
Member

@KazuCocoa KazuCocoa Jan 16, 2025

Choose a reason for hiding this comment

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

then, others also need to be self.snapshot.additionalAttributes in here and visible?

Copy link
Author

Choose a reason for hiding this comment

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

not really due to

- (id)forwardingTargetForSelector:(SEL)aSelector
magic

@mykola-mokhnach mykola-mokhnach merged commit 08f1306 into master Jan 16, 2025
42 of 46 checks passed
@mykola-mokhnach mykola-mokhnach deleted the native_snap branch January 16, 2025 16:44
github-actions bot pushed a commit that referenced this pull request Jan 16, 2025
## [9.0.0](v8.12.2...v9.0.0) (2025-01-16)

### ⚠ BREAKING CHANGES

* snapshotTimeout and customSnapshotTimeout settings have been removed as a result of the custom snapshotting logic removal

### Features

* Refactor snapshotting mechanism ([#970](#970)) ([08f1306](08f1306))
Copy link

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants