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

CocoaList initialContentAlignment issue #32

Closed
MatteCarra opened this issue Mar 7, 2020 · 8 comments
Closed

CocoaList initialContentAlignment issue #32

MatteCarra opened this issue Mar 7, 2020 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@MatteCarra
Copy link
Contributor

MatteCarra commented Mar 7, 2020

I have noticed an issue with current CocoaList initialContentAlignment implementation.
The issue seems to be in UIHostingTableViewController.correctContentOffset function.
This function is called whenever content size is updated by the observe value function in the event a view is added to the data source.
The issue here is the oldContentSize value: observeValue seems to be called multiple times with values like:

  1. old: 642 new: 452 (discarded by correctContentOffset: more on that later)
  2. old: 452 new: 722 (accepted by correctContentOffset)

You seem to have noticed this in correctContentOffset:

guard oldContentSize.maximumDimensionLength < newContentSize.maximumDimensionLength else {
    return
}

You then calculate the offset like this:

if initialContentAlignment.vertical == .bottom {
      newContentOffset.y += newContentSize.height - oldContentSize.height
}

but the delta is way higher than it should be (722-452 instead of 722-642) resulting in a wrong content offset.

If you remove the said guard the delta would be correct, but the offset will be set even when tableView.frame.size.height < newContentSize.height causing the view to be scrolled to bottom even when the content is not filling the whole frame.
You can't even guard on that condition because the condition could match the second call from observeValue and not the first one.

I'm new to UIKit and I have no idea why it's behaving like that.
I'm looking forward to hear your opinion on that.
Thanks.

@MatteCarra
Copy link
Contributor Author

MatteCarra commented Mar 7, 2020

I have done more tests in a simpler project and was not able to reproduce the multiple calls to the observeValue.
I will perform more tests on the other project, but why was the condition oldContentSize.maximumDimensionLength < newContentSize.maximumDimensionLength imposed?

@MatteCarra
Copy link
Contributor Author

MatteCarra commented Mar 7, 2020

I was able to reproduce the issue in a demo project by adding a vertical padding:

import SwiftUI
import SwiftUIX

struct Item: Codable, Equatable, Identifiable {
    let id = UUID()
}

struct ContentView: View {
    @State var items: [Item] = []
    
    var body: some View {
        VStack {
            CocoaList(items) { item in
                Text("Hello, World \(item.id)")
                    .padding(.vertical, 30)
            }.initialContentAlignment(.bottom)
            
            Button(action: {
                self.items.append(Item())
            }) {
                Text("Add")
            }
        }
    }
}

You can use this code to reproduce the bug.

@vmanot
Copy link
Member

vmanot commented Mar 7, 2020

@MatteCarra I'm currently struggling to fix content offset correction myself - UITableView is just fucked in terms of how/when it updates contentSize. I'm thinking about ditching it altogether and switching to UICollectionView (a port that I'm working on).

@MatteCarra
Copy link
Contributor Author

Thanks for the confirmation.
I was not aware of this issue with UITableView before.
I'm looking forward to your UICollectionView port.
Thanks

@vmanot
Copy link
Member

vmanot commented Mar 8, 2020

@MatteCarra if you need this functionality ASAP - I'd recommend you try raising a workaround/patch PR (you seem to have understood the issue exactly). I'm available if you need any assistance in understanding the code - which I understand is under-documented currently.

@vmanot vmanot added the help wanted Extra attention is needed label Mar 8, 2020
@MatteCarra
Copy link
Contributor Author

I'm not in a rush, by I'm actively trying to find a solution to the problem.
I'm actually playing around with swift for the first time and I've on my back 3 days experience of practice and only a few hours on uikit.

I'm looking forward on collaborating to this great project.

@MatteCarra
Copy link
Contributor Author

I was able to perform some working tests with CocoaList, but I have some questions for the desired implementation.

What is initialContentAlignment designed to do?

I think that a common use case of initialContentAlignment could be for a logs/chat view: messages start filling from top until all the view is filled and then the list is automatically scrolled to the last element whenever a new item is added.

I would also need to know why you imposed the condition oldContentSize.maximumDimensionLength < newContentSize.maximumDimensionLength.

Please let me know.

@vmanot
Copy link
Member

vmanot commented Mar 15, 2020

@MatteCarra the purpose of initialContentAlignment is to describe the initial content offset for the scroll view (be it a basic scroll view, a table view or a collection view). This is useful when, yes, you're developing a chat application and the scroll view must be scrolled to the very bottom at the start.

The reason I'd imposed that condition is because UITableView has this weird behavior where when reloadData() is called after new items are added, the contentSize decreases and then increases. I fear this condition is more harmful than good, so feel free to remove it.

@vmanot vmanot closed this as completed Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants