Set actual dataSource after diff applying

[iDmitriyy]

This is the second variant of https://github.com/RxSwiftCommunity/RxDataSources/pull/379 All tests succeeded, but I am still not fully sure it is correct. Seems Mikhail Markin and me having different aims.

The task I am trying to solve: I have a cell with a cart button. When the cart button is tapped, a stepper with +- buttons appears. When I tap these buttons, I need to update the model. But model updating leads to cell reload, and stepper disappears. This is a problem. The decision is to remove the model amount property from == and hash(into hasher:) functions. Thus the model is updated, but diff algorithm can't detect this fact, and finally, the row is not reloaded.

The same problem we have in a cell with UISwitch: when UISwitch changes value, the model is updated, which leads to cell reload, and UISwitch animation is interrupted. This effect is looking very ugly sometimes.

[iDmitriyy]

Maybe, I need to create another Type of Datasource. Unfortunately, I can't tag Mikhail Markin here :( @bigMOTOR

[bigMOTOR]

Hi @iDmitriyy Actually to solve your problem you don't need any modifications in RxDataSources at all. This library already can do it. It will be a little tricky but anyway. Let's take a look at the Switcher cell example which you mentioned.

1. Make your switcher cell view model a class to mutate.
2. Make the property that holds switcher state a var.
3. Mutate this property on switcher change before doing the actual job of DataSource mutation. The point here switcher cell view model isEqual will say that prev. and current models are equal and the cell won't be reloaded then your DataSource stays mutated and holds actual value.

Something like this:

final class SwitchCellViewModel {

  let title: String
  var isSwitchOn: Bool
  private let _onChangeHandler: (Bool)->Void

  init(title: String, isOn: Bool = false, onChangeHandler: @escaping (Bool)->Void) {
    self.title = title
    self.isSwitchOn = isOn
    self._onChangeHandler = onChangeHandler
  }

  func stateChangeHandler(_ state: Bool) -> Void {
    isSwitchOn = state
    _onChangeHandler(state)
  }
}

extension SwitchCellViewModel {
  var identity: Int {
    return title.hash
  }

  func isEqual(to: IdentifiableCellViewModel) -> Bool {
    guard let to = to as? SwitchCellViewModel else { return false }
    return isSwitchOn == to.isSwitchOn
  }
}

[iDmitriyy]

Thank you for the fast response @bigMOTOR

The reason is that we can not use a reference type. We use Unidirectional Data Flow with value types only. There is a chain of mappings in the app. Firstly, we get DTO from our backend. The full chain is: DTO -> Domain Model -> ViewModel(the same as Props / ViewState struct) -> RowItem (enum)

We mutate ViewModels, and sometimes models. Usage of classes leads to a shared mutable state, which we want to avoid.

The decision I described in the first post worked earlier. Now, if diff algorithm doesn't detect changes via ==, it doesn't update sections in the datasource instance. Also, dataSource setSections(newSections) is called in all cases except one where the differences array is empty. My suggestion is that one line of code in this PR leads to a bit more correct behavior, or at least doesn't do anything wrong.

If I'm not mistaken, both variants with value and reference types will become possible.

[mashe]

@iDmitriyy Hi there,

As I got, you are trying to deal with a cell being reloaded while you want to keep as it is, aren't you? As you mentioned before, the flow goes like Model -> ViewModel -> Cell Also DataSource deals with diffs, so there are two things here: identity and equality functions in the ViewModel The first one is for tracking cell movements; (I could get you wrong but using values like amount or switchState property in identity function might be wrong, because it will lead for cell reload in any case, Diff will act like it is a new ViewModel and will not compare with the old one) and the second one is for updates. When user interacts with the app and updates UI state (UISwitch as a sample), Cell interacts with ViewModel, ViewModel updates Model, what updates Sections and generates a new ViewModel, Diff finds two cells with the same identity and runs equality functions on the new ViewModel and the old one, most probably there will be a state variable (isOn for Switch) used in equality function, and these states in the new and the old ViewModels are not equal, what leads to the Cell reload. So it looks like: Cell(update UI by user) -> ViewModel(interaction) -> Model(update) -> New ViewModel(generated in sections) -> Diff -> NewViewModel -> New Cell To break this behaviour we need to make equal new and old ViewModels and to achieve that we need to save state within ViewModel. The easiest way to do that is to update ViewModel while interaction step. @bigMOTOR has provided a good example of this idea. I guess, It is ok to use Classes instead of Structs in some suitable cases (Class is still a good tool, that should be properly used), but if you have a strict requirement to avoid classes, I may propose you to use BehaviourSubject for storing the state within the ViewModel.

The decision I described in the first post worked earlier. Now, if diff algorithm doesn't detect changes via ==, it doesn't update sections in the datasource instance. Also, dataSource setSections(newSections) is called in all cases except one where the differences array is empty. My suggestion is that one line of code in this PR leads to a bit more correct behavior, or at least doesn't do anything wrong.

Well, it does a wrong thing: Setting new Sections without a TableView reload, leads to a state when sections are not equal to actual ViewModels assigned to Cells.

Please, let me know if you have any questions and your issue still persists.

[iDmitriyy]

Hello, @mashe Thanks a lot for detailed answer

We don't have view models for every cell, that are similar to ViewModel from MVVM. We have a ViewState, it is a struct. ViewState is made from Domain Models – also structs in most cases. Sometimes they are classes, but made with an "Immutable Object" pattern. And row items for RxDatasources are enum instances.

What I do:

public struct TitledBool: Hashable {
  public let title: String
  public let isOn: Bool   // not included to ==

  public func toggled() -> Self {
    Self(title: title, isOn: !isOn)
  }

  public func hash(into hasher: inout Hasher) {
    hasher.combine(title)
  }

  public static func == (lhs: Self, rhs: Self) -> Bool {
    lhs.title == rhs.title
  }
}

enum RowItem {
  case toggle(TitledBool)

  var identity: String {
    switch self {
     // -- Bool value also doesn't included to identity
      case toggle(let titledBool): return titledBool.title
    }
  }
}

"To break this behaviour we need to make equal new and old ViewModels" – TitledBool custom == operator does this.

For example, I tap uiSwitch, the value changes from false to true. TitledBool is also toggled from false to true. View recreates row items. The TitledBool's 'isOn' property is changed, but the diff algorithm can not understand it, and the cell is not reloaded. The problem comes when we scroll up and scroll down, the cell is shown again with a false value, because the datasource wasn't updated. dataSource.setSections(newSections) call after applying array of ChageSet resolve this. Finally, when we change UI, the cell is not reloaded, and the datasource contains actual data.

Seems that it is ok, as newSections is the actual data.

Just remember that the initial task is a cell with +- buttons for adding products to the cart. If the cell is reloading, these buttons disappear. So, we need to update data without cell reloading using value types.

This PR achieves this goal, all tests are passed, and everything visually works correctly. We get an alternative of using value types instead of classes.

I am not experienced enough in implementation details, and I want to understand if there are any arguments against such approach.

[mashe]

@iDmitriyy long story short, what you are proposing is conceptually wrong, that is breaking data-driven pattern. Let's imagine that model could be mutated from another source too (not a user interaction). And as I got you right, in a case of mutation coming from that source, your implementation will not update UI. Model creates ViewState, ViewStates are equal, newSections are set, but Cell is not updated with new ViewState.

[mashe]

@iDmitriyy here what you can use with value types:

import RxSwift
import RxCocoa

public struct TitledBool: Hashable {
  public let title: String
  public let onToggle: ((Bool) -> ())?
  public var isOn: Bool {
    _isOn.value
  }

  private let _isOn: BehaviorRelay<Bool>

  public init(title: String, isOn: Bool, onToggle: ((Bool) -> ())?) {
    self.title = title
    self._isOn = BehaviorRelay(value: isOn)
    self.onToggle = onToggle
  }

  public func toggle(_ newValue: Bool) {
    _isOn.accept(newValue)
    onToggle?(newValue)
  }

  public func hash(into hasher: inout Hasher) {
    hasher.combine(title)
  }

  public static func == (lhs: Self, rhs: Self) -> Bool {
    lhs.title == rhs.title
  }
}

enum RowItem {
  case toggle(TitledBool)

  var identity: String {
    switch self {
     // -- Bool value also doesn't included to identity
      case toggle(let titledBool): return titledBool.title
    }
  }
}

[mashe]

@iDmitriyy I am not a owner of this repo and I could be wrong too. Please, feel free to ask other community members opinion on this topic.