Use external DittoPresenceViewerCore dependency #166
Conversation
bplattenburg
force-pushed
the
BP/extract_presence_viewer
branch
from
36e7fd5 to
8f85300
Compare
|
This will now depend on getditto/DittoPresenceViewerCore#5 and a matching new release of the new package to avoid undoing the work in #167. Marking as a draft for now. |
bplattenburg
changed the title
| // MARK: Constants | ||
|
|
||
| private struct LocalizedStrings { | ||
| static let title = NSLocalizedString("Ditto Presence", |
There was a problem hiding this comment.
We don't actually have any matching localized strings (or any at all in the package), and I was getting some inconsistent build errors from Xcode 16.2 around the Bundle+FrameworkBundle.swift code - removing this single usage allowed me to remove that extension as well.
There was a problem hiding this comment.
@zmarkan I was seeing the links from the other comment thread not working, this is the localization removal
|
This now depends on getditto/DittoPresenceViewerCore#7 |
|
And here is this used in the POS demo app, showcasing no breaking changes for consuming apps: getditto/demoapp-pos-kds#69 |
| @@ -128,7 +120,7 @@ final class DittoPresenceViewController: PlatformViewController { | |||
| #if canImport(UIKit) | |||
| guard navigationController == nil else { return } | |||
|
|
|||
| navigationItem.title = LocalizedStrings.title | |||
| navigationItem.title = "Ditto Presence" | |||
There was a problem hiding this comment.
See #166 - essentially we didn't actually do any localization and all the machinery to make it work was causing other build issues with the current Xcode version.
If we want to revisit localization later the tools need a much larger overhaul
| @@ -83,7 +75,7 @@ final class DittoPresenceViewController: PlatformViewController { | |||
| override func viewDidLoad() { | |||
| super.viewDidLoad() | |||
|
|
|||
| title = LocalizedStrings.title | |||
| title = "Ditto Presence" | |||
There was a problem hiding this comment.
See #166 - essentially we didn't actually do any localization and all the machinery to make it work was causing other build issues with the current Xcode version.
If we want to revisit localization later the tools need a much larger overhaul
|
I noticed that when building for Mac Catalyst, then Presence Viewer doesn't work as expected: When run as Mac (Designed for iPad), it works fine. Although, it's not a regression. Same behaviour on the prior version, with embedded js. Given there are (apparently) other issues when building for Mac Catalyst (I noticed creds don't persist in Keychain, either) — shall we consider this expected behaviour, so we can move forward, and fix Catalyst later? |
There was a problem hiding this comment.
This breaks compilation for tvOS, as there are numerous platform deltas.
My understanding is that the Presence Viewer (as it relies on js) is entirely incompatible on tvOS (it is excluded from the menu entirely), so I'll work out cleanly excluding all the relevant source from the platform (as opposed to resolving issues, or line-by-line workarounds)
That's right, presence viewer doesn't work today on tvOS, but we should still ensure that it compiles so customers can use the other tools. I'm hoping that's as simple as some |
I'd lean towards tracking the mac catalyst issues separately if the behavior is unchanged by this work - can you write up a summary in a new issue? |
|
Slightly more involved than !tvOS, but I'm working on a fix. Should be able to push tomorrow. I want to snip the tv condition as high up the food chain as possible. Got it nearly worked out. |
Updates to use the presence viewer from its own repo / package as part of the path to a single source of truth for the presence viewer's web code (JS & HTML). All the Swift code stays here.
Depends on getditto/DittoPresenceViewerCore#7
Closes CXTOOLS-358
Part of CXTOOLS-337