Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding in UICollectionViewDiffableDataSource #2511

Open
5 of 17 tasks
XITRIX opened this issue Apr 18, 2023 · 13 comments
Open
5 of 17 tasks

Binding in UICollectionViewDiffableDataSource #2511

XITRIX opened this issue Apr 18, 2023 · 13 comments

Comments

@XITRIX
Copy link

XITRIX commented Apr 18, 2023

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

  • iOS
  • macOS
  • tvOS
  • watchOS
  • playgrounds

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

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

  14.3

⚠️ 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. ⚠️

Installation method:

  • CocoaPods
  • Carthage
  • Git submodules

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

  • yes (which ones)
  • no

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

  • just starting
  • I have a small code base
  • I have a significant code base

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
Copy link
Collaborator

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

@XITRIX
Copy link
Author

XITRIX commented Apr 18, 2023

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

@danielt1263
Copy link
Collaborator

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
Copy link
Author

XITRIX commented Apr 18, 2023

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
Copy link
Collaborator

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
Copy link
Author

XITRIX commented Apr 18, 2023

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
Copy link
Collaborator

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

@XITRIX
Copy link
Author

XITRIX commented Apr 18, 2023

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
Copy link
Collaborator

Where are you putting the breakpoint?

@XITRIX
Copy link
Author

XITRIX commented Apr 18, 2023

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

@XITRIX
Copy link
Author

XITRIX commented Apr 18, 2023

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
Copy link
Collaborator

danielt1263 commented Apr 18, 2023

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
Copy link
Author

XITRIX commented Apr 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants