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

[deviceinfo] Add os and firmware information properties. JB#57114 #22

Merged
merged 1 commit into from Jan 27, 2022

Conversation

spiiroin
Copy link
Contributor

Allows lightweight access to os / firmare version information without
using custom /etc/*-release parsers.

Signed-off-by: Simo Piiroinen simo.piiroinen@jollamobile.com

src/deviceinfo.h Outdated
@@ -48,6 +48,9 @@ class SYSTEMSETTINGS_EXPORT DeviceInfo: public QObject
Q_PROPERTY(QString designation READ designation CONSTANT)
Q_PROPERTY(QString manufacturer READ manufacturer CONSTANT)
Q_PROPERTY(QString prettyName READ prettyName CONSTANT)
Q_PROPERTY(QString osName READ osName CONSTANT)
Q_PROPERTY(QString osVersion READ osVersion CONSTANT)
Q_PROPERTY(QString firmwareVersion READ firmwareVersion CONSTANT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Firmware being a bit vague term I'm pondering if we could use something more concrete. In our case this is sort of hw adaptation version. Also to be seen how much we need this exposed, ref: comment on buteo-mtp if it should use osVersion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was struggling with that... usually one tries to keep terminology in sync with ssu, but this is something ssu does not expose. And we have something that is read from "hw-release", QDeviceInfo legacy is "firmwareVersion", and what it actually means is "adaptation version"...

I've been thinking about: use "hw_release" naming in ssu-sysinfo to match source of data (which is what ssu does internally too), and "adaptationVersion" here where we know we are dealing with sailfish context. How does that sound to you?

In any case, I guess it would not hurt to document all of those properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about: use "hw_release" naming in ssu-sysinfo to match source of data (which is what ssu does internally too), and "adaptationVersion" here where we know we are dealing with sailfish context. How does that sound to you?

Those names sound better to me. Though thinking should we even add this yet here since usage got dropped in buteo etc.

For another thing, this and the os name + version are already also available in AboutSettings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those names sound better to me.

Changed, both here and in ssu-sysinfo.

Though thinking should we even add this yet here since usage got dropped in buteo etc.

There are more packages to patch. And migrating is easier if there is feature parity. Not that I'm sure all of these are that beneficial. But it is easy to drop them once it is known for sure that they are needed - as long as dropping is done before release. Or alternatively, once things stabilize enough, I could stash them in separate feature branch to be added if there is genuine need.

For another thing, this and the os name + version are already also available in AboutSettings.

If the long term goal is to have single source for this info, having c lib, used by a qt/qml wrapper lib + making AboutSettings use that too would seem like a logical choice to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add comments here and here:

I was thinking about "developer documentation", so describing what the methods are supposed to do and how they relate to similar functions elsewhere. Having comments in /etc files is probably not helpful towards that goal, or did I misunderstand what you meant?

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

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

LGTM. There is some api duplication between the DeviceInfo and AboutSettings but that's already there. Maybe DeviceInfo is anyway better name as source of this info.

@spiiroin
Copy link
Contributor Author

Review commits squashed

Allows lightweight access to os / firmare version information without
using custom /etc/*-release parsers.

Signed-off-by: Simo Piiroinen <simo.piiroinen@jollamobile.com>
@spiiroin
Copy link
Contributor Author

Force pushed
+BuildRequires: pkgconfig(ssu-sysinfo) >= 1.4.0

@spiiroin spiiroin merged commit de83a9d into sailfishos:master Jan 27, 2022
@spiiroin spiiroin deleted the jb57114_os_version branch January 27, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants