Rewrite TestApp using SwiftUI

[stevenzeck]

This is a long-term issue. We will continually to make incremental changes to develop/main. No changes should be made that break UIKit functionality. It is ok to create additional functions that do the same thing as an existing one. For example, adding an async/await based function for SwiftUI in addition to the existing promise/completion based one that is used by UIKit.

General

• [x] Tab bar for navigating books, OPDS feeds, and about screens
• [ ] Localization
• [ ] Accessibility

Reader

• [ ] Open the reader from the Bookshelf
• [ ] Navigate back to the Bookshelf from the reader

Bookshelf

• [x] Show a grid list of books that are ready for reading
• [ ] Add a book by URL
• [ ] Add a book by selecting a file on device
• [ ] Remove one or multiple books

OPDS feeds

• [x] Show a list of OPDS feeds
• [x] Add a new feed
• [ ] Rename a feed
• [x] Remove a feed

OPDS Feed Detail

• [x] Show list of navigation links for a single feed
• [x] Show groups for the feed, with a grid view of publications in the group and a link to view all of the publications in that group. Also show Navigation links for that group
• [x] Show grid view of publications for the feed
• [x] Navigate to another feed detail
• [ ] Show Facet filters

Publication Detail

• [x] Show cover image, title, author, and description.
• [ ] Download publication onto the device.

About

• [x] Show About screen

[wisarmy]

No more updates for swiftui?

[mickael-menu]

New screens are built using SwiftUI (e.g. the pending Settings API PR), but converting existing screens is not high priority so progress is going slow. @gatamar and @stevenzeck have been contributing great PRs for this but I currently have my plate quite full and didn't have time to follow-up on this yet.

[stevenzeck]

@wisarmy Sorry, I've been busy with a new job and don't have time to continue with this right now. It was coming along though, OPDS is nearly finished, and adding books from a URL or on the device and removing them should be straightforward. I know @gatamar has a PR out for opening books.

[jpollard-cs]

Hey there just a heads up I'm going to try to take a stab at integrating the latest changes from main and will also try to get @gatamar's PR over the line. Just FYI in case anyone is also working on this branch ATM.

[gatamar]

Sorry guys, I'm unavailable as of now. Will be happy if my branch is useful in any way. As far as I remember there were 2 main challenges: SwiftUI navigation and rewriting some (Promise based?) code to async await. Regarding the first challenge, I'm still not sure if SwiftUI is mature enough to easily do what we wanted to achieve. Regarding the second challenge, sorry for the unfinished code - I know how to do it now, but have no time :)

On Mon, 22 May 2023 at 21:49, Jordan @.***> wrote:

Hey there just a heads up I'm going to try to take a stab at integrating the latest changes from main and will also try to get @gatamar https://github.com/gatamar's PR over the line. Just FYI in case anyone is also working on this branch ATM.

— Reply to this email directly, view it on GitHub https://github.com/readium/swift-toolkit/issues/60#issuecomment-1557862597, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOSN2O3LM4GCTXZF5RTQSDXHO7LHANCNFSM5YBIHSCQ . You are receiving this because you were mentioned.Message ID: @.***>

[mickael-menu]

@gatamar Thanks for summarizing the issues and taking a stab at it in the first place!

@jpollard-cs Sure, that would be welcome. But since it's complicated to get a full SwiftUI rewrite done in one go, I'm thinking we should change the strategy and do more incremental changes based on and synchronized with main. Here's a proposition:

1. Make the minimal changes in swiftui to be on par with main, feature-wise (doesn't need to be perfect regarding SwiftUI).
2. Merge swiftui into main.
3. Migrate existing Test App Promise/completion-block based APIs to async/await.
   • Better have plenty of small PRs for individual services/components instead of one huge PR changing this throughout the whole app.
4. Convert individual UIKit screens/views to SwiftUI, but keeping the navigation and containers as UIKit components
   • This should be split in at least one PR per modules: bookshelf, opds catalogs, reader. Could be split even further.
5. Investigate and convert the UIKit navigation to SwiftUI. Probably needs iOS 16+.
   • This part might need some refactoring in the services, especially related to opening a publication. See the discussions in @gatamar's PR.
6. Convert the rest of the app to be full SwiftUI-native (removing AppDelegate, etc.).

What do you think?

[mickael-menu]

I talked with @stevenzeck, here are some additional comments:

• For transitioning to async/await, it can be done incrementally, without deleting the existing promise/completion code. This will help avoid having to convert all the UIKit code to async/await.
• One big shortcoming of this strategy is that most of the UIKit views in the Test App are encoded in storyboards (forgot about this "detail"...). So we can't really convert inner "atomic" views to SwiftUI without touching the navigation/containers too.

My main concern is that keeping a swiftui branch separate without having someone committed to finalizing it and then merging it in main means that it will likely become obsolete, like we're seeing now. That's why I'm advocating for incremental changes based on main.

[domkm]

Apologies if this is the wrong place to ask, but is it possible to convert TestApp to async await with the current streamer.open API and strict concurrency checking? I'm having trouble doing so as Publication is not Sendable.

[mickael-menu]

You might be able to pass it around with an async wrapper if you create a wrapping class or struct with @unchecked Sendable. I didn't try it though.

struct PublicationWrapper: @unchecked Sendable {
    let publication: Publication
}

Be careful as Publication is not thread-safe, you should not call its methods concurrently.

[domkm]

Thanks. My question was unclear. I was referring to your comment on May 23 where you noted the goal to "Migrate existing Test App Promise/completion-block based APIs to async/await."

I'm not trying to share Publications across threads, just create one using structured concurrency. In other words, I want to be able to let pub = await streamer.open(...). I tried to do this using withCheckedContinuation around streamer.open() and it fails with strict concurrency checking.

[mickael-menu]

It should work if you use the technique I described in the previous comment, as long as you don't use any API on Publication concurrently.

[stevenzeck]

I created a PR as a soft restart of this effort: #460. It does not incorporate anything from #240, but that PR would be a great place to continue moving forward.

• To switch to the SwiftUI based UI, comment out @UIApplicationMain in AppDelegate.swift and uncomment out @main in TestApp.swift.
• The TestApp now targets iOS 16, so we hopefully shouldn't encounter many more compatibility issues.