No more singleton #8
Conversation
We handle the mutability internally and ensure access is synchronized. This mimics the behavior of Rkv as well.
| } | ||
|
|
||
| /** | ||
| * Returns true if the Glean library has been initialized. | ||
| */ | ||
| internal fun isInitialized(): Boolean { | ||
| val initialized = LibGleanFFI.INSTANCE.glean_is_initialized() | ||
| if (glean == 0L) { |
There was a problem hiding this comment.
With lateinit, we can simply check glean.isInitialized, making klint happy.
| @@ -1,5 +1,5 @@ | |||
| ifeq ($(ANDROID_HOME),) | |||
| ANDROID_HOME := ~/Library/Android | |||
| ANDROID_HOME := ~/Library/Android/sdk | |||
There was a problem hiding this comment.
On my machine this is Sdk. Will we run into issues on case-sensitive filesystems here?
| @@ -45,7 +43,8 @@ impl CommonMetricData { | |||
| } | |||
|
|
|||
| pub fn should_record(&self) -> bool { | |||
| if self.disabled || !Glean::singleton().is_upload_enabled() { | |||
| //if self.disabled || !Glean::singleton().is_upload_enabled() { | |||
There was a problem hiding this comment.
Should we add a TODO here to remember to come back to this so Glean is passed in and we can check whether upload is enabled?
| @@ -13,7 +13,8 @@ import mozilla.telemetry.glean.rust.RustError | |||
|
|
|||
| open class GleanInternalAPI internal constructor () { | |||
| // `internal` so this can be modified for testing | |||
| internal var bool_metric: MetricHandle = 0 | |||
| internal var bool_metric: MetricHandle = 0L | |||
| internal var glean: MetricHandle = 0L | |||
There was a problem hiding this comment.
It might make sense to use lateinit for this. We should also probably call this apiHandle, and directly use it from the types.
| return initialized.toInt() != 0 | ||
| } | ||
|
|
||
| fun handle(): Long { |
There was a problem hiding this comment.
We don't need this: we should be able to access internal variables from the types.
| @@ -45,7 +43,8 @@ impl CommonMetricData { | |||
| } | |||
|
|
|||
| pub fn should_record(&self) -> bool { | |||
| if self.disabled || !Glean::singleton().is_upload_enabled() { | |||
| //if self.disabled || !Glean::singleton().is_upload_enabled() { | |||
There was a problem hiding this comment.
How can we now handle is_upload_enabled? Would it help to pass around the Glean instance rathre than the Storage alone? Then we could check, at call site, if upload is enabled or not. Or maybe we don't simply care now, and keep the same behaviour as desktop.
Pull Request checklist
cargo clean; cargo test --allruns without emitting any warningscargo fmtdoes not produce any changes to the code