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

[frameit] fix device detection that would match less specific devices first #20642

Merged
merged 3 commits into from Oct 25, 2022

Conversation

guidev
Copy link
Contributor

@guidev guidev commented Sep 10, 2022

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Resolves #20514

Trying to frame a screenshot named Simulator Screen Shot - iPhone 13 Pro doesn't work correctly. It0s recognised as an iPhone 13.

Description

One of the checks used to detect the correct device is to see if the filename (Simulator Screen Shot - iPhone 13 Pro) contains the device's formatted name (iPhone 13, iPhone 13 Pro, etc).

Unfortunately the first device to pass all the checks is iPhone 13 (the device list doesn't seem to be sorted?).

Current device list:
  • IPHONE_12_PRO_MAX
  • IPHONE_12_MINI
  • IPHONE_13
  • IPHONE_13_PRO
  • IPHONE_13_PRO_MAX
  • IPHONE_13_MINI
  • IPAD_10_2
  • IPAD_AIR_2
  • IPAD_AIR_2019
  • IPAD_MINI_4
  • IPAD_MINI_2019
  • IPAD_PRO
  • IPAD_PRO_12_9
  • IPAD_PRO_12_9_4
  • IPAD_PRO_10_5
  • IPAD_PRO_11
  • GOOGLE_PIXEL_3
  • GOOGLE_PIXEL_3_XL
  • GOOGLE_PIXEL_4
  • GOOGLE_PIXEL_4_XL
  • GOOGLE_PIXEL_5
  • HTC_ONE_A9
  • HTC_ONE_M8
  • HUAWEI_P8
  • MOTOROLA_MOTO_E
  • MOTOROLA_MOTO_G
  • NEXUS_4
  • NEXUS_5X
  • NEXUS_6P
  • NEXUS_9
  • SAMSUNG_GALAXY_GRAND_PRIME
  • SAMSUNG_GALAXY_NOTE_5
  • SAMSUNG_GALAXY_NOTE_10
  • SAMSUNG_GALAXY_NOTE_10_PLUS
  • SAMSUNG_GALAXY_S_DUOS
  • SAMSUNG_GALAXY_S3
  • SAMSUNG_GALAXY_S5
  • SAMSUNG_GALAXY_S7
  • SAMSUNG_GALAXY_S8
  • SAMSUNG_GALAXY_S9
  • SAMSUNG_GALAXY_S10
  • SAMSUNG_GALAXY_S10_PLUS
  • XIAOMI_MI_MIX_ALPHA
  • IPHONE_5S
  • IPHONE_5C
  • IPHONE_SE
  • IPHONE_6S
  • IPHONE_6S_PLUS
  • IPHONE_7
  • IPHONE_7_PLUS
  • IPHONE_8
  • IPHONE_8_PLUS
  • IPHONE_X
  • MAC
  • IPHONE_XS
  • IPHONE_XR
  • IPHONE_XS_MAX
  • IPHONE_11
  • IPHONE_11_PRO
  • IPHONE_11_PRO_MAX
  • IPHONE_12
  • IPHONE_12_PRO

I believe the best way to fix this is to check first for the longer device models (check first it it matches iPhone 13 Pro Max, iPhone 13 Pro, iPhone 13, etc..).

Testing Steps

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

💯 Thanks for this!

@@ -51,7 +51,7 @@ def self.detect_device(path, platform)
found_device = nil
filename_device = nil
filename = Pathname.new(path).basename.to_s
Devices.constants.each do |c|
Devices.constants.sort_by(&:length).reverse_each do |c|
Copy link
Member

Choose a reason for hiding this comment

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

This is a crazy assumption here. But quite safe, IMO. It doesn't rely on business logic or marketing naming standards, but rather "the longer the string, the more specific it will be" and thus matching with partial substrings won't be possible. Quite clever IMO :)

@rogerluan rogerluan changed the title [frameit] Fixed device detection [frameit] fix device detection that would match less specific devices first Sep 26, 2022
@guidev
Copy link
Contributor Author

guidev commented Oct 22, 2022

any update?

@rogerluan rogerluan merged commit 7de28ca into fastlane:master Oct 25, 2022
@guidev guidev deleted the frameit_fix_device_selection branch October 25, 2022 11:45
@fastlane-bot
Copy link

Hey @guidev 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.211.0 🚀

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

Successfully merging this pull request may close these issues.

[frameit] "fastlane frameit" doesn't detect iPhone 13 Pro
3 participants