ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.32k stars 4.17k forks source link

Binding in UICollectionViewDiffableDataSource #2511

Open XITRIX opened 1 year ago

XITRIX commented 1 year ago

Short description of the issue:

MainScheduler's DispatchQueue.isMain contains false on binding to BehaviorRelay in context of UICollectionViewDiffableDataSource's cellForRow function, which leads to UI blink with old data for a moment

Expected outcome:

Binding ViewModel's BehaviorRelay to Cell's view should update it's UI instantly

What actually happens:

Cells are able to appear on screen with old data before value of BehaviorRelay applies to Cell's views

Self contained code example that reproduces the issue:

Link to repo with issue

Breakpoint should be in Reactive.swift -> 42: base[keyPath: keyPath] = value, It will show the main problem of this issue

All code needed presents in ViewController and TestCell files. Everything else in this repo are default iOS app template from xCode

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

RxSwift 6.5.0

Platform/Environment

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

Xcode version:

  14.3

:warning: Fields below are optional for general issues or in case those questions aren't related to your issue, but filling them out will increase the chances of getting your issue resolved. :warning:

Installation method:

I have multiple versions of Xcode installed: (so we can know if this is a potential cause of your issue)

Level of RxSwift knowledge: (this is so we can understand your level of knowledge and formulate the response in an appropriate manner)

Some more details:

UICollectionViewDiffableDataSource's cellForRow function calls on queue named "Thread 1 Queue : com.apple.uikit.datasource.diffing (serial)", so it's not a 'main' queue, but it calls on Thread 1 which in fact IS the main thread

As experiment, I've added to MainScheduler->scheduleInternal's function additional condition - (Thread.current.isMainThread || DispatchQueue.isMain) and it fixed my problem with diffableDataSource but I'm afraid it could damage anything else, so I'd like to find better solution to that problem

danielt1263 commented 1 year ago

Sorry, but the code posted is not self-contained. Please update the code to a self contained example that exhibits the problem?

XITRIX commented 1 year ago

Ok, I've replaced self contained code with repo, cause I have no idea how to fit everything there

danielt1263 commented 1 year ago

The issue with this code is here with the ViewModel class. You made it Hashable. It doesn't make sense to put a BehaviorRelay in a Hashable type. The diffable data source has no notion of the view model getting updated so it doesn't allow the cell to change.

When the text in a ViewModel needs to change, the proper way to communicate that is by updating the data BehaviorRelay.

XITRIX commented 1 year ago

It has to be Hashable to use it with DiffableDataSource whom is responsible to create cell's view and animate it's insert\delete\move, and BehaviorRelay is responsible for cell's content.

Also, it's just an example, I uses more complex ViewModels than just text in it. Here is the place I've noticed this issue in the first place

My main problem is that UIKit is able to run it's UI stuff on main thread, but not on main queue, and because of it MainScheduler's DispatchQueue.isMain check fails which could cause UI glitches because of View's update delay

danielt1263 commented 1 year ago

I understand it has to be Hashable, but you cannot put a BehaviorRelay in a Hashable type. It doesn't make any sense. Additionally, you are not creating your Hashable type correctly in the link you posted. The type has to compare all the internal elements.

Understand, that the way the diffable data source works is that it looks at the values of the elements and compares them to the previous values. If the values change when the data source isn't looking (for example because they are bound to an Observable) then the data source will not acknowledge those changes and will not update the collection view.

I'm not seeing any problems with isMain failing in the code.

XITRIX commented 1 year ago

I don't want allow DiffableDataSource to compare model's values cause I don't want it to be responsible for updating cell's content (I want to put this responsibility onto RX), I want it to be responsible only for cell's position in collection, it's insert\delete\move animation, that's why I do not include value of BehaviorRelay info hasher.

Also, my question is not about "how to use DiffableDataSource" my question is "UIKit is able to run it's UI stuff on main thread, but not on main queue, and because of it MainScheduler's DispatchQueue.isMain check fails which could cause UI glitches because of View's update delay" and this DiffableDataSource case is just an example of that problem.

Please, don't take it as an attack, I just think we are discussing a wrong thing. If you think it's important to discuss so be it.

danielt1263 commented 1 year ago

I have not seen any issues with DispatchQueue.isMain in the code you have provided.

XITRIX commented 1 year ago

If you'll check the screen at the moment breakpoint triggers you'll see that collection presents cell's with it's default values.

Because DispatchQueue.isMain says false scheduleInternal delays calling action(state) with self.mainQueue.async

danielt1263 commented 1 year ago

Where are you putting the breakpoint?

XITRIX commented 1 year ago

I thought I included it in repo, it should be in Reactive.swift -> 42: base[keyPath: keyPath] = value

XITRIX commented 1 year ago

It will be called with delay by self.mainQueue.async cause MainScheduler's 62: DispatchQueue.isMain returned false, while in fact, it was called by UIKit on main thread (but not main queue)

Replacing DispatchQueue.isMain with Thread.current.isMainThread fixed this "old data UI blink" but I'm afraid it could cause problems with something else (I'm not sure that mainThread is always UI thread), so I don't think it is a solution

danielt1263 commented 1 year ago

I see it now. It feels like the "DispatchQueue+Extensions.swift" file needs to be changed to:

extension DispatchQueue {
    static var isMain: Bool {
        Thread.current.isMainThread
    }
}

But I'm not sure why that extension was made the way it way in the first place. @kzaher or @freak4pc might have some input here.

XITRIX commented 1 year ago

I'm not sure that fix should be like that, cause this extension is about queue, and it is correct that the queue is not main.

As I know, in some cases it is possible that mainThread could be not UI thread, and as we can see here, UI thread could be not main queue also, so we cannot trust neither mainThread nor the mainQueue