-
Notifications
You must be signed in to change notification settings - Fork 27
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
Bug 1706102 - Collect Qt platform information #288
Conversation
This uses the global `Qt` object to get the OS name and the locale. Unfortunately, os version and architecture are not available.
This is required in order to satisfy the TS compiler. Without this file, we would be forced to pollute the code with various eslint rules suppression.
async osVersion(): Promise<string> { | ||
// This data point is not available in Qt QML. | ||
return Promise.resolve("Unknown"); | ||
}, | ||
|
||
async arch(): Promise<string> { | ||
// This data point is not available in Qt QML. | ||
return Promise.resolve("Unknown"); | ||
}, |
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.
@bakulf do you know any way to get this in Qt-QML?
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.
My main concern is related to the use of Qt.locale(). The rest of the code looks good!
}, | ||
|
||
async locale(): Promise<string> { | ||
const locale = Qt.locale(); |
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.
Reading this: https://doc.qt.io/qt-5/qml-qtqml-qt.html#locale-method it seems that if name
is not valid, QT uses the 'C' locale. Have you tested if this works with non-'C' locales?
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.
Yup, this seems to behave as expected, thanks!
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.
I'm not sure I understand the docs, so for understanding: this will give us the dash-separated locale like es-ES
or en-US
?
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.
I'm not sure I understand the docs, so for understanding: this will give us the dash-separated locale like
es-ES
oren-US
?
Good thing you mentioned this, I double checked and I missed that we were sending es_ES
and not es-ES
. I twaked the code to fix this :)
This was mistakenly using the QT path, but it was working because the QT platform was using the "test platform".
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.
Seems simple enough for what's there. Just the one question about the locale.
If Qt has functionality to get os_version and arch, then yeah, we should include that, but it's fine to report unknown for now too
}, | ||
|
||
async locale(): Promise<string> { | ||
const locale = Qt.locale(); |
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.
I'm not sure I understand the docs, so for understanding: this will give us the dash-separated locale like es-ES
or en-US
?
This means reporting "it-IT" instead of "it_IT".
This uses the global
Qt
object to get the OS name and the locale. Unfortunately, os version and architecture are not available.I manually tested that this works using the Glean.js Qt sample app. I get the following output: