-
Notifications
You must be signed in to change notification settings - Fork 30
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
IOS-578 Suppress Readium 2.5 PublicationServer deprecation warnings #1625
Conversation
The switch from `PublicationServer` to `GCDHTTPServer` in Readium 2.5.0 caused some regressions when browsing ePubs with VoiceOver: - rendering issues while browsing (swift L/R) when scroll mode is not set - no automatic scroll when continuous reading mode is enabled. This silences the many deprecation warnings that are now emitted while still using the old PublicationServer. It also fixes a few other Xcode 14.3 warnings.
@@ -84,7 +84,7 @@ final class NetworkQueue: NSObject { | |||
let methodString = method.rawValue | |||
let dateCreated = NSKeyedArchiver.archivedData(withRootObject: Date()) | |||
// left for backward compatibility | |||
let headerData = NSKeyedArchiver.archivedData(withRootObject: [:]) | |||
let headerData: Data? = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code in the history, since we are no longer sending header data as a parameter (line 78) this appears equivalent.
self.publicationServer = publicationServer | ||
|
||
init() { | ||
guard let server = (PublicationServerFactory() as PublicationServerMaking).make() else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the opposite of dependency injection, so it's a "bad practice" per se. I only did it to control the creation of the PublicationServer where it's actually needed. This also allows us to keep those workaround protocols private
. This code is hopefully temporary anyway (last famous words).
I can also move it out where it was if we want.
func removeAll() | ||
} | ||
|
||
extension PublicationServer: PublicationServing { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still emits a warning because PublicationServer
as a class is deprecated. I don't see a way to work around this, but if you do let me know.
extension PublicationServer: PublicationServing { | ||
@available(*, deprecated, message: "To suppress this warning, cast to PublicationServing protocol") | ||
func add(_ publication: Publication) throws { | ||
try add(publication, at: UUID().uuidString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the UUID string was a default param for the PublicationServer::add(_, at:)
method, but we can't have default params in protocols so I have to add it here explicitly.
What's this do?
The switch from
PublicationServer
toGCDHTTPServer
in Readium 2.5.0 caused some regressions when browsing ePubs with VoiceOver:This silences the many deprecation warnings that are now emitted while still using the old PublicationServer.
It also fixes a few other Xcode 14.3 warnings.
Why are we doing this? (w/ JIRA link if applicable)
https://jira.nypl.org/browse/IOS-578
How should this be tested? / Do these changes have associated tests?
You should not see any warnings, except for one in
LibraryService.swift
, which seems unavoidable.Dependencies for merging? Releasing to production?
n/a
Does this include changes that require a new SimplyE/Open eBooks build for QA?
no
Has the application documentation been updated for these changes?
yes
Did someone actually run this code to verify it works?
yes, @ettore