Prevent location state changes during emergency calls #27
Conversation
…57710 AML is ran with the emergency calls and it requires to keep location services, i.e., GPS on during the call. It must not be allowed for the user to try to change this during the call. This change tracks the call state with QMceCallState functionality. When state is "Active" and type is "Emergency" all changes made to the location or GPS status are prevented and warning is logged.
| @@ -466,6 +479,12 @@ bool LocationSettings::locationEnabled() const | |||
| void LocationSettings::setLocationEnabled(bool enabled) | |||
| { | |||
| Q_D(LocationSettings); | |||
|
|
|||
| if (d->isEmergencyCallActive()) { | |||
| qWarning() << "Cannot allow to change location enabled status when emergency call is active"; | |||
There was a problem hiding this comment.
Just wondering whether we want debug logging for any attempt to enable/disable, or just for the ones that would actually lead to changes i.e. should this be withing the below "if it differs from current" conditional block?
Also, is it okay for qml property setter not to set the property? I'd guess yes, but does somebody know for sure?
There was a problem hiding this comment.
Sort of related, if there were properties like "canToggleLocationEnabled" or stte -> those could perhaps be used in (qml) ui to disable/hide those element that could lead to attempts to make such changes?
There was a problem hiding this comment.
That is a good option I think, to have a canToggleLocation visible upwards to UI.
There was a problem hiding this comment.
Added some with 517f7bd -> naming needs to be rethought. On Friday evening brain just shortcircuits.
src/locationsettings.cpp
Outdated
| return; | ||
|
|
||
| m_callstate = m_mceCallState->state(); | ||
| m_calltype = m_mceCallState->type(); |
There was a problem hiding this comment.
Any reason to have member variables for these instead of just calling getters when evaluating the state?
There was a problem hiding this comment.
No, not any particular reason. I can drop those if polling is preferred.
There was a problem hiding this comment.
Looks like on mce-qt these are in the end plain "return iValue;" type of code so the extra copies shouldn't provide much. -> Let's just call the getters when the status is needed.
There was a problem hiding this comment.
Yeah I was just looking the same that it does not create a new request but just returns internal value. On it.
There was a problem hiding this comment.
But, if we'd need the canToggleGPS property I think we'd need to keep track of the state in order for that to work.
There was a problem hiding this comment.
In that case it could boil down to boolean. Not sure which would be better but such canToggle & protection could be even implemented fully in the UI side as the location setting modification is couple very specific places.
Speaking of that, there's also already the policy support for disallowing location settings changes in the UI.
There was a problem hiding this comment.
An enum vs. boolean? I'd take the former any day in state tracking. It is more future proof. I think the safeguard should not be only in UI but could be also there to track what is going on beneath.
Where is that policy support implemented?
There was a problem hiding this comment.
Boolean as in canToggleGPS sounds quite much like such.
Policy is sailfish-policy, living with the mdm libraries. UI side honors the policy, e.g. systemsettings / location.qml, disables the settings if the policy prevents usage.
There was a problem hiding this comment.
I really think the enum serves here as we might need more granularity 517f7bd (naming improvement suggestions are welcome).
src/locationsettings.cpp
Outdated
| @@ -466,6 +479,12 @@ bool LocationSettings::locationEnabled() const | |||
| void LocationSettings::setLocationEnabled(bool enabled) | |||
| { | |||
| Q_D(LocationSettings); | |||
|
|
|||
| if (d->isEmergencyCallActive()) { | |||
There was a problem hiding this comment.
Pondering should it be allowed to turn the location/gps on.
There was a problem hiding this comment.
Why? AML does it already.
There was a problem hiding this comment.
And AML controls the location anyways, when battery is low it keeps running but shuts down all power consumers to have some grace period for the user to find a charger. Anyways, it would compete with AML requirements and the AML service, which will force the value to the file causing maybe confusion as in such case user would see "-> on -> off" and vice versa. Better to have this ignore all cases.
Or maybe if we could have an additional QMceCallState or variable aml_enabled ? In such case all actions would be blocked but when AML is not enabled user could control this.
There was a problem hiding this comment.
Thinking of robustness and limiting only the minimum. AML is sort of an add-on so these rules on this level are not totally unproblematic, and also it's now assuming that AML will not use this component while on the other hand it would be good if all access to the file would go via this.
There was a problem hiding this comment.
AML is a service that at some point should also be launched at the lock UI so I think this is not available then. It is simple enough just to prevent user access to the options as there is a legislative requirement to do so. I left opening for enable only option in 517f7bd which might be needed. Anyways, all emergency calls != AML is on.
There was a problem hiding this comment.
I'm starting to think that if the situation is not yet clear enough to have a future-proof mw policy it might be better not to implement such a thing. Especially if the only gotcha would be having device user repeatedly and persistently toggling things in settings app during an ongoing emergency call. Or was there something else too?
This adds the properties and signals for getting info whether the location and GPS state can be modified. An enum is used to define the level of access to enabling location&GPS with a bit more granularity. Not all emergency calls are AML enabled calls that need explicit access to location. As of now, we can just detect emergency call -> always disabled when it is active.
|
In general I think the |
AML is ran with the emergency calls and it requires to keep location
services, i.e., GPS on during the call. It must not be allowed for the
user to try to change this during the call.
This change tracks the call state with QMceCallState funcionality. When
state is "Active" and type is "Emergency" all changes made to the
location or GPS status are prevented and warning is logged.