DeclarativeHub / Bond

A Swift binding framework
MIT License
4.23k stars 359 forks source link

Best strategy to update a UITableView #150

Closed DenisLaboureyras closed 8 years ago

DenisLaboureyras commented 9 years ago

First I'm using Bond a lot in my app as MVVM, it's quite a great framework, I love it ! I had a hard time to migrate to the V4 version, but I'm glad I did it :).

That said, I can't figure out which is the best strategy to update a UITableView (or UICollectionView) using Bond V4, and I haven't find any demos or documentation to help me My issue is that I'm fetching data from a caching system (Realm Database) and from a Server API, using asynchronous tasks, and need to update my table view with insert/update/delete operations.

The 2 strategies I figured out are:

As obviously I don't like my app crashing, I'm using the first strategy, but I don't know if a better solution exists or not, or if there is a bug on the performbatch function. I would be glad to have your insights on this matter.

Regards, Denis

iamtomcat commented 9 years ago

I think there are still some bugs in this area. I was able to crash my app pretty consistently a while ago just by using batch updates and removing all items. To get around this atm I just do a removeall and add the items back in as needed. Obviously not very efficient, but I haven't had a chance to look at the code for this area to see what the issue is.

srdanrasic commented 9 years ago

@DenisLaboureyras , @iamtomcat, thanks for your feedback. Would you be able to give me an example of batch operations combinations that crash the app?

DenisLaboureyras commented 9 years ago

Thanks for your answer! Here is a sample of my code.

The function called when new data (array of members in this case) arrived:

func mergeMembers(newMembers: [SRMembers])
{

    self.membersViewModel.performBatchUpdates { membersViewModel in

        //update existing viewmodels replacing member inside the viewmodel
        for (index, _) in membersViewModel.array.enumerate() {
            if(index < newMembers.count){
                membersViewModel[index].changeMember(newMembers[index])
            }

        }

        //delete old viewmodels if needed
        if(membersViewModel.count > newMembers.count){
            membersViewModel.removeRange(newMembers.count..<membersViewModel.count)
        }

        //append new viewmodels if needed by creating them
        if(membersViewModel.count < newMembers.count){
            let membersToExtend = newMembers[membersViewModel.count..<newMembers.count].map {SRMemberCellViewModel(member: $0)}
            membersViewModel.extend(membersToExtend)
        }

    }

}

Then a (simplified) version of the binding function to the collection view:

self.viewModel.membersViewModel.lift().bindTo(collectionView, proxyDataSource: self) {[weak self] indexPath, dataSource, collectionView in

    let memberViewModel = dataSource[indexPath.section][indexPath.row]
    let cell = (collectionView.dequeueReusableCellWithReuseIdentifier("MemberCell", forIndexPath:indexPath) as SRMemberCell

    memberViewModel.name
        .bindTo(cell.memberName.bnd_text)
        .disposeIn(cell.bnd_bag)

    return cell

}.disposeIn(bnd_bag)

I tried also more simple versions of the merge function, but it produced the same result. Now I'm simply using this syntax to update the array, the UI flash but it is working:

self.membersViewModel.array = newMembers.map {SRMemberCellViewModel(member: $0)}
srdanrasic commented 9 years ago

I'm not having any luck reproducing this... I've setup a simple table view controller and tried your algorithm:

import UIKit
import Bond

class ViewController: UITableViewController {

  let data = ObservableArray([0, 1, 2])

  override func viewDidLoad() {
    super.viewDidLoad()

    data.lift().bindTo(tableView) { (indexPath, array, tableView) -> UITableViewCell in
      let cell = tableView.dequeueReusableCellWithIdentifier("Cell")!
      let element = array[indexPath.section][indexPath.row]
      cell.textLabel?.text = "\(element)"
      return cell
    }
  }

  override func viewDidAppear(animated: Bool) {
    super.viewDidAppear(animated);

    data.performBatchUpdates { (array) -> () in
      array[0] = 100
      array[1] = 101
      array[2] = 102
    }
  }
}

Also tried the case when new elements are inserted:

    data.performBatchUpdates { (array) -> () in
      array[0] = 100
      array[1] = 101
      array[2] = 102
      array.extend([3, 4, 5])
    }

... the case when existing elements are removed:

    data.performBatchUpdates { (array) -> () in
      array[0] = 100
      array.removeRange(1..<3)
    }

... and the case when all elements are removed:

    data.performBatchUpdates { (array) -> () in
      array.removeRange(0..<3)
    }

Maybe I'm missing some case / combination of updates/inserts/deletes? Can you post the full message it prints - those mismatching number of elements?

DenisLaboureyras commented 9 years ago

Here is the fullstack trace. Basically the array is initialized with 0 elements, the cache system fills the array with 3 elements, then the server replaces these 3 values with 3 newer values. Another thing, the merging task is asynchronous, I dispatch it on the main queue. The problem doesn't occur every time, sometimes it works, sometimes not... And btw I blame more the UICollectionView than Bond on the issue :). I will try to provide you tomorrow or this WE a small app that reproduces the problem, it will be easier for you.

* Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid update: invalid number of items in section 0. The number of items contained in an existing section after the update (3) must be equal to the number of items contained in that section before the update (3), plus or minus the number of items inserted or deleted from that section (3 inserted, 0 deleted) and plus or minus the number of items moved into or out of that section (0 moved in, 0 moved out).' * First throw call stack: ( 0 CoreFoundation 0x000000010a453f65 exceptionPreprocess + 165 1 libobjc.A.dylib 0x000000010706fdeb objc_exception_throw + 48 2 CoreFoundation 0x000000010a453dca +[NSException raise:format:arguments:] + 106 3 Foundation 0x000000010927aae2 -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 198 4 UIKit 0x0000000108564c3b -[UICollectionView _endItemAnimationsWithInvalidationContext:tentativelyForReordering:] + 15247 5 UIKit 0x000000010856bc3e -[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:] + 415 6 UIKit 0x000000010856ba7c -[UICollectionView _performBatchUpdates:completion:invalidationContext:] + 74 7 UIKit 0x000000010856ba1f -[UICollectionView performBatchUpdates:completion:] + 53 8 Bond 0x00000001052684d5 _TFFC4BondP33_9102B403720A1FA7EF4238F1B745828427BNDCollectionViewDataSource24setupPerSectionObserversurFGS0_qFT_T_U_FGVS_20ObservableArrayEventGSaQT + 1093 9 Bond 0x0000000105268ceb _TTRGrXFo_oGV4Bond20ObservableArrayEventGSaqdTXFo_iGS0_GSaq__dT + 43 10 Bond 0x0000000105232afe _TFFeRq_4Bond17EventProducerType_S_S0_10observeNewuRq_S0Fq_FFqq_S0_9EventTypeT_PS_14DisposableType_U_FQQPS09EventTypeT + 158 11 Bond 0x000000010522dcec _TPATFFeRq_4Bond17EventProducerType_S_S0_10observeNewuRq_S0Fq_FFqq_S0_9EventTypeT_PS_14DisposableType_U_FQQPS09EventTypeT + 156 12 Bond 0x000000010522d7b8 _TTRGrXFo_iqdTXFo_iqiT + 24 13 Bond 0x000000010522d3e1 _TPA__TTRGrXFo_iqdTXFo_iqiT + 97 14 Bond 0x000000010522d794 _TTRGrXFo_iqiTXFo_iqdT + 36 15 Bond 0x000000010522b952 _TFC4Bond13EventProducerP33_E3AABF94B17DA8055EE44FE13449BAE812dispatchNexturfGS0_qFqT + 786 16 Bond 0x000000010522b063 _TFC4Bond13EventProducer4nexturfGS0_qFqT + 563 17 Bond 0x0000000105240f8e _TFC4Bond15ObservableArray19performBatchUpdatesurfGS0_q__FFGS0_qTT + 1102 18 MyApp 0x00000001034d4ef8 _TFFC11MyApp21SRMyMembersModel12mergeMembersFS0_FGSaV15SRSharedDataKit6SRUser_T_U_FTT + 120 19 Async 0x00000001052133e7 _TTRXFodTXFdCbdT + 39 20 libdispatch.dylib 0x000000010b33349b _dispatch_client_callout + 8 21 libdispatch.dylib 0x000000010b3155e7 _dispatch_block_invoke + 716 22 Async 0x0000000105213403 _TTRXFdCbdTXFodT + 19 23 Async 0x0000000105213431 _TTRXFodTXFo_iTiT + 17 24 Async 0x00000001052133b1 _TPATTRXFodTXFo_iTiT111 + 81 25 Async 0x0000000105213460 _TTRXFo_iTiTXFodT + 32 26 Async 0x00000001052133e7 _TTRXFodTXFdCbdT__ + 39 27 libdispatch.dylib 0x000000010b312ef9 _dispatch_call_block_and_release + 12 28 libdispatch.dylib 0x000000010b33349b _dispatch_client_callout + 8 29 libdispatch.dylib 0x000000010b31b34b _dispatch_main_queue_callback_4CF + 1738 30 CoreFoundation 0x000000010a3b43e9 CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE + 9 31 CoreFoundation 0x000000010a375939 __CFRunLoopRun + 2073 32 CoreFoundation 0x000000010a374e98 CFRunLoopRunSpecific + 488 33 GraphicsServices 0x000000010c401ad2 GSEventRunModal + 161 34 UIKit 0x0000000107d46676 UIApplicationMain + 171 35 MyApp 0x00000001038e8a7d main + 109 36 libdyld.dylib 0x000000010b36892d start + 1 ) libc++abi.dylib: terminating with uncaught exception of type NSException

ivanmoskalev commented 9 years ago

@DenisLaboureyras can you please show us the code which does the merging. As a wild guess: It seems that replacing happens in two discreet steps, and there is a race condition in which the deletion sometimes occurs after the insertion.

DenisLaboureyras commented 9 years ago

@ivanmoskalev the merge function is in my 2nd post (named mergeMembers). Maybe it's a race condition indeed, but don't know really why.

I've build a small app to reproduce the issue, but it's too simple and it works fine... I will try to add step by step the complexity of my real app to figure out where the problem can be. Keep you posted.

ivanmoskalev commented 9 years ago

@DenisLaboureyras you could try logging a message before each change is applied to the ViewModel. This can give you some insight into what operation / pattern of operations causes the crash :)

whateverx commented 9 years ago

@srdanrasic I can reproduce a crash (maybe it is the same as @DenisLaboureyras)

I set an ObservableArray with 200 strings Each 0.2 seconds I remove 5 objects in the performBatchUpdates It always (randomly) crash

     @IBOutlet weak var tableView: UITableView!

    var arraybind:ObservableArray<String>!

    var queue: dispatch_queue_t!
    var timer: dispatch_source_t!

    override func viewDidLoad() {

        super.viewDidLoad()

        var array:[String] = []
        for _ in 1..<200 {

            array.append(NSUUID().UUIDString)

        }

        arraybind = ObservableArray(array)
        arraybind.lift().bindTo(tableView) { indexPath, dataSource, tableView in
            let cell = tableView.dequeueReusableCellWithIdentifier("Cell", forIndexPath: indexPath)
            cell.textLabel?.text = dataSource[0][indexPath.row]
            return cell
        }

        queue = dispatch_queue_create("timer", nil);
        timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, queue);
        dispatch_source_set_timer(timer, DISPATCH_TIME_NOW, UInt64(0.2 * Double(NSEC_PER_SEC)), UInt64(0.2 * Double(NSEC_PER_SEC)));
        dispatch_source_set_event_handler(timer) {
            dispatch_async(dispatch_get_main_queue(), {

                self.arraybind.performBatchUpdates { myArray in

                    if myArray.count > 10 {

                        // create an array with unique random index
                        var arrayIndexToDelete:[Int] = []
                        while arrayIndexToDelete.count < 5 {
                            let randomIndex = (Int(arc4random_uniform(UInt32(myArray.count-5))))
                            if !arrayIndexToDelete.contains(randomIndex){
                                arrayIndexToDelete.append(randomIndex)
                            }

                        }

                        // delete
                        for randomIndex in arrayIndexToDelete {

                            myArray.removeAtIndex(randomIndex)

                        }

                    }

                }

            })
        }

        dispatch_resume(timer)

        arraybind.observe { event in

            switch event.operation {
            case .Insert(let elements, let fromIndex):
                print("Inserted \(elements) from index \(fromIndex)")
            case .Remove(let range):
                print("Removed elements in range \(range)")
            case .Update(let elements, let fromIndex):
                print("Updated \(elements) from index \(fromIndex)")
            case .Reset(let _):
                print("Array was reset ")
            case .Batch(let operations):
                print("Operations \(operations) were perform on the array")
            }
        }

    }

the error seems to happen in ObservableArrayEvent.swift

public func changeSetsFromBatchOperations<T>(operations: [ObservableArrayOperation<T>]) -> [ObservableArrayEventChangeSet] 

we got 5 operations (delete) and should return an array of type ObservableArrayEventChangeSet with 5 Deletes(Set), but sometimes it return only 4 Deletes. So the tableView.endUpdates() crash

seems to happen when we want to delete two ranges who are close

[Bond.ObservableArrayOperation<Swift.String>.Remove(Range(64..<65)), Bond.ObservableArrayOperation<Swift.String>.Remove(Range(63..<64)), Bond.ObservableArrayOperation<Swift.String>.Remove(Range(6..<7)), Bond.ObservableArrayOperation<Swift.String>.Remove(Range(17..<18)), Bond.ObservableArrayOperation<Swift.String>.Remove(Range(66..<67))]

[Bond.ObservableArrayOperation.Remove(Range(64..<65)) Bond.ObservableArrayOperation.Remove(Range(63..<64))

srdanrasic commented 9 years ago

Thank you @whateverx, I think I now have an idea what is going on wrong here. Insertions+Removes in performBatchUpdates are are not correctly translated to Table/Collection view 'change sets'. I'll have to revise the algorithm. Hope I'll be able to handle it this week.

DenisLaboureyras commented 9 years ago

Sorry I was a bit busy these days and had not much time to dig my issue. Anyway, I found the root of my problem (and the solution :)) and think my problem was a little bit different than @whateverx.

My problem occurred when I was changing view controllers quickly, and my assumption is that my collection view or cells were not deallocating correctly (haven't found why yet), but the view model was deallocated. So when I was going back to my page, the perform batch operations was asking to insert new elements to my collection view, assuming it had no elements in it, but my collection view had retained old elements from previous loading... I'm not 100% sure but think that was the reason

I manage to fix that by reseting the array when I initialize my view model instead of doing batch operations, and then performing batch operations when I update my view model. Hope it helps...

srdanrasic commented 8 years ago

@DenisLaboureyras and @whateverx, thank you for you feedbacks. Issues with performBatchUpdates should be fixed now.

@DenisLaboureyras, if you think that your second issue is also Bond related, please open a new ticket. Having dangling view controllers sounds very bad :)

Thanks!