search

readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift
https://readium.org/mobile/
BSD 3-Clause "New" or "Revised" License
223 stars 96 forks source link

Rewrite TestApp using SwiftUI #60

Open stevenzeck opened 2 years ago

stevenzeck commented 2 years ago

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

Reader

Bookshelf

OPDS feeds

OPDS Feed Detail

Publication Detail

About

wisarmy commented 1 year ago

No more updates for swiftui?

mickael-menu commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 days ago

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.