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

RUM-4883 Tabbar Icon Default Tint Color #1906

Merged
merged 10 commits into from
Jun 18, 2024

Conversation

mariedm
Copy link
Member

@mariedm mariedm commented Jun 14, 2024

What and why?

This PR introduces changes to the Session Replay feature to handle the default color of tab bar icons when no unselectedItemTintColor is set. This enhances the visual consistency and clarity in the session replays.

How?

This PR updates the UITabBarRecorder to handle the default color for unselected tab bar items. It introduces a logic that sets the tab bar's icon color to gray when the unselectedItemTintColor is not provided. The UITabBarRecorder now includes its own UIImageViewRecorder to handle icons properly.

Here's a SR example with unselectedTintColor not set on tab bar: Session Replay Example 1.

Notes on Image Comparison

This PR uses image comparison to identify the selected item in the tab bar. I considered different options for this:

  • Image Data Comparison: accurate but too costly in terms of performance, especially if executed on the main thread
  • Using srIdentifier: provides a unique string, and while cached, still not ideal for performance
  • Using Image Description, idea suggested by @maciejburda: provides a sort of unique identifier with minimal overhead (though descriptions could be similar for images with similar properties (?))
  • Using cgImage properties: generates a unique identifier, still theoretically possible for collisions, but highly unlikely

To make a decision between the last 2 options, I did a small benchmark on real devices. Results show that cgImage is much more efficient (24x faster on iPhone 13 mini, 12x faster on iPhone SE 2nd gen.).

Screenshots

Updated screenshots from snapshot tests.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@mariedm mariedm force-pushed the RUM-4883-tabbar-icon-default-tint-color branch from 528961e to 3f7c67e Compare June 14, 2024 16:44
@mariedm mariedm marked this pull request as ready for review June 14, 2024 16:45
@mariedm mariedm requested review from a team as code owners June 14, 2024 16:45
@mariedm mariedm marked this pull request as draft June 17, 2024 08:17
@mariedm mariedm marked this pull request as ready for review June 17, 2024 08:35
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jun 17, 2024

Datadog Report

Branch report: RUM-4883-tabbar-icon-default-tint-color
Commit report: c955313
Test service: dd-sdk-ios

✅ 0 Failed, 254 Passed, 0 Skipped, 24.75s Total Time

ncreated
ncreated previously approved these changes Jun 17, 2024
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

I considered different options for this:
...
To make a decision between the last 2 options, I did a small benchmark on real devices. Results show that cgImage is much more efficient (24x faster on iPhone 13 mini, 12x faster on iPhone SE 2nd gen.).

Excellent approach 🏅! We need more benchmark driven decisions in SR, well done 💪

ncreated
ncreated previously approved these changes Jun 18, 2024
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Very nice work, I really appreciate the performance driven solution and the level of comments 👏
I have left a minor change request on releasing the UITabBar in the recorder.

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Very nice work, I really appreciate the performance driven solution and the level of comments 👏
I have left a minor change request on releasing the UITabBar in the recorder.

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Thanks, well done!

@mariedm mariedm merged commit d184aeb into develop Jun 18, 2024
4 checks passed
@mariedm mariedm deleted the RUM-4883-tabbar-icon-default-tint-color branch June 18, 2024 14:55
@maxep maxep mentioned this pull request Jul 4, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants