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

Move from OrderedSet to IdentifiedArrayOf #1339

Open
LePips opened this issue Dec 5, 2024 · 5 comments
Open

Move from OrderedSet to IdentifiedArrayOf #1339

LePips opened this issue Dec 5, 2024 · 5 comments
Labels
developer Developer facing issues

Comments

@LePips
Copy link
Member

LePips commented Dec 5, 2024

I've long thought about this but think we're going to run into this issue soon, especially with item deletion and updating throughout the app. I've run into some implementation issues while working on static notification payloads, and this will help with that.

We are very susceptible to the following structure, where our items have a ton of optionally-typed properties. In some contexts some properties may be set or not but we actually care about id equivalence.

struct MyItem: Hashable {
    
    let id: String?
    let name: String?
    let otherValue: Int?
}

let foo = MyItem(id: "123", name: "foo", otherValue: 123)
let bar = MyItem(id: "123", name: nil, otherValue: nil)

// false, but we actually care about the IDs
print(foo.hashValue == bar.hashValue)

We should migrate to swift-identified-collections instead for things like PagingLibraryViewModel and other contexts where we currently use OrderedSet to represent BaseItemDto, BaseItemPerson, and other items that conform to Identifiable.

@LePips LePips added the developer Developer facing issues label Dec 5, 2024
@LePips
Copy link
Member Author

LePips commented Dec 7, 2024

Oof, this requires a bit of thinking for my CollectionHStack and CollectionVGrid packages to take Hashable or Identifiable elements...

@JPKribs
Copy link
Member

JPKribs commented Dec 10, 2024

@LePips I found a new one. Selecting See All from the ItemView for Cast & Crew results in:

Image

I tried some other similar locations like Similar Items / Recommendations, Search > See All, & the Home View See All. All of these work but give me:

Accessing StateObject's object without being installed on a View. This will create a new instance each time.

I did some digging and it looks like every on of these appropriately uses viewModel.elements which is formatted as IdentifiedArray<Int, Element>. Only Cast & Crew is just a pure [BaseItemPerson].

Image

@LePips
Copy link
Member Author

LePips commented Dec 10, 2024

Ah, good catch. Fixed in #1354.

For the runtime warning, that's always kind of been an issue when working with Stinsen. However since we'll be moving away from it I haven't thought about what it's warning about much but we shouldn't have to worry about it for much longer.

@JPKribs
Copy link
Member

JPKribs commented Dec 18, 2024

@LePips Do you think this is related: #1368? Otherwise, I can start looking at this on my end!

@LePips
Copy link
Member Author

LePips commented Dec 19, 2024

Sadly yes it's related but it should all be in my CollectionHStack project. The sample project that I have in the project works on iPadOS so I'm bugged that there's an issue here.

The fact that the data source is an IdentifiedArray shouldn't mean anything, but I've seen weirder things.

Edit: there is a bug in my project when the data updates and how things get laid out. Seems tricky.

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

No branches or pull requests

2 participants