Add method to check whether a data source is active or not
ClosedPublic

Authored by vitalyf on Mar 14 2019, 12:18 PM.

Details

Summary

"Active" state means that all data collected by the data source will be
sent to a server.
This change aims to give a user more control of what data they want to
send. Without this functionality the user might decide to reduce
telemetry level because of one small part of it. This means that we can
collect less statistic without a really good reason.

Diff Detail

Repository
R849 User Feedback Collection Framework
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vitalyf requested review of this revision.Mar 14 2019, 12:18 PM
vitalyf created this revision.

Do we need to persist this setting?

Do we need to persist this setting?

For most of users, I think so.

And there are a few ways to implement this:

  1. Hard way (decorator-like approach): de-virtualize store/load/reset methods. Add new virtual methods with with the same names, but suffix "Impl". These methods will be used by children. De-virtualized methods will be implemented like this:
load(...)
{
    // Custom logic
    loadImpl(...);
}
  1. Easy way: change ProviderPrivate::store/load to also write/read additional settings like "active" flag.
  2. Leave it as is. If user need this functionality, he can easily implement it in code around.

Personally I would prefer the first option, but on the other hand, the second one involves less changes. What do you think is the best?

I had option (2) in mind initially, assuming that this doesn't hurt even for scenarios where this functionality isn't needed. It would also be consistent with how the current settings are persisted, avoiding the surprise that disabled elements get automatically activated again if you don't implement persistence manually in your application. (1) would achieve the same thing I think, so I'd be ok with that too. I just don't like option (3) :)

I had option (2) in mind initially, assuming that this doesn't hurt even for scenarios where this functionality isn't needed. It would also be consistent with how the current settings are persisted, avoiding the surprise that disabled elements get automatically activated again if you don't implement persistence manually in your application. (1) would achieve the same thing I think, so I'd be ok with that too. I just don't like option (3) :)

Ok, I'm going to implement option (1) then, because it looks like more universal solution and will produce the same results for persisting settings via Provider and DataSource itself.

vitalyf updated this revision to Diff 53962.Mar 15 2019, 3:46 PM

"Active" state is stored in the settings now.
Also added unit tests covers new functionality.

Thanks! Almost there, but I think this is too aggressive in reset(). reset() is meant to reset internal counters etc between submissions, not to reset user settings, so that should not touch the active flag.

vitalyf updated this revision to Diff 54184.Mar 18 2019, 10:58 AM

"Active" state is not reset in between data submissions anymore.
Also fixed unit test for this piece of functionality.

vkrause accepted this revision.Mar 18 2019, 6:59 PM

Thanks!

This revision is now accepted and ready to land.Mar 18 2019, 6:59 PM
This revision was automatically updated to reflect the committed changes.