Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
package com.duckduckgo.app.fire

import androidx.test.annotation.UiThreadTest
import androidx.test.platform.app.InstrumentationRegistry
import androidx.work.WorkManager
import com.duckduckgo.app.global.view.ClearDataAction
import com.duckduckgo.app.settings.clear.ClearWhatOption
import com.duckduckgo.app.settings.clear.ClearWhenOption
import com.duckduckgo.app.settings.db.SettingsDataStore
import com.duckduckgo.app.statistics.pixels.Pixel
import com.nhaarman.mockitokotlin2.*
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
Expand All @@ -39,12 +41,14 @@ class AutomaticDataClearerTest {
private val mockClearAction: ClearDataAction = mock()
private val mockTimeKeeper: BackgroundTimeKeeper = mock()
private val mockWorkManager: WorkManager = mock()
private val pixel: Pixel = mock()
private val dataClearerForegroundAppRestartPixel = DataClearerForegroundAppRestartPixel(InstrumentationRegistry.getInstrumentation().targetContext, pixel)

@UiThreadTest
@Before
fun setup() {
whenever(mockSettingsDataStore.hasBackgroundTimestampRecorded()).thenReturn(true)
testee = AutomaticDataClearer(mockWorkManager, mockSettingsDataStore, mockClearAction, mockTimeKeeper)
testee = AutomaticDataClearer(mockWorkManager, mockSettingsDataStore, mockClearAction, mockTimeKeeper, dataClearerForegroundAppRestartPixel)
}

private suspend fun simulateLifecycle(isFreshAppLaunch: Boolean) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright (c) 2020 DuckDuckGo
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.duckduckgo.app.fire

import android.content.Intent
import android.net.Uri
import androidx.test.platform.app.InstrumentationRegistry
import com.duckduckgo.app.browser.BrowserActivity
import com.duckduckgo.app.statistics.pixels.Pixel
import com.duckduckgo.app.systemsearch.SystemSearchActivity
import com.nhaarman.mockitokotlin2.mock
import com.nhaarman.mockitokotlin2.verify
import org.junit.Test

class DataClearerForegroundAppRestartPixelTest {

private val context = InstrumentationRegistry.getInstrumentation().targetContext
private val pixel = mock<Pixel>()
private val testee = DataClearerForegroundAppRestartPixel(context, pixel)

@Test
fun whenAppRestartsAfterOpenSearchWidgetThenPixelWithIntentIsSent() {
val intent = SystemSearchActivity.fromWidget(context)
testee.registerIntent(intent)
testee.incrementCount()

testee.firePendingPixels()

verify(pixel).fire(Pixel.PixelName.FORGET_ALL_AUTO_RESTART_WITH_INTENT)
}

@Test
fun whenAppRestartsAfterOpenExternalLinkThenPixelWithIntentIsSent() {
val i = givenIntentWithData("https://example.com")
testee.registerIntent(i)
testee.incrementCount()

testee.firePendingPixels()

verify(pixel).fire(Pixel.PixelName.FORGET_ALL_AUTO_RESTART_WITH_INTENT)
}

@Test
fun whenAppRestartsAfterOpenAnEmptyIntentThenPixelIsSent() {
val intent = givenEmptyIntent()
testee.registerIntent(intent)
testee.incrementCount()

testee.firePendingPixels()

verify(pixel).fire(Pixel.PixelName.FORGET_ALL_AUTO_RESTART)
}

@Test
fun whenAllUnsentPixelsAreFiredThenResetCounter() {
val intent = givenEmptyIntent()
testee.registerIntent(intent)
testee.incrementCount()

testee.firePendingPixels()
testee.firePendingPixels()

verify(pixel).fire(Pixel.PixelName.FORGET_ALL_AUTO_RESTART)
}

@Test
fun whenAppRestartedAfterGoingBackFromBackgroundThenPixelIsSent() {
val intent = SystemSearchActivity.fromWidget(context)
testee.registerIntent(intent)
testee.onAppBackgrounded()
testee.incrementCount()

testee.firePendingPixels()

verify(pixel).fire(Pixel.PixelName.FORGET_ALL_AUTO_RESTART)
}

private fun givenEmptyIntent(): Intent = Intent(context, BrowserActivity::class.java)

private fun givenIntentWithData(url: String) = Intent(Intent.ACTION_VIEW).apply {
data = Uri.parse(url)
}
}
11 changes: 7 additions & 4 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import com.duckduckgo.app.browser.rating.ui.GiveFeedbackDialogFragment
import com.duckduckgo.app.browser.rating.ui.RateAppDialogFragment
import com.duckduckgo.app.feedback.ui.common.FeedbackActivity
import com.duckduckgo.app.fire.DataClearer
import com.duckduckgo.app.fire.DataClearerForegroundAppRestartPixel
import com.duckduckgo.app.global.ApplicationClearDataState
import com.duckduckgo.app.global.DuckDuckGoActivity
import com.duckduckgo.app.global.intentText
Expand Down Expand Up @@ -64,6 +65,9 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope by MainScope() {
@Inject
lateinit var playStoreUtils: PlayStoreUtils

@Inject
lateinit var dataClearerForegroundAppRestartPixel: DataClearerForegroundAppRestartPixel

private var currentTab: BrowserTabFragment? = null

private val viewModel: BrowserViewModel by bindViewModel()
Expand All @@ -81,11 +85,9 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope by MainScope() {
@SuppressLint("MissingSuperCall")
override fun onCreate(savedInstanceState: Bundle?) {
super.daggerInject()

renderer = BrowserStateRenderer()

Timber.i("onCreate called. freshAppLaunch: ${dataClearer.isFreshAppLaunch}, savedInstanceState: $savedInstanceState")

dataClearerForegroundAppRestartPixel.registerIntent(intent)
renderer = BrowserStateRenderer()
val newInstanceState = if (dataClearer.isFreshAppLaunch) null else savedInstanceState
instanceStateBundles = CombinedInstanceState(originalInstanceState = savedInstanceState, newInstanceState = newInstanceState)

Expand All @@ -110,6 +112,7 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope by MainScope() {
override fun onNewIntent(intent: Intent?) {
super.onNewIntent(intent)
Timber.i("onNewIntent: $intent")
dataClearerForegroundAppRestartPixel.registerIntent(intent)

if (dataClearer.dataClearerState.value == ApplicationClearDataState.FINISHED) {
Timber.i("Automatic data clearer has finished, so processing intent now")
Expand Down
5 changes: 3 additions & 2 deletions app/src/main/java/com/duckduckgo/app/di/PrivacyModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ class PrivacyModule {
workManager: WorkManager,
settingsDataStore: SettingsDataStore,
clearDataAction: ClearDataAction,
dataClearerTimeKeeper: BackgroundTimeKeeper
dataClearerTimeKeeper: BackgroundTimeKeeper,
dataClearerForegroundAppRestartPixel: DataClearerForegroundAppRestartPixel
): DataClearer {
return AutomaticDataClearer(workManager, settingsDataStore, clearDataAction, dataClearerTimeKeeper)
return AutomaticDataClearer(workManager, settingsDataStore, clearDataAction, dataClearerTimeKeeper, dataClearerForegroundAppRestartPixel)
}

@Provides
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class AutomaticDataClearer(
private val workManager: WorkManager,
private val settingsDataStore: SettingsDataStore,
private val clearDataAction: ClearDataAction,
private val dataClearerTimeKeeper: BackgroundTimeKeeper
private val dataClearerTimeKeeper: BackgroundTimeKeeper,
private val dataClearerForegroundAppRestartPixel: DataClearerForegroundAppRestartPixel
) : DataClearer, LifecycleObserver, CoroutineScope {

private val clearJob: Job = Job()
Expand Down Expand Up @@ -159,7 +160,7 @@ class AutomaticDataClearer(
Timber.i("All data now cleared, will restart process? $processNeedsRestarted")
if (processNeedsRestarted) {
clearDataAction.setAppUsedSinceLastClearFlag(false)

dataClearerForegroundAppRestartPixel.incrementCount()
// need a moment to draw background color (reduces flickering UX)
Handler().postDelayed(100) {
Timber.i("Will now restart process")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright (c) 2020 DuckDuckGo
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.duckduckgo.app.fire

import android.content.Context
import android.content.Intent
import android.content.SharedPreferences
import androidx.annotation.UiThread
import androidx.annotation.VisibleForTesting
import androidx.core.content.edit
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleObserver
import androidx.lifecycle.OnLifecycleEvent
import com.duckduckgo.app.global.intentText
import com.duckduckgo.app.statistics.pixels.Pixel
import com.duckduckgo.app.systemsearch.SystemSearchActivity
import timber.log.Timber
import javax.inject.Inject
import javax.inject.Singleton

/**
* Stores information about unsent automatic data clearer restart Pixels, detecting if user started the app from an external Intent.
* Contains logic to send unsent pixels.
*
* When writing values here to SharedPreferences, it is crucial to use `commit = true`. As otherwise the change can be lost in the process restart.
*/
@Singleton
class DataClearerForegroundAppRestartPixel @Inject constructor(
private val context: Context,
private val pixel: Pixel
) : LifecycleObserver {
private var detectedUserIntent: Boolean = false

private val pendingAppForegroundRestart: Int
get() = preferences.getInt(KEY_UNSENT_CLEAR_APP_RESTARTED_PIXELS, 0)

private val pendingAppForegroundRestartWithIntent: Int
get() = preferences.getInt(KEY_UNSENT_CLEAR_APP_RESTARTED_WITH_INTENT_PIXELS, 0)

@UiThread
@OnLifecycleEvent(Lifecycle.Event.ON_STOP)
fun onAppBackgrounded() {
Timber.i("Registered App on_stop")
detectedUserIntent = false
}

fun registerIntent(intent: Intent?) {
Copy link
Member

@CDRussell CDRussell Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we find a better name for this? it's not clear from the name what it does or where/when to call it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could come up with a better name, like I previously expected. No problem since this will be removed once we work on the fix at some point.

detectedUserIntent = widgetActivity(intent) || !intent?.intentText.isNullOrEmpty()
}

fun incrementCount() {
if (detectedUserIntent) {
Timber.i("Registered restart with intent")
incrementCount(pendingAppForegroundRestart, KEY_UNSENT_CLEAR_APP_RESTARTED_WITH_INTENT_PIXELS)
} else {
Timber.i("Registered restart without intent")
incrementCount(pendingAppForegroundRestartWithIntent, KEY_UNSENT_CLEAR_APP_RESTARTED_PIXELS)
}
}

fun firePendingPixels() {
firePendingPixels(pendingAppForegroundRestart, Pixel.PixelName.FORGET_ALL_AUTO_RESTART)
firePendingPixels(pendingAppForegroundRestartWithIntent, Pixel.PixelName.FORGET_ALL_AUTO_RESTART_WITH_INTENT)
resetCount()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called from the Application class and there is no crash handling. Specially because resetCount() uses preferences who might not have been initialized properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this offline, and we think it's fine to keep it as it's right now.

}

private fun incrementCount(counter: Int, sharedPrefKey: String) {
val updated = counter + 1
preferences.edit(commit = true) {
putInt(sharedPrefKey, updated)
}
}

private fun firePendingPixels(counter: Int, pixelName: Pixel.PixelName) {
if (counter > 0) {
for (i in 1..counter) {
Timber.i("Fired pixel: ${pixelName.pixelName}/$counter")
pixel.fire(pixelName)
}
}
}

private fun resetCount() {
preferences.edit(commit = true) {
putInt(KEY_UNSENT_CLEAR_APP_RESTARTED_PIXELS, 0)
putInt(KEY_UNSENT_CLEAR_APP_RESTARTED_WITH_INTENT_PIXELS, 0)
}
Timber.i("counter reset")
}

private fun widgetActivity(intent: Intent?): Boolean =
intent?.component?.className?.contains(SystemSearchActivity::class.java.canonicalName.orEmpty()) == true

private val preferences: SharedPreferences by lazy {
context.getSharedPreferences(FILENAME, Context.MODE_PRIVATE)
}

companion object {
@VisibleForTesting
const val FILENAME = "com.duckduckgo.app.fire.unsentpixels.settings"
const val KEY_UNSENT_CLEAR_APP_RESTARTED_PIXELS = "KEY_UNSENT_CLEAR_APP_RESTARTED_PIXELS"
const val KEY_UNSENT_CLEAR_APP_RESTARTED_WITH_INTENT_PIXELS = "KEY_UNSENT_CLEAR_APP_RESTARTED_WITH_INTENT_PIXELS"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.duckduckgo.app.di.DaggerAppComponent
import com.duckduckgo.app.fire.DataClearer
import com.duckduckgo.app.fire.FireActivity
import com.duckduckgo.app.fire.UnsentForgetAllPixelStore
import com.duckduckgo.app.fire.DataClearerForegroundAppRestartPixel
import com.duckduckgo.app.global.Theming.initializeTheme
import com.duckduckgo.app.global.initialization.AppDataLoader
import com.duckduckgo.app.global.install.AppInstallStore
Expand Down Expand Up @@ -110,6 +111,9 @@ open class DuckDuckGoApplication : HasAndroidInjector, Application(), LifecycleO
@Inject
lateinit var unsentForgetAllPixelStore: UnsentForgetAllPixelStore

@Inject
lateinit var dataClearerForegroundAppRestartPixel: DataClearerForegroundAppRestartPixel

@Inject
lateinit var offlinePixelScheduler: OfflinePixelScheduler

Expand Down Expand Up @@ -170,6 +174,7 @@ open class DuckDuckGoApplication : HasAndroidInjector, Application(), LifecycleO
it.addObserver(appDaysUsedRecorder)
it.addObserver(defaultBrowserObserver)
it.addObserver(appEnjoymentLifecycleObserver)
it.addObserver(dataClearerForegroundAppRestartPixel)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use apply instance of also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, they are equivalent, but no strong preference on styles here. So for now, it's fine to keep it as it is now and focus in related changes to our initial goal. But that was a nice catch. 👍


if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N_MR1) {
Expand Down Expand Up @@ -254,6 +259,8 @@ open class DuckDuckGoApplication : HasAndroidInjector, Application(), LifecycleO
}
unsentForgetAllPixelStore.resetCount()
}

dataClearerForegroundAppRestartPixel.firePendingPixels()
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ interface Pixel {
FORGET_ALL_PRESSED_BROWSING("mf_bp"),
FORGET_ALL_PRESSED_TABSWITCHING("mf_tp"),
FORGET_ALL_EXECUTED("mf"),
FORGET_ALL_AUTO_RESTART("m_f_r"),
FORGET_ALL_AUTO_RESTART_WITH_INTENT("m_f_ri"),

APPLICATION_CRASH("m_d_ac"),
APPLICATION_CRASH_GLOBAL("m_d_ac_g"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import com.duckduckgo.app.browser.BrowserActivity
import com.duckduckgo.app.browser.R
import com.duckduckgo.app.browser.autocomplete.BrowserAutoCompleteSuggestionsAdapter
import com.duckduckgo.app.browser.omnibar.OmnibarScrolling
import com.duckduckgo.app.fire.DataClearerForegroundAppRestartPixel
import com.duckduckgo.app.global.DuckDuckGoActivity
import com.duckduckgo.app.global.view.TextChangedWatcher
import com.duckduckgo.app.global.view.hideKeyboard
Expand All @@ -53,6 +54,9 @@ class SystemSearchActivity : DuckDuckGoActivity() {
@Inject
lateinit var omnibarScrolling: OmnibarScrolling

@Inject
lateinit var dataClearerForegroundAppRestartPixel: DataClearerForegroundAppRestartPixel

private val viewModel: SystemSearchViewModel by bindViewModel()
private lateinit var autocompleteSuggestionsAdapter: BrowserAutoCompleteSuggestionsAdapter
private lateinit var deviceAppSuggestionsAdapter: DeviceAppSuggestionsAdapter
Expand All @@ -66,6 +70,7 @@ class SystemSearchActivity : DuckDuckGoActivity() {

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
dataClearerForegroundAppRestartPixel.registerIntent(intent)
setContentView(R.layout.activity_system_search)
configureObservers()
configureOnboarding()
Expand All @@ -82,6 +87,7 @@ class SystemSearchActivity : DuckDuckGoActivity() {

override fun onNewIntent(newIntent: Intent?) {
super.onNewIntent(newIntent)
dataClearerForegroundAppRestartPixel.registerIntent(newIntent)
viewModel.resetViewState()
newIntent?.let { sendLaunchPixels(it) }
}
Expand Down