Refactor and Enhance Identity Configuration Flow #159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Resolved crash on tvOS hardware - DittoManager initializes with platform-specific persistenceDirectory - Refactored Login → ConfigurationView - Updated UI for ConfigurationView to enable easier pasting of credentials
# Conflicts: # DittoToolsApp/DittoToolsApp/Views/MenuListItem.swift
Sources/DittoAllToolsMenu/Views/Tool Container Views/LoggingDetailsViewer.swift
Show resolved
Hide resolved
| var authProvider: String = "" | ||
| var authToken: String = "" | ||
| var customAuthURL: String = "" | ||
| var siteID: UInt64 = 0 |
There was a problem hiding this comment.
If possible, I'd like to intentionally leave siteID out of all our demo apps & tools, as we would rarely if ever recommend customer set it themselves, and it has been mostly replaced by peerKey
There was a problem hiding this comment.
In this case, I could see us approaching handling this 2 ways:
- As it's an optional param in the DittoIdentity constructor, we could simply omit it.
- The DittoIdentity object could be updated to deprecate, or remove it (eg. in 4.9.1+) and DittoTools could reflect that deprecation/removal.
I'd advocate for option 1, since it's more expedient, but just want to check thoughts on 2?
There was a problem hiding this comment.
Lets do option 1 here, but it is worth checking about deprecating it in the SDK itself, we can take that discussion internal.
- Refactored the Identity Form, now with added Validation! - Separated some types and enums into their own files, for clarity. - Added DittoServiceError type. - Added code docs and comments for Identity Configuration and Supplementary Credentials.
- Restored LoggingDetailsView to working order. - Updated ExportData to help distinguish it from ExportLogs.
…y with the new changes. - LoggingDetailsView now directly manipulates the DittoLogger attached to the ditto instance, rather than manipulating a passed Binding. This improves encapsulation and ensures that anyone can use this to easily visualize (and change) their Log level at runtime in their app, without having to create any new logic. - updated Readme - removed references to DittoLogger+LoggingOptions which is now replaced by an extension on DittoLogLevel instead.
- removed published log level tool, as the ExportLogs tool manipulates DittoLogger directly, to improve encapsulation and remove dependency. - added more documentation - improved error handling and throwing - migrated persistenceDirectoryURL logic to an extension, and made it throw - applied Swift Formatting
- Added an animated SyncButton that both indicates the sync status of the Ditto engine, and allows it to be toggled.
- improved destroyDittoInstance logic, ensuring everything is shut down correctly. - moved PersistenceDirectory extension into its own file.
- Used ViewBuilders to simplify the logic for multiplatform (tvOS vs. everything else) layout, to make the view easier to understand. - Resolved a logic issue where the logic for applyConfiguration had to be passed into the Form. This is now handled consistently across all platforms.
- Improved the separation of concerns by moving the implementation of services out of the Framework, and into the App layer. - Sources/AllToolsMenu → DittoToolsApp/MenuView - Refactored ToolsList → AllToolsMenu - removed unused classes, stale code
- Updated App Icon to current Ditto Logo - “DittoToolsApp” → “Ditto Tools” - Added tvOS assets (app can now be released to the App Store)
… group to improve clarity.
- Refactored params from SupplementalCredentials to be part of the Credentials object, which now holds a Ditto Identity, plus the other properties. - Simplifies KeychainService significantly, since now we’re only storing one object. NOTE: If you have previously installed the tools app on a device, and it stored credentials (ie. an older version from this branch), you may experience a crash.
- Moved tab bar item logic to parent ContentView - Simplified MenuView implementation - ContentView now managed showing/hiding CredentialsView
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes TOOLS-299
This PR started as a fix for the issue of tools crashing on AppleTV hardware (https://github.com/getditto/DittoSwiftTools/issues/153), but has evolved into a more significant refactor to improve the Identity Configuration flow, resulting in a more flexible, maintainable, and user-centered experience. I’m opening this PR now to invite feedback on the current direction, even as final work on some components is still underway. Early review and comments will help refine this approach further.
Changes in Scope
The changes touch several areas:
• Identity Configuration Service: Centralized identity management responsibilities, streamlining keychain interactions and making activeConfiguration handling more robust.
• Form Refactoring: Broke down the form into more modular components (IdentityForm, IdentityFormInputView, etc.), enabling a more adaptable UI that changes based on the identity type selected.
• Validation and Supplementary Fields: Added support for identity-specific fields and supplementary credentials (e.g., AuthDelegate details) in the form, with validation and input management centralized within the ViewModel.
• Error Handling and Sync State Management: Improved error handling with dedicated DittoServiceError cases, better encapsulating sync state and managing initialization flows.
Key Changes
• DittoService: Refined to distinguish between identity setup and sync management, now using distinct methods for initializing, starting, and stopping the sync engine.
• Configuration Data Flow: Moved identity and supplementary data persistence into the ConfigurationService, with Keychain integration for storage and retrieval.
• View Structure: Updated ConfigurationView to encapsulate form data binding and apply consistent identity configuration handling across iOS and tvOS.
• Delegates: Enhanced AuthDelegate integration to interact seamlessly with ConfigurationService, reducing duplicate identity configuration code.
Still in Progress
• UI Adjustments: Fine-tuning visual aspects of the form, especially on tvOS, to better align with system conventions.
• Validation and Error Messaging: Additional UI messaging for invalid configurations, with further error handling around sync restarts.
Next Steps
• Testing: I’ll continue testing each identity configuration type and sync state management in various scenarios, especially around error and edge cases.
• Further Review: Any feedback on architecture, naming, or potential simplifications would be highly appreciated at this stage.
Thanks for taking a look early. I’m excited to hear any thoughts and adjust as needed!