NotifTroubleshootingScreen: Add Android manufacturer/model info to report #5754
Conversation
src/reactNativeUtils.js
Outdated
| export function androidManufacturer(): string { | ||
| invariant(Platform.OS === 'android', 'androidRelease called on iOS'); |
There was a problem hiding this comment.
nit: invariant message is wrong, looks like a copy-paste issue
src/reactNativeUtils.js
Outdated
| // Flow isn't refining `Platform` to a type that corresponds to values | ||
| // we'll see on Android. | ||
| // |
There was a problem hiding this comment.
Maybe leave out this comment; it's not adding much information, and it bulks these functions up (which then tends to conceal things like the "androidRelease called" issue above).
src/reactNativeUtils.js
Outdated
| // Flow isn't refining `Platform` to a type that corresponds to values | ||
| // we'll see on Android. | ||
| // | ||
| // (If changing the implementation, adjust comment below jsdoc.) |
There was a problem hiding this comment.
Then this last comment, and the blank line, can be left out too — because now the implementation just below this is just a couple of lines away from that other comment, so there's no longer a need for a second comment to point to it.
src/reactNativeUtils.js
Outdated
| /** | ||
| * "The end-user-visible name for the end product." | ||
| * | ||
| * E.g., "sdk_gphone64_x86_64", "SM-G960U1". |
There was a problem hiding this comment.
Unfortunately, in my test using an emulated Pixel XL (results recorded in jsdoc), none of these three fields actually had anything like "Pixel XL" in it.
On my physical Pixel 5, I get:
"manufacturer": "Google",
"brand": "google",
"model": "Pixel 5",
So that's encouraging. And for this line we can write:
| * E.g., "sdk_gphone64_x86_64", "SM-G960U1". | |
| * E.g., "Pixel 5", "sdk_gphone64_x86_64", "SM-G960U1". |
chrisbobbe
force-pushed
the
pr-notif-report-android-manufacturer
branch
from
c02b23b to
340dcc3
Compare
|
Thanks for the review! Revision pushed. |
src/reactNativeUtils.js
Outdated
| export function androidModel(): string { | ||
| invariant(Platform.OS === 'android', 'androidRelease called on iOS'); |
…port This could provide useful context, e.g., to diagnose issues like issue zulip#4074, "Some device vendors break notifications". Unfortunately, in my test using an emulated Pixel XL (results recorded in jsdoc), none of these three fields actually had anything like "Pixel XL" in it. Possibly we could get that from something like Android's `Build.PRODUCT`, documented as "the name of the overall product": https://developer.android.com/reference/android/os/Build#PRODUCT but React Native's Platform.constants doesn't expose that value or any other that seems likely, and we have more urgent tasks than plumbing things through React Native (like <https://github.com/zulip/zulip-flutter>).
chrisbobbe
force-pushed
the
pr-notif-report-android-manufacturer
branch
from
340dcc3 to
a53b5ed
Compare
|
Thanks for the review! Merging, with that fix. |
This could provide useful context, e.g., to diagnose issues like issue #4074, "Some device vendors break notifications".
Unfortunately, in my test using an emulated Pixel XL (results recorded in jsdoc), none of these three fields actually had anything like "Pixel XL" in it. Possibly we could get that from something like Android's
Build.PRODUCT, documented as "the name of the overall product":https://developer.android.com/reference/android/os/Build#PRODUCT
but React Native's Platform.constants doesn't expose that value or any other that seems likely, and we have more urgent tasks than plumbing things through React Native (like
https://github.com/zulip/zulip-flutter).