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

Sessions table view model refactoring #703

Merged
merged 16 commits into from
Jul 6, 2023

Conversation

allenhumphreys
Copy link
Collaborator

@allenhumphreys allenhumphreys commented Jun 22, 2023

Simplify how the table data source gets it's data and receives updated results from storage and how that is then combined with filtering predicates.

A structural change that will help make it simpler to convert to diffable datasource and/or leverage SwiftUI or move to outline view, etc. It's just simpler.

  • Search coordinator should update filters granularly if a storage change causes options to change and it shouldn't re-apply the saved state
  • Search coordinator needs a way to apply the filter selection directly to the filters to support explore deep linking
  • App coordinator needs cleaned up and checked for correctness
  • WWDC "today" feature needs fixed
  • Clearing filters to select a session needs fixed
  • Realm schema migration block
  • The currently playing video needs to be inserted into filter results like it used to be

@allenhumphreys allenhumphreys force-pushed the ah/sessions-table-view-controller-refactor branch 2 times, most recently from 13f8114 to 4b5dca0 Compare June 30, 2023 20:47
@allenhumphreys allenhumphreys force-pushed the ah/sessions-table-view-controller-refactor branch from 9899bac to cee9fe2 Compare July 2, 2023 20:56
@allenhumphreys allenhumphreys marked this pull request as ready for review July 2, 2023 20:56

keywords.removeAll()
keywords.append(objectsIn: otherKeywords)
keywords.append(realm.object(ofType: Keyword.self, forPrimaryKey: newKeyword.name) ?? newKeyword)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer equivalent since it doesn't handle removals

other.focuses.forEach { newFocus in
let effectiveFocus: Focus
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing logic doesn't handle removals, should we fix that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(that's true for all 3 lists that get updated here)

@@ -14,6 +14,7 @@ let package = Package(
targets: ["ConfCore"])
],
dependencies: [
.package(url: "https://github.com/apple/swift-collections.git", from: "1.0.4"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in use anymore

Copy link
Owner

@insidegui insidegui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Haven't found any issues living on this build for a couple of days :)

@allenhumphreys allenhumphreys merged commit f816d00 into master Jul 6, 2023
@allenhumphreys allenhumphreys deleted the ah/sessions-table-view-controller-refactor branch July 6, 2023 13:49
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

Successfully merging this pull request may close these issues.

2 participants