[deviceinfo] Add imeiNumbers property. JB#56945 #23
Conversation
There was a problem hiding this comment.
Just some style related comments. I also looked that those types (e.g. QString and QDBusObjectPath) in arguments thinking if they should be const references but it probably doesn't make much difference here. AFAIK Qt can handle const references just fine in slots but I'm not sure there is any performance difference in practice.
src/deviceinfo.cpp
Outdated
| }; | ||
|
|
||
| DeviceInfoPrivate::DeviceInfoPrivate() | ||
| DeviceInfoPrivate::DeviceInfoPrivate(bool synchronousInit) | ||
| : m_ofonoTracker(new OFonoTracker(synchronousInit, nullptr)) |
There was a problem hiding this comment.
Should we actually do this stuff elsewhere? Now the ofono tracking is started for anything using DeviceInfo when none of the existing usage is interested about it. Furthermore this class didn't use to refer to too secrets things, but the ofono side requires permissions -> one could argue anything using this should be having Phone permission.
Maybe this could warrant a separate class?
There was a problem hiding this comment.
Separate class, having ofono tracking on demand, as singleton, etc would be things to consider if/when the impl is otherwise not bonkers...
There was a problem hiding this comment.
By the looks of it, libqofono that @monich brought up already has all the low level technical details covered (shared instances, optional blocking, whatnot). I wish I would have known about it a bit earlier... having a new parallel implementation of essentially the same thing with possibility for fresh set of hiccups probably does not make too much sense.
Now, does it also provide a way to "get all imeis" that is easy enough that we can use it as is - without any glue classes?
If not, having a wrapper class that != "DeviceInfo" using libqofono would probably be preferable - any suggestions about naming?
There was a problem hiding this comment.
Custom ofono tracking dropped in favor of using libqofono. @monich please check if it looks sane.
Regarding permissions: a process that does not have them, gets empty imei set. Which sounds about right to me. Or at least better than splitting things into multiple interfaces based on current permission requirements, or?
Also changed things so that D-Bus access is done on demand so that no ofono access is done unless imei numbers are queried -> we can still move this to somewhere else, but that would be arch decision rather than technical one.
|
Out of curiosity, what's the reason for not using libqofono? |
For example: not knowing that such thing exists. |
spiiroin
force-pushed
the
jb56945_imei_numbers
branch
from
d6b3efe
to
f9d345a
Compare
|
Review commits squashed |
spiiroin
force-pushed
the
jb56945_imei_numbers
branch
2 times, most recently
from
dbefc0f
to
715651d
Compare
There was a problem hiding this comment.
Not entirely sure should the imei list be here or in some separate class, but as the usage isn't common and the existing DeviceInfo instances don't now start fetching ofono properties maybe it's good enough to start with. Kind of interesting API, though, when the async version requires to first call getter to return empty list.
| * | ||
| * Should be functionally equivalent with obtaining data via: | ||
| * QDeviceInfo::imei() | ||
| * QDeviceInfo::imeiCount() |
There was a problem hiding this comment.
Could myself skip this latter notice as imeis should be equivalent no matter where they are gotten and the usage isn't too widespread. But doesn't matter much.
As an enabler for deprecating QDeviceInfo class from qt5-qtsysteminfo package, provide access to device IMEI numbers via DeviceInfo class. By default imeiNumbers property value gets determined and updated in a fully asynchronous manner. But to provide behavior that is compatible with QDeviceInfo it is possible to request synchronous initialization and make DeviceInfo constructor block until current IMEI numbers have been fetched. Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
spiiroin
force-pushed
the
jb56945_imei_numbers
branch
from
ad00ec3
to
518f0e6
Compare
|
Review commits squashed |
As an enabler for deprecating QDeviceInfo class from qt5-qtsysteminfo
package, provide access to device IMEI numbers via DeviceInfo class.
By default imeiNumbers property value gets determined and updated in a
fully asynchronous manner. But to provide behavior that is compatible
with QDeviceInfo it is possible to request synchronous initialization
and make DeviceInfo constructor block until current IMEI numbers
have been fetched.
Signed-off-by: Simo Piiroinen simo.piiroinen@jolla.com