Add device class field for Android #951
Conversation
BiliBoomBo
requested review from
bliptec,
kindbe,
lalitb,
larvacea,
maxgolov,
mkoscumb,
reyang and
sid-dahiya
as code owners
|
|
|
@saurabh-sagrawal , @larvacea - Could you please help review this PR. Thanks. |
|
@lalitb , @saurabh-sagrawal , @larvacea All CI build has passed, please help to review this pr, thanks~ |
| @@ -274,6 +275,8 @@ private void calculateAndSetSystemInfo(android.content.Context context) { | |||
|
|
|||
| String time_zone = getTimeZone(); | |||
|
|
|||
| String device_class = getDeviceClass(context); | |||
There was a problem hiding this comment.
"final String device_class", it's better to mark the variable final to avoid accidental value changes down the line.
Similarly update other variables as well.
There was a problem hiding this comment.
Sure, have made the change.
| return "Android.PC"; | ||
| } else { | ||
| return "Android.Phone"; |
There was a problem hiding this comment.
- These should be constants declared somewhere at a common place.
- What's the source for this and how are these values determined, is the list of values complete and why are you not expecting tablet here?
There was a problem hiding this comment.
What's the source for this and how are these values determined, is the list of values complete and why are you not expecting tablet here?
@saurabh-sagrawal - the list is coming from common schema Part A extension:
https://github.com/microsoft/common-schema/blob/main/PartA/device.md#deviceclass
| iOS | iOS.Emulator iOS.Phone iOS.Tablet iOS.Phablet iOS.AppleTV |
|---|---|
| Android | Android.Phone Android.PC |
There was a problem hiding this comment.
Thanks lalitb for help to reply!
And I have made the related change for 1.
There was a problem hiding this comment.
It's cool, my minor suggestion would be to revisit / review this in schema forum. These definitions were just someone's random suggestion, and may not be very neatly tuned / optimal for today's realities as of 2021... i.e. Android.PC - what is that exactly, like a Chromebook or a Tablet with external keyboard attached??
There was a problem hiding this comment.
It may be a random suggestion, but it may hard to change these values right now, the low version Edge app uploads these strings, and the other apps may have the similiar situation, we may need to keep them consistent.
|
I'd like to complete this PR if there is no more comments. |
Yes, looks good to go now. |
* add device class for Android * Fix CI build fail and add test for device_class * Fix test build fail * Resolve comments Co-authored-by: Wangyang Dai <[email protected]> Co-authored-by: Lalit Kumar Bhasin <[email protected]>
* add device class for Android * Fix CI build fail and add test for device_class * Fix test build fail * Resolve comments Co-authored-by: Wangyang Dai <[email protected]> Co-authored-by: Lalit Kumar Bhasin <[email protected]> Co-authored-by: Wangyang Dai <[email protected]> Co-authored-by: Lalit Kumar Bhasin <[email protected]>
https://1dsdocs.azurewebsites.net/schema/PartA/device.html#deviceclass