-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
...lus/device_info_plus/macos/device_info_plus/Sources/device_info_plus/DeviceIdentifiers.swift
Show resolved
Hide resolved
Don't know why iOS integration test fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Could be that increasing the minimum deployment target makes these properties mandatory:
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 |
I saw these warnings earlier, but still tried to add version - still failing. Will investigate later. |
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. |
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 |
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. |
What I do in i18n when a key is not translated, is to display the key, for example if We could have a similar approach, and return At least is a way to know that the |
No, it doesn't mean that somebody need to do like this. It only means in the use case that you are thinking of.
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 Still don't agree that the output has to be different here. And I would like to not discuss it anymore for now. |
I am somewhat puzzled why the general use plugin, such as 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 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 |
That's not true. The current From the docs:
The whole point of this PR IMO is replacing the legacy You still have
Maybe for Previously you would only had |
That is the one I am referring to. The current 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 You also yet to see what will be returned for an iOS app running on Apple silicon Mac.
I'm saying that the changed 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. |
As I already mentioned,
The I'd honestly like to hear from a dev using iOS's device info
That's a valid request. I personally don't see how the 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 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”. |
I'm using the
The
Thank you. I hope it could be updated this way. |
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 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.
Because if you would check the API for Android info you would see that in Android iOS/MacOS devices don't do so and while being one of the most popular platforms it is something that is currently missing.
It wasn't representative before with a generic
You still have words
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. |
This comment was marked as spam.
This comment was marked as spam.
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. |
e530bcc
to
20153d1
Compare
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 likeiPhone 15 Pro
, etc. instead of justiPhone
,iPad
.Screenshot from iPhone with model name
On MacOS there is also the same
modelName
property that returns names likeMacBook Pro 16-inch (2021)
Screenshot from MacBook with model name and identifier
Related Issues
Closes #2656
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
!
in the title as explained in Conventional Commits).