SwiftUI: Open a book on tap #240
Conversation
There was a problem hiding this comment.
Thank you @gatamar, I left a few comments already but didn't have time to review everything yet.
| class BookOpener: ObservableObject, Loggable { | ||
| @Published var openedBookPublication: Publication? | ||
| @Published var openedBook: Book? | ||
| @Published var openedBookTag: Book.Id? | ||
| @Published var curOpeningBookId: Book.Id? | ||
|
|
||
| init(readerDependencies: ReaderDependencies) { |
There was a problem hiding this comment.
Using an actor for BookOpener will make sure that any async function called on it will be run from a background thread and not block the UI. This will be useful when accessing the files on the file system for example.
The nonisolated keyword on init is to avoid needing to call await BookOpener() when creating the BookOpener.
| class BookOpener: ObservableObject, Loggable { | |
| @Published var openedBookPublication: Publication? | |
| @Published var openedBook: Book? | |
| @Published var openedBookTag: Book.Id? | |
| @Published var curOpeningBookId: Book.Id? | |
| init(readerDependencies: ReaderDependencies) { | |
| actor BookOpener: ObservableObject, Loggable { | |
| @Published var openedBookPublication: Publication? | |
| @Published var openedBook: Book? | |
| @Published var openedBookTag: Book.Id? | |
| @Published var curOpeningBookId: Book.Id? | |
| nonisolated init(readerDependencies: ReaderDependencies) { |
There was a problem hiding this comment.
Thanks, added the actor declaration. For the nonisolated, it seems it's already deprecated :)
There was a problem hiding this comment.
Ha interesting, and you don't need to spawn a Task to call the initializer without nonisolated?
| NavigationLink( | ||
| destination: readerView(), | ||
| tag: book.id!, | ||
| selection: self.$bookOpener.openedBookTag |
There was a problem hiding this comment.
The SwiftUI Navigation API pre iOS 16 is really not great :/ We need to create the destination view for all the covers and the bookshelf has to know how to create the reader views.
I suggest we migrate to the iOS 16 NavigationStack to move this responsibility outside of the Bookshelf view:
- http://swiftwithmajid.com/2022/06/15/mastering-navigationstack-in-swiftui-navigator-pattern/
- https://developer.apple.com/documentation/swiftui/migrating-to-new-navigation-types
Here we have a situation which is a bit special because we have an async task that is run before changing the stack, so I think that NavigationLink is not very convenient as it is synchronous. Instead we could use a simple button to start the task, and use the programmatic navigation using a @State variable as explained here:
https://developer.apple.com/documentation/swiftui/navigationlink#Control-a-presentation-link-programmatically
That might be a bit complicated and out of scope for this PR. Maybe for now you could just present the reader outside of the NavigationView when it's ready using .fullScreenCover()?
There was a problem hiding this comment.
@mickael-menu Thank you for the guidance!
Regarding iOS 16 API, I think NavigationPath fits for our purposes (it's closely related to the NavigationStack).
More details:
- https://medium.com/devtechie/new-in-swiftui-4-ios-16-navigationstack-e6ddb45d4798
- https://github.com/gualtierofrigerio/SwiftUI4
I've added a basic version (no tasks cancellation added yet), please check if this API looks good to you.
…ort sheet UI, as it's already added, there're just serious issues with navigation
This PR aims to open a book somehow.
Changes:
BookOpener,BookImporter,BookRemoverfrom theLibraryServiceReaderViewControllerContainer, as now it has more responsibilitiesOut of scope (coming soon in the next PRs):