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

Add Catalyst support to Analytics #2057

Merged
merged 33 commits into from
May 20, 2020

Conversation

AnastasiaKubova
Copy link

@AnastasiaKubova AnastasiaKubova commented May 15, 2020

Things to consider before you submit the PR:

  • Has CHANGELOG.md been updated?
  • Are tests passing locally?
  • Are the files formatted correctly?
  • Did you add unit tests?
  • Did you check UI tests on the sample app? They are not executed on CI.
  • Did you test your change with either the sample apps that are included in the repository or with a blank app that uses your change?

Description

In Catalyst project for macOS and for native macOS we have different sending data in analytics reports.

  1. Need to fix the getting data in analytics module:
  • Different screen size
    Was: “screenSize” : “1800x2880",
    Now: “screenSize” : “1200x1920"

  • Different model name (now just numbers)
    Was: “MacBookPro15,2”,
    Now: x86_64

  • Different OS name:
    Was: “osName” : “macOS”,
    Now: “osName” : “Mac OS X”

  1. Add check for TARGET_OS_MACCATALYST where need and make sure that all test passed.

Related PRs or issues

AB#79044

@AnastasiaKubova AnastasiaKubova force-pushed the feature/analytics-catalyst branch from f707d8d to 56230d5 Compare May 18, 2020 06:03
@codecov-io
Copy link

codecov-io commented May 18, 2020

Codecov Report

Merging #2057 into feature/support-catalyst will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feature/support-catalyst    #2057   +/-   ##
=========================================================
  Coverage                     91.63%   91.63%           
=========================================================
  Files                           129      129           
  Lines                         10072    10072           
=========================================================
  Hits                           9229     9229           
  Misses                          843      843           
Flag Coverage Δ
#iOS 91.64% <100.00%> (-0.27%) ⬇️
#macOS 91.62% <100.00%> (ø)
#tvOS 91.04% <100.00%> (-0.33%) ⬇️
Impacted Files Coverage Δ
...pCenter/Internals/Context/Device/MSDeviceTracker.m 92.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1364f5...396fa61. Read the comment docs.

@AnastasiaKubova AnastasiaKubova force-pushed the feature/analytics-catalyst branch from 854b67d to 2fbbc01 Compare May 18, 2020 08:36
@AnastasiaKubova AnastasiaKubova force-pushed the feature/analytics-catalyst branch from 2fbbc01 to 396fa61 Compare May 18, 2020 09:40
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #2057 into feature/support-catalyst will decrease coverage by 0.07%.
The diff coverage is 82.97%.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           feature/support-catalyst    #2057      +/-   ##
============================================================
- Coverage                     91.38%   91.30%   -0.08%     
============================================================
  Files                           113      113              
  Lines                          8413     8431      +18     
============================================================
+ Hits                           7688     7698      +10     
- Misses                          725      733       +8     
Flag Coverage Δ
#macOS 91.54% <81.39%> (-0.08%) ⬇️
#tvOS 90.96% <80.95%> (-0.09%) ⬇️
Impacted Files Coverage Δ
AppCenter/AppCenter/Internals/Util/MSUtility.m 56.00% <55.55%> (-1.15%) ⬇️
...pCenter/Internals/Context/Device/MSDeviceTracker.m 92.72% <100.00%> (+0.03%) ⬆️
...enter/Internals/Context/Session/MSSessionContext.m 100.00% <100.00%> (ø)
...pCenter/Internals/Context/UserId/MSUserIdContext.m 100.00% <100.00%> (ø)
...ternals/DelegateForwarder/MSAppDelegateForwarder.m 59.01% <100.00%> (ø)
...enter/AppCenter/Internals/Storage/MSLogDBStorage.m 94.69% <100.00%> (-0.06%) ⬇️
...ppCenter/AppCenter/Internals/Util/MSUtility+File.m 90.75% <100.00%> (ø)
AppCenter/AppCenter/Internals/Util/MSUtility.h 100.00% <100.00%> (ø)
...er/Internals/Vendor/Reachability/MS_Reachability.m 61.06% <100.00%> (ø)
AppCenter/AppCenter/MSAppCenter.m 94.58% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4955cce...4955cce. Read the comment docs.

@AnastasiaKubova AnastasiaKubova changed the base branch from feature/support-catalyst to feature/catalyst-tests May 19, 2020 09:13
#elif TARGET_OS_MACCATALYST
// This returns scale value based on scale values iOS.
// So we need to use base value for macOS.
CGFloat scale = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move magic numbers to consts

@AnastasiaKubova AnastasiaKubova changed the base branch from feature/catalyst-tests to feature/core-catalyst May 19, 2020 14:17
@AnastasiaKubova AnastasiaKubova marked this pull request as ready for review May 19, 2020 14:34
Base automatically changed from feature/core-catalyst to feature/support-catalyst May 20, 2020 10:49
@annakocheshkova annakocheshkova merged commit b9843fe into feature/support-catalyst May 20, 2020
@annakocheshkova annakocheshkova deleted the feature/analytics-catalyst branch May 20, 2020 10:55
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.

5 participants