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(device_info_plus): Return model name for iOS and MacOS devices #3358

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

vbuberen
Copy link
Collaborator

@vbuberen vbuberen commented Nov 14, 2024

Description

Long awaited feature to show device names that are known to users. Marking as breaking, because I changed the API.

On iOS there is a new modelName field that returns names like iPhone 15 Pro, etc. instead of just iPhone, iPad.

Screenshot from iPhone with model name

photo_2024-12-10 11 58 52

On MacOS there is also the same modelName property that returns names like MacBook Pro 16-inch (2021)

Screenshot from MacBook with model name and identifier Screenshot 2024-12-10 at 11 53 11

Related Issues

Closes #2656

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@vbuberen
Copy link
Collaborator Author

Don't know why iOS integration test fails.

Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

LGTM!

@miquelbeltran
Copy link
Member

miquelbeltran commented Nov 15, 2024

Don't know why iOS integration test fails.

Could be that increasing the minimum deployment target makes these properties mandatory:

Warning: Missing build name (CFBundleShortVersionString).
Warning: Missing build number (CFBundleVersion).
ERROR: Action Required: You must set a build name and number in the pubspec.yaml file version field before submitting to the App Store.

You can try to add a version here: https://github.com/fluttercommunity/plus_plugins/blob/main/packages/device_info_plus/device_info_plus/example/pubspec.yaml

@vbuberen
Copy link
Collaborator Author

Could be that increasing the minimum deployment target makes these properties mandatory:

I saw these warnings earlier, but still tried to add version - still failing. Will investigate later.

@StanleyCocos
Copy link
Contributor

This feature is needed by many people, but the downside is that maintenance can be troublesome. After Apple releases new devices, we need to update the code promptly.

Therefore, I suggest changing the "Unknown Model" returned in modelName to modelIdentifier. This would avoid situations where modelName == Unknown Model, and we might need to use modelIdentifier in conjunction with it to determine the device information accurately.

@vbuberen
Copy link
Collaborator Author

vbuberen commented Dec 3, 2024

This feature is needed by many people, but the downside is that maintenance can be troublesome. After Apple releases new devices, we need to update the code promptly.

This is not an issue with current pace of product releases from Apple. It is not a big deal to report the missing model. Returning modelIdentifier won't solve anything and it is better to show that the model is unknown explicitly than to create a confusion with returning modelIdentifier 2 times. So don't agree with this suggestion.

@StanleyCocos
Copy link
Contributor

This is how I see it: modelName and modelIdentifier are essentially the same thing. For our actual application, we typically use APIs to upload the user's device information to the backend for future data analysis and similar purposes. If the return value is "Unknown Model", it means we would need to handle it like this

var modelName = iosInfo.modelName == 'Unknown Model' ? iosInfo.modelIdentifier : iosInfo.modelName;

I think this is unnecessary. modelName is essentially a translation of modelIdentifier. If it cannot be translated, it should be output as is. Personally, I believe this approach is reasonable.

@miquelbeltran
Copy link
Member

miquelbeltran commented Dec 4, 2024

What I do in i18n when a key is not translated, is to display the key, for example if hello_message is "Hello World", but the translation is missing, the app displays [hello_message] in brackets.

We could have a similar approach, and return [$modelIdentifier] when modelName is not available.

At least is a way to know that the modelName is not "translated", but it gives more info than just "unknown model".

@vbuberen
Copy link
Collaborator Author

vbuberen commented Dec 4, 2024

If the return value is "Unknown Model", it means we would need to handle it like this

No, it doesn't mean that somebody need to do like this. It only means in the use case that you are thinking of.

At least is a way to know that the modelName is not "translated", but it gives more info than just "unknown model".

There is already a model identifier field returned separately. It is available for usage already. I would prefer people coming and opening an issue that some model is missing from the list and they see Unknown model instead of confusion with returning just model identifier, etc.

Still don't agree that the output has to be different here. And I would like to not discuss it anymore for now.
I would appreciate if somebody points out what is wrong with out iOS integration tests, though, as it is the main blocker to releasing this feature and I still haven't found out why tests fail in such a strange way.

@ekuleshov
Copy link
Contributor

I am somewhat puzzled why the general use plugin, such as device_info_plus need to hold a copy of the Apple's hardware product lists? And make data transformations based on some other info that platform returns...

As others already noted, the current model code is representative enough for the data analysis and also you could get two app potentially reporting a different model name because one could not update to the latest version of the device_info_plus plugin, even so that the current model string is representative and if an app need to show user the Apple Mac name - it could do that based on its own hardware database.

To extend this comment. Why you are making such change only for iOS and Mac OS, but not for Android and Amazon Fire devices (e.g. their codes like KFKAWI)? I know it is a much bigger list, bit the device info data need to be somewhat consistent between platforms if you want to start doing this.

In any case, as a consumer of this plugin, and someone who has to deal with other dependencies that depends on the device_info_plus, I strongly believe this breaking change is not justified. It will cause more issues that it says it would address.

@miquelbeltran
Copy link
Member

miquelbeltran commented Dec 8, 2024

the current model code is representative enough for the data analysis

That's not true. The current model only returns iPhone or iPad, as it returns the value in https://developer.apple.com/documentation/uikit/uidevice/model

From the docs:

Possible examples of model strings are “iPhone” and “iPod touch”.

The whole point of this PR IMO is replacing the legacy model that only ever returns iPhone or iPad to something a bit more meaningful.

You still have utsname.machine, which will remain the same, and returns the model code: e.g. iPhone14,5


you could get two app potentially reporting a different model name because one could not update to the latest version of the device_info_plus plugin

Maybe for model, but you still have utsname.machine which remains the same.

Previously you would only had iPhone or iPad in the model value, so not much better either way.

@ekuleshov
Copy link
Contributor

the current model code is representative enough for the data analysis

That's not true. The current model only returns iPhone or iPad, as it returns the value in https://developer.apple.com/documentation/uikit/uidevice/model
From the docs:

Possible examples of model strings are “iPhone” and “iPod touch”.

The whole point of this PR IMO is replacing the legacy model that only ever returns iPhone or iPad to something a bit more meaningful.

You still have utsname.machine, which will remain the same, and returns the model code: e.g. iPhone14,5

That is the one I am referring to.

The current machine been useful in multiple ways, e.g. you can make simple single-string checks for device types as they returned by host OS. The iPhone and iPad aren't the only values there. E.g. tvOS and visionOS would return different values and the iOS apps can run at least in the latter.

With these current changes, you can't rely on these values anymore. They are gone and you would have to check for one of the 50 device names or do string manipulation on utsname.machine. Neither is a good idea.

You also yet to see what will be returned for an iOS app running on Apple silicon Mac.

you could get two app potentially reporting a different model name because one could not update to the latest version of the device_info_plus plugin
Maybe for model, but you still have utsname.machine which remains the same.
Previously you would only had iPhone or iPad in the model value, so not much better either way.

I'm saying that the changed model is not representative. You won't be using the current model to identify devices. The utsname.machine is fine for that and you don't need to make any package updates in order to get the correct value from the host OS.

And you can't anymore identify device category, such as iPhone or an iPad.

If you really want to return these "maybe-readable" and potentially incomplete device names, it should not be a breaking change. Please make it a separate property and keep the API backward compatible.

@miquelbeltran
Copy link
Member

miquelbeltran commented Dec 8, 2024

With these current changes, you can't rely on these values anymore. They are gone and you would have to check for one of the 50 device names or do string manipulation on utsname.machine. Neither is a good idea.

As I already mentioned, utsname.machine is not changing. Please take a look at the code in this PR because you are making wrong assumptions.

And you can't anymore identify device category, such as iPhone or an iPad.

The model property was returning a value provided by the OS (https://developer.apple.com/documentation/uikit/uidevice/model), there was also no warranty that the OS returned a valid value or something else than "iPhone" or "iPad" before.

I'd honestly like to hear from a dev using iOS's device info model in their app/library for something specific.

Please make it a separate property and keep the API backward compatible.

That's a valid request.

I personally don't see how the model property (https://developer.apple.com/documentation/uikit/uidevice/model) can be of any use to anyone, and I'd like to be proved wrong.

Edit: on StackOverflow people suggest using it to detect if iPad: https://stackoverflow.com/questions/53763886/flutter-check-if-app-is-running-in-ipad-or-iphone although they don't match the whole string but with contains. The right way to do that would be to read the userInterfaceIdiom from iOS, something that the device_info_plus could expose if not doing it already.

BUT on the other hand, I like that the plugins provide values “as is”, so I'd be in favor to keep the properties as they are and instead provide a new one for the “translation”.

@ekuleshov
Copy link
Contributor

Please make it a separate property and keep the API backward compatible.
That's a valid request.

I personally don't see how the model property (https://developer.apple.com/documentation/uikit/uidevice/model) can be of any use to anyone, and I'd like to be proved wrong.

I'm using the model property to tailor some UI to iPad screens.
I find it easier to deal with than looking at the screen size and DPI.

Edit: on StackOverflow people suggest using it to detect if iPad: https://stackoverflow.com/questions/53763886/flutter-check-if-app-is-running-in-ipad-or-iphone although they don't match the whole string but with contains. The right way to do that would be to read the userInterfaceIdiom from iOS, something that the device_info_plus could expose if not doing it already.

The userInterfaceIdiom is not about device but about currently active interface idiom of the ones supported in the app

BUT on the other hand, I like that the plugins provide values “as is”, so I'd be in favor to keep the properties as they are and instead provide a new one for the “translation”.

Thank you. I hope it could be updated this way.

@vbuberen
Copy link
Collaborator Author

vbuberen commented Dec 9, 2024

I will write just one response and won't discuss it further with you, because, from previous interactions over the last few years I know that there will be a list of demands and always maintainers do things wrong and only you know how the package should work, how API should look, etc. It happened with changes to share_plus, already happened with device_info_plus before and now again. At the same time there were no real code contributions from you at all with claims that Plus Plugins setup is too complex and you couldn't build packages locally.

This change is for an often requested feature, which was asked not once already in the repo if you do the search. The issue mentioned in the PR description exists for almost a year now and except upvotes I didn't see any feedback, including none from you. Not to mention that the issue is a better place for such discussions, not the PR.

Why you are making such change only for iOS and Mac OS, but not for Android and Amazon Fire devices (e.g. their codes like KFKAWI)?

Because if you would check the API for Android info you would see that in Android model property already returns an actual model for devices where vendors care. Here is a screenshot from Google Pixel 7, for example:

photo_2024-12-09 13 04 09

iOS/MacOS devices don't do so and while being one of the most popular platforms it is something that is currently missing.

I'm saying that the changed model is not representative. You won't be using the current model to identify devices. The utsname.machine is fine for that and you don't need to make any package updates in order to get the correct value from the host OS.

It wasn't representative before with a generic iPhone, iPad, etc. At the same time on Android users can use model field as I showed above, so people who might use the plugin for analytics, support, etc. will be able to see understandable device model instead of the need to do model translation for iOS/MacOS devices.

With these current changes, you can't rely on these values anymore. They are gone and you would have to check for one of the 50 device names or do string manipulation on utsname.machine. Neither is a good idea.

You still have words iPhone, iPad etc. in the returned model, so with simple contains() you can check.
If you would also check the returned properties more you would see that you still have localizedModel property which returns exactly the same info as model without any changes, despite Apple's documentation saying that this should be localised version of model.

I'm using the model property to tailor some UI to iPad screens.

Alright, this is why the change is marked as breaking, so people like you who don't want to put an effort into changing their code can not be afraid that something breaks for them or ignore this update.

So with that being said I saw mostly rant for the sake of rant like it happened before not once from your side.

@ekuleshov

This comment was marked as spam.

@ekuleshov
Copy link
Contributor

I will reiterate.

This functionality can be added as a non-breaking change (i.e. keep existing properties as is and add a new one with the descriptive device info).

That would address several concerns mentioned in the conversation above.

PS: regarding Android info including device name.

I run the demo app on a real Amazon Fire 8 device and I don't see the Fire 8 name there

image

@vbuberen vbuberen force-pushed the feature/apple_models_name branch from e530bcc to 20153d1 Compare December 10, 2024 09:57
@vbuberen vbuberen changed the title feat(device_info_plus)!: Return model name for iOS and MacOS devices feat(device_info_plus): Return model name for iOS and MacOS devices Dec 10, 2024
@vbuberen vbuberen merged commit 63ca4cd into main Dec 10, 2024
18 of 20 checks passed
@vbuberen vbuberen deleted the feature/apple_models_name branch December 10, 2024 14:07
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.

[Request]: Show iOS user known device model names
4 participants