mer-hybris / geoclue-providers-hybris

GNU Lesser General Public License v2.1
2 stars 17 forks source link

[geoclue-provider-hybris] Add Cold Start. Contributes to JB#41709 #28

Closed dmamaevomp closed 4 years ago

dmamaevomp commented 4 years ago

Added org.freedesktop.Geoclue.Data interface to support GNSS cold start.

pvuorela commented 4 years ago

No, it shouldn't be com.gps either, that's overly generic.

You didn't comment anything if this even needs a separate service.

And you haven't still commented anything about the common api.

chriadam commented 4 years ago

Thanks for your work on this, I think we're definitely getting closer to merge-able state.

My personal opinion is that:

1) we should favour using the SetOptions() way if that will suffice, as then we don't need to add or extend any interface, but can simply use the existing one 2) if the SetOptions() way won't suffice, and a custom interface is needed, then I would prefer the interface to be org.freedesktop.Geoclue.Providers.Hybris as per Pekka's earlier suggestion

dmamaevomp commented 4 years ago

Thanks for your work on this...

How do I use the SetOptions method? I didn't quite understand. Please explain

chriadam commented 4 years ago

Thanks for your work on this...

How do I use the SetOptions method? I didn't quite understand. Please explain

My understanding is that you just need to pass a QVariantMap (i.e. DBus a{sv}) parameter. e.g. clients could call SetOptions({ "GnssDeleteAidingData": true })

The code in ::SetOptions() would have some extra check like:

if (options.contains(QStringLiteral("GnssDeleteAidingData"))
        && options.value(QStringLiteral("GnssDeleteAidingData")).toBool()
        && m_backend) {
    //GPS_DELETE_ALL = 0xFFFF (almanac, ephemeris, position, time and other cache data)
    m_backend->gnssDeleteAidingData(0xFFFF);
}

You can see that SetOptions is already used to set the UpdateInterval (i.e. polling interval) for the plugin, currently.

--

After thinking about this a bit more, it does seem like these Options are more like settings rather than intended for a way to trigger some behaviour (such as this GnssDeleteAidingData). So perhaps a separate interface with bespoke invokable method is the best option after all: @pvuorela what's your thoughts currently?

dmamaevomp commented 4 years ago

Thanks for the clarification. Now everything is clear to me! I agree that this is not an option, but I do not mind doing it through SetOptions Then I wait for the final decision from @pvuorela

pvuorela commented 4 years ago

After thinking about this a bit more, it does seem like these Options are more like settings rather than intended for a way to trigger some behaviour (such as this GnssDeleteAidingData). So perhaps a separate interface with bespoke invokable method is the best option after all: @pvuorela what's your thoughts currently?

True, it's not a search option as much as some others, for example that "UpdateInterval". Then again if wanted it could be named a bit more like one, say "NoCachedAidingData".

But if it's now really just needed for testing help, I think the options way would be the most straightforward. One thing I could consider if the options name should include hybris to make it really provider specific. Like "HybrisDeleteAidingData".

dmamaevomp commented 4 years ago

Please check the updated code

pvuorela commented 4 years ago

This I can approve, thanks!

LGTM.

dmamaevomp commented 4 years ago

Thanks to all!