Respect unique only on configuration changes. #207

BenSchwab · 2019-03-11T01:28:13Z

closes #204

Currently there is a bug where uniqueOnly will redeliver the last value, even if unchanged, during an orientation change. The intent of uniqueOnly was to emit a value only once, ever, to allow for actions that need to be taken only once, such as showing a PopTart or logging. (Just for posterity sake I want to note I recommend logging in ViewModel, as subscriptions there have a much simpler lifecycle).

In order to dedup a repeated value after a configuration change, the view model maintains a map between "subscriptionIds" and the last delivered value.

When you choose to subscribe with uniqueOnly, you must provide a subscription id, so the view model can perform the dedupping logic. An extension function uniqueOnly() on MvRxView will handle creating and persisting a unique subscription id across configuration changes.

@gpeal @elihart

elihart

@BenSchwab this is great! nice solution to a tricky issue, didn't realize you were working on this, it was definitely something that needed solving.

general strategy lgtm and is pretty clever. I like that the enum approach allows us to expand delivery settings in the future.

just had a few questions about edge cases

elihart · 2019-03-11T15:41:54Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxFragment.kt

@@ -12,14 +13,20 @@ abstract class BaseMvRxFragment : Fragment(), MvRxView {

    override val mvrxViewModelStore by lazy { MvRxViewModelStore(viewModelStore) }

+    final override val mvrxViewId: String by lazy { mvrxPersistedViewId }


maybe worth noting that accessing this before onCreate is invalid and will crash?

elihart · 2019-03-11T15:45:54Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

+
+/**
+ * The subscription will receive all state updates, and may receive repeated state updates when transitioning
+ * from locked to unlocked states (stopped/destroyed -> started).


nit: generally I don't think lifecycles recover back to started once they are destroyed. not in fragments or activities at least - the fragment view can be destroyed and recreated, but that's slightly different

elihart · 2019-03-11T15:57:33Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxLifecycleAwareObserver.kt

+            val valueToDeliverOnUnlock = when {
+                deliveryMode is UniqueOnly -> lastUndeliveredValue
+                deliveryMode is Standard && lastUndeliveredValue != null -> lastUndeliveredValue
+                deliveryMode is Standard && lastUndeliveredValue == null -> lastValue


what about the case where the state property is nullable, and there is a valid value of null to deliver? Seems like we need to better differentiate between a value of null and not having a lastUndeliveredValue set

We always wrap properties in tuples, so they can never be null. I updated the generics to reflect this (RxJava doesn't allow null anyway)

ah, great. In that case it seems the deliveredFirstValue atomic boolean is unnecessary because it will always be true when lastDeliveredValueFromPriorObserver is non null (I think), so checking both seems redundant.

it is maybe more clear though

elihart · 2019-03-11T15:59:31Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxView.kt

@@ -37,101 +45,125 @@ interface MvRxView : MvRxViewModelStoreOwner, LifecycleOwner {
    /**
     * Subscribes to all state updates for the given viewModel.
     *
-     * @param uniqueOnly If true, when this MvRxView goes from a stopped to start lifecycle a state value
+     * @param deliveryMode If [UniqueOnly], when this MvRxView goes from a stopped to start lifecycle a state value


nit start should be started

elihart · 2019-03-11T16:05:31Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

@@ -35,6 +36,7 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
    private val tag by lazy { javaClass.simpleName }
    private val disposables = CompositeDisposable()
    private lateinit var mutableStateChecker: MutableStateChecker<S>
+    private val lastDeliveredStates = ConcurrentHashMap<String, Any>()


I don't think there is any check that the same id isn't used twice in separate subscriptions. The potential bug where different subscriptions with the same properties leads to collision worries me, especially since it doesn't seem like it would be an obvious issue.

Could we store a map of Id to Disposable here, and it is only valid to register the same id if the disposable is disposed?

Good idea! I'll add this, but this makes it very important for us to fix the duplicate subscriptions if you subscribe outside of onCreate.

I have a fix in progress for that.

gpeal

This is really awesome. I'm impressed with the solution you've turned around here 😄 I had a minor comment and LGTM once Eli is on board too!

gpeal · 2019-03-12T04:48:20Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxView.kt

+     * A globally unique id for this MvRxView. If your MvRxView is being recreated due to a lifecycle event (e.g. rotation)
+     * you should assign a consistent id. Likely this means you should save the id in onSaveInstance state.
+     * The viewId will not be accessed until a subscribe method is called. Accessing mvrxViewId before calling
+     * super.onCreate() will cause a crash.


You might want to give an example implementation of this for people who are only looking at this and are implementing MvRxView themself.

gpeal · 2019-03-12T04:51:48Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

+
+    internal fun appendPropertiesToId(vararg properties: KProperty1<*, *>) : DeliveryMode {
+        return when (this) {
+            is Standard -> Standard


How about something more explicit like RedeliverOnStart?

elihart

@BenSchwab looks great! thanks for this

elihart · 2019-03-12T14:50:01Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

+            lastDeliveredValue = if (deliveryMode is UniqueOnly) {
+                if (activeSubscriptions.contains(deliveryMode.subscriptionId)) {
+                    throw IllegalStateException("Subscribing with a duplicate subscription id: ${deliveryMode.subscriptionId}. " +
+                        "If you have multiple unique only subscriptions in a MvRx view that listen to the same")


looks like this comment wasn't finished

elihart · 2019-03-12T14:51:20Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

+                        "If you have multiple unique only subscriptions in a MvRx view that listen to the same")
+                }
+                activeSubscriptions.add(deliveryMode.subscriptionId)
+                lastDeliveredStates[deliveryMode.subscriptionId] as? T


can we remove the value from the map lastDeliveredStates too, or does it need to be stored for later?

Hmm, I guess we need to keep in the edge case that the state doesn't change across two configuration changes.

elihart · 2019-03-12T14:51:36Z

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

+            },
+            onDispose = {
+                if (deliveryMode is UniqueOnly) {
+                    activeSubscriptions.remove(deliveryMode.subscriptionId)


gpeal

LGTM once comments are addressed :)

Respect unique only on configuration changes

a2a3b25

elihart reviewed Mar 11, 2019

View reviewed changes

BenSchwab added 2 commits March 11, 2019 10:51

Fix test

493b34e

Throw exception on duplicate unique subscriber

3d3dfc6

gpeal reviewed Mar 12, 2019

View reviewed changes

elihart approved these changes Mar 12, 2019

View reviewed changes

gpeal approved these changes Mar 12, 2019

View reviewed changes

Code review

7d34874

BenSchwab merged commit 883437d into master Mar 12, 2019

BenSchwab deleted the bschwab--unique-id branch September 4, 2019 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect unique only on configuration changes. #207

Respect unique only on configuration changes. #207

BenSchwab commented Mar 11, 2019

elihart left a comment

elihart Mar 11, 2019

elihart Mar 11, 2019

elihart Mar 11, 2019

BenSchwab Mar 11, 2019

elihart Mar 11, 2019

elihart Mar 11, 2019

elihart Mar 11, 2019

BenSchwab Mar 11, 2019

gpeal left a comment

gpeal Mar 12, 2019

gpeal Mar 12, 2019

elihart left a comment

elihart Mar 12, 2019

elihart Mar 12, 2019

BenSchwab Mar 12, 2019 •

edited

Loading

elihart Mar 12, 2019

gpeal left a comment

		@@ -12,14 +13,20 @@ abstract class BaseMvRxFragment : Fragment(), MvRxView {

		override val mvrxViewModelStore by lazy { MvRxViewModelStore(viewModelStore) }

		final override val mvrxViewId: String by lazy { mvrxPersistedViewId }

Respect unique only on configuration changes. #207

Respect unique only on configuration changes. #207

Conversation

BenSchwab commented Mar 11, 2019

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

BenSchwab Mar 12, 2019 •

edited

Loading