Settings2swiftui #1
Conversation
- Using LabeledContent instead of HStack - Proper handling of settingsSendCrashReports when already set - Integrated ClientCertificatesView, RTFTexView
…ingsViewController are not needed anymore
WalkthroughThe pull request introduces several modifications across multiple files in the openHAB project. Key changes include updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientCertificatesView
participant ClientCertificatesViewModel
participant NetworkConnection
User->>ClientCertificatesView: Load Certificates
ClientCertificatesView->>ClientCertificatesViewModel: loadCertificates()
ClientCertificatesViewModel->>NetworkConnection: Retrieve Client Certificates
NetworkConnection-->>ClientCertificatesViewModel: Return Certificates
ClientCertificatesViewModel-->>ClientCertificatesView: Update UI with Certificates
sequenceDiagram
participant User
participant SettingsView
participant Preferences
User->>SettingsView: Modify Settings
SettingsView->>Preferences: Save Settings
Preferences-->>SettingsView: Confirm Save
SettingsView-->>User: Update UI with New Settings
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (6)
openHAB/OpenHABNotification.swift (1)
70-70: Decoding Logic inOpenHABNotificationExtensionThe use of a
CodingDatastruct to handle decoding and then converting it to anOpenHABNotificationinstance is a clean and efficient approach. This separation of concerns aids in maintaining clean code and simplifies the decoding process.Consider documenting the decoding keys and their mapping to the model properties to enhance maintainability and understandability of the code.
openHAB/Main.storyboard (4)
Line range hint
1-1: Review of Main.storyboard: Significant Structural ChangesThe removal of entire scenes related to "Settings", "Client Certificates", and "Legal" indicates a major shift in how these functionalities are handled within the app. It's crucial to ensure that these functionalities are either being deprecated or migrated to new interfaces. This change could impact user navigation and accessibility, requiring thorough testing and potentially updates to user documentation.
Ensure that the new structure is communicated clearly to all stakeholders and that documentation is updated accordingly to reflect these changes. Consider user feedback mechanisms to gauge the impact of these changes on the end-user experience.
Line range hint
1-1: Potential Impact on User ExperienceRemoving direct UI components for settings management could simplify the interface but might also reduce user control or transparency. It's important to balance simplicity with user empowerment, especially in an application like openHAB that likely has power users who appreciate detailed control settings.
Evaluate user engagement and satisfaction post-release to assess whether the new UI meets the needs of all user segments. Consider reintroducing certain advanced settings in a more integrated or streamlined manner if user feedback indicates a gap.
Line range hint
1-1: Consistency and AccessibilityWith the restructuring of the storyboard, maintaining consistency in UI design and ensuring accessibility are paramount. This includes consistent use of color schemes, font sizes, and interactive elements that are accessible to users with disabilities.
Review the remaining UI components for consistency in visual and interactive design. Ensure that accessibility standards are met, particularly for new or modified UI elements. This might include voice-over compatibility, sufficient color contrast, and easy navigation for users with limited dexterity.
Line range hint
1-1: Documentation and Developer CommunicationSignificant changes to a project's UI, especially the removal of entire scenes, require clear communication within the development team and external documentation. This ensures that all team members are aware of the changes and that external developers or contributors can adapt their workflows or integrations accordingly.
Update internal and external documentation to reflect these changes. Consider creating visual aids or diagrams that outline the new navigation flows and UI structure to assist in onboarding new developers or updating existing integrations.
openHAB/OpenHABRootViewController.swift (1)
24-27: Incomplete Implementation ofTargetControllerEnum CasesThe
TargetControllerenum has been updated with new cases, but thebrowsercase is not yet implemented, which could lead to incomplete functionality. Ensure that thebrowsercase is fully integrated into the application logic to maintain robustness.
OpenHABRootViewController.swift: Implement logic for thebrowsercase.Analysis chain
Approved: Enhancements to
TargetControllerenum.The modifications to the
TargetControllerenum enhance its flexibility and functionality. Ensure that all references to these enum cases are updated throughout the application to maintain consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the new enum cases in the application. # Test: Search for the enum usage. Expect: Only occurrences of the new cases. rg --type swift -A 5 $'TargetController'Length of output: 2942
Script:
#!/bin/bash # Description: Verify the usage of the new enum cases `browser` and `tile` in the application. # Test: Search for the `browser` case usage. Expect: Occurrences of the `browser` case. rg --type swift -A 5 $'case .browser' # Test: Search for the `tile` case usage. Expect: Occurrences of the `tile` case. rg --type swift -A 5 $'case .tile'Length of output: 441
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- .gitignore (1 hunks)
- BuildTools/.swiftlint.yml (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/Endpoint.swift (1 hunks)
- openHAB.xcodeproj/project.pbxproj (13 hunks)
- openHAB.xcodeproj/xcshareddata/xcschemes/openHAB.xcscheme (1 hunks)
- openHAB/ClientCertificatesView.swift (1 hunks)
- openHAB/ClientCertificatesViewModel.swift (1 hunks)
- openHAB/DrawerView.swift (1 hunks)
- openHAB/Main.storyboard (4 hunks)
- openHAB/NotificationsView.swift (1 hunks)
- openHAB/OpenHABDrawerItem.swift (2 hunks)
- openHAB/OpenHABDrawerTableViewController.swift (2 hunks)
- openHAB/OpenHABNotification.swift (2 hunks)
- openHAB/OpenHABRootViewController.swift (8 hunks)
- openHAB/OpenHABSitemapViewController.swift (1 hunks)
- openHAB/RTFTextView.swift (1 hunks)
- openHAB/SettingsView.swift (1 hunks)
Files skipped from review due to trivial changes (3)
- .gitignore
- BuildTools/.swiftlint.yml
- openHAB.xcodeproj/xcshareddata/xcschemes/openHAB.xcscheme
Additional context used
SwiftLint
openHAB/DrawerView.swift
[Warning] 117-117: TODOs should be resolved (Replace network calls with app...)
(todo)
openHAB/SettingsView.swift
[Error] 234-234: Force casts should be avoided
(force_cast)
Additional comments not posted (18)
openHAB/ClientCertificatesView.swift (3)
12-12: Approved import statement.The import of SwiftUI is essential for building the user interface in this context.
14-30: Well-structured SwiftUI view for managing client certificates.The
ClientCertificatesViewstruct is well-defined with a clear separation of view model and view layers. The use of@StateObjectfor the view model ensures that the view model is correctly retained during the lifecycle of the view. The dynamic list handling withForEachand.onDeleteis correctly implemented, and the use of localization for the navigation title is commendable.
33-35: Preview section correctly implemented.The preview provider is correctly set up, which is useful for visual development in SwiftUI.
openHAB/ClientCertificatesViewModel.swift (1)
12-14: Approved import statements.The imports are essential for the functionality of the view model, including logging and SwiftUI compatibility.
openHAB/OpenHABDrawerItem.swift (2)
13-13: Approved import of SFSafeSymbols.The import of
SFSafeSymbolsis necessary for using system symbols safely, which enhances the maintainability and safety of the code.
38-44: Enhanced functionality with icon computed property.The addition of the
iconcomputed property to theOpenHABDrawerItemenum is a significant enhancement. It encapsulates the visual representation directly within the enum, making it easier to manage and use throughout the UI. This change is well-implemented and follows best practices for encapsulation and usability.openHAB/OpenHABNotification.swift (1)
19-26: Enhancements toOpenHABNotificationClassThe updates to the
OpenHABNotificationclass are thoughtful and improve its flexibility and usability:
- New Property
id: The addition of theidproperty is crucial for uniquely identifying notifications, which is especially useful in a list context.- Updated Initializers: The inclusion of default values in the primary initializer allows for more flexible instantiation of notification objects. This is a good practice in Swift to facilitate easier and safer object creation.
- Dictionary Initializer: The updated dictionary initializer now handles more properties and includes a date formatter for the
createdproperty. Ensure that the date format string matches the expected input to avoid parsing errors.Consider adding error handling or logging in the dictionary initializer to manage cases where date parsing fails.
Also applies to: 29-31
OpenHABCore/Sources/OpenHABCore/Util/Endpoint.swift (1)
36-47: EnumSortSitemapsOrderenhancements are well-implemented.The additions to the
SortSitemapsOrderenum are correctly implemented and enhance the enum's usability in user interfaces and logs.openHAB/OpenHABDrawerTableViewController.swift (1)
Line range hint
189-290: ClassOpenHABDrawerTableViewControllerupdates enhance readability and functionality.The introduction of pattern matching and the modification of the delegate method argument are positive changes. However, ensure that these changes are compatible with existing delegate implementations.
Verify the compatibility of the delegate method argument change with existing implementations.
openHAB/SettingsView.swift (2)
12-36: Approved: Import statements and initial setup ofSettingsView.The import statements are appropriate and necessary for the functionality of the view. The
@Stateproperties are correctly used to manage the state of UI components in a SwiftUI view.
149-185: Approved: Handling ofsettingsSendCrashReportstoggle.The implementation uses
.onAppearand.onChangemodifiers effectively to manage the state of the toggle based on user preferences and changes. The confirmation dialog is appropriately used to confirm the user's intent when enabling crash reporting, which is a good practice for features that have significant implications.openHAB.xcodeproj/project.pbxproj (5)
90-90: Review of Added Files in PBXBuildFile SectionThe following new files have been added to the project's build phases:
SettingsView.swiftClientCertificatesViewModel.swiftClientCertificatesView.swiftDrawerView.swiftNotificationsView.swiftRTFTextView.swiftThese additions are consistent with the PR's objectives to introduce new SwiftUI views and view models. Ensure that these files are correctly placed in the project directory and referenced in the appropriate targets for compilation.
Also applies to: 101-101, 102-102, 105-105, 106-106, 115-115
381-381: Review of Added Files in PBXFileReference SectionThe following new file references have been added:
SettingsView.swiftClientCertificatesViewModel.swiftClientCertificatesView.swiftDrawerView.swiftNotificationsView.swiftRTFTextView.swiftThese references are necessary for the Xcode project to recognize the files. Verify that the paths and file types are correctly specified and that they match the actual files in the project directory.
Also applies to: 393-393, 394-394, 397-397, 398-398, 409-409
864-868: Review of Added Files in Sources SectionThe following new source files have been added to the project's sources:
SettingsView.swiftClientCertificatesView.swiftRTFTextView.swiftNotificationsView.swiftDrawerView.swiftThese additions are aligned with the PR's objectives to expand the project's functionality with new views and components. Confirm that these files are included in the correct targets to ensure they are compiled and linked appropriately.
997-997: Review of Added Files in Models and Sources SectionsThe following new model and source files have been added:
ClientCertificatesViewModel.swiftin ModelsClientCertificatesViewModel.swiftin SourcesDrawerView.swiftin SourcesThese additions support the new functionality related to client certificates and UI components. Ensure that the model is correctly integrated with the view and that all necessary data bindings are established.
Also applies to: 1495-1495, 1500-1500
1519-1519: Review of ClientCertificatesView.swift in SourcesThe file
ClientCertificatesView.swifthas been correctly added to the sources section, which is necessary for it to be compiled and included in the app. This file should contain the SwiftUI view for managing client certificates as described in the PR objectives.openHAB/OpenHABRootViewController.swift (1)
143-154: Approved: Integration of SwiftUIDrawerView.The integration of
DrawerViewusingUIHostingControlleris a positive step towards modernizing the user interface. Ensure that the integration is seamless and test for any issues that might arise when interacting with existing UIKit components.Verification successful
Approved: SwiftUI Integration with UIKit
The integration of SwiftUI views using
UIHostingControllerinopenHAB/OpenHABRootViewController.swiftis consistent and aligns with best practices for embedding SwiftUI within a UIKit application. Ensure thorough testing to verify that these integrations do not interfere with existing UIKit functionalities, particularly gesture recognizers and navigation flows.
- Instances of
UIHostingControllerwrappingdrawerView,SettingsView, andNotificationsVieware correctly integrated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of SwiftUI components with UIKit. # Test: Search for the integration usage. Expect: Proper configuration and usage. rg --type swift -A 5 $'UIHostingController'Length of output: 1446
openHAB/OpenHABSitemapViewController.swift (1)
Line range hint
1-1: Approved: Removal ofwebViewControllerproperty.The removal of the
webViewControllerproperty fromOpenHABSitemapViewControllersimplifies the class structure. Ensure that all references to this property are properly addressed to avoid runtime errors.
| class ClientCertificatesViewModel: ObservableObject { | ||
| @Published var clientCertificates: [SecIdentity] = [] | ||
|
|
||
| init() { | ||
| loadCertificates() | ||
| } | ||
|
|
||
| func loadCertificates() { | ||
| clientCertificates = NetworkConnection.shared.clientCertificateManager.clientIdentities | ||
| } | ||
|
|
||
| func deleteCertificate(at index: Int) { | ||
| let status = NetworkConnection.shared.clientCertificateManager.deleteFromKeychain(index: index) | ||
| if status == noErr { | ||
| clientCertificates.remove(at: index) | ||
| } | ||
| } | ||
|
|
||
| func getIdentityName(for index: Int) -> String { | ||
| NetworkConnection.shared.clientCertificateManager.getIdentityName(index: index) | ||
| } | ||
| } |
There was a problem hiding this comment.
Well-implemented view model for client certificates.
The ClientCertificatesViewModel class is well-structured with appropriate use of ObservableObject for state management in SwiftUI. The @Published property for client certificates allows the view to react to changes. The methods for loading and deleting certificates are clear and interact correctly with the shared network connection. Consider adding error handling or feedback mechanisms for operations that can fail, such as deleting a certificate.
Consider enhancing the user experience by handling errors or providing feedback when operations fail.
| struct RTFTextView: UIViewRepresentable { | ||
| let rtfFileName: String | ||
|
|
||
| func makeUIView(context: Context) -> UITextView { | ||
| let textView = UITextView() | ||
| textView.isEditable = false | ||
| textView.isSelectable = true | ||
| textView.backgroundColor = UIColor.clear | ||
| return textView | ||
| } | ||
|
|
||
| func updateUIView(_ uiView: UITextView, context: Context) { | ||
| if let url = Bundle.main.url(forResource: rtfFileName, withExtension: "rtf") { | ||
| do { | ||
| let attributedString = try NSAttributedString( | ||
| url: url, | ||
|
|
||
| options: [.characterEncoding: String.Encoding.utf8.rawValue], | ||
| documentAttributes: nil | ||
| ) | ||
| uiView.attributedText = attributedString | ||
| uiView.backgroundColor = .ohSystemBackground | ||
| uiView.textColor = .ohLabel | ||
| } catch { | ||
| print("Failed to load RTF file: \(error.localizedDescription)") | ||
| } | ||
| } else { | ||
| print("RTF file not found") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of RTFTextView SwiftUI View
The RTFTextView struct is well-implemented with clear separation of concerns between view creation and update logic. Here are some suggestions for improvement:
- Error Handling: Currently, errors in loading the RTF file are logged using
printstatements. Consider using a more robust logging mechanism or providing user feedback through the UI. - Code Documentation: Adding comments explaining the purpose of each method and the parameters used can enhance readability and maintainability.
- Preview Section: The preview provider uses an empty string for
rtfFileName. It might be more illustrative to use a sample file name or include a conditional check to handle empty or invalid filenames gracefully in the view itself.
| struct NotificationsView: View { | ||
| @State var notifications: [OpenHABNotification] = [] | ||
|
|
||
| var body: some View { | ||
| List(notifications, id: \.id) { notification in | ||
| NotificationRow(notification: notification) | ||
| } | ||
| .refreshable { | ||
| loadNotifications() | ||
| } | ||
| .navigationTitle("Notifications") | ||
| .onAppear { | ||
| loadNotifications() | ||
| } | ||
| } | ||
|
|
||
| private func loadNotifications() { | ||
| NetworkConnection.notification(urlString: Preferences.remoteUrl) { response in | ||
| DispatchQueue.main.async { | ||
| switch response.result { | ||
| case let .success(data): | ||
| do { | ||
| let decoder = JSONDecoder() | ||
| decoder.dateDecodingStrategy = .formatted(DateFormatter.iso8601Full) | ||
| let codingDatas = try data.decoded(as: [OpenHABNotification.CodingData].self, using: decoder) | ||
| notifications = codingDatas.map(\.openHABNotification) | ||
| } catch { | ||
| os_log("%{PUBLIC}@ ", log: .default, type: .error, error.localizedDescription) | ||
| } | ||
| case let .failure(error): | ||
| os_log("%{PUBLIC}@", log: .default, type: .error, error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of NotificationsView SwiftUI View
The NotificationsView is well-structured for displaying a list of notifications and includes effective state management and data loading mechanisms:
- Network Integration: The integration of network calls directly in the view is handled well, using asynchronous callbacks to update the UI. Consider encapsulating network logic into a separate model or view model to adhere to the MVVM pattern more strictly, enhancing testability and maintainability.
- Error Handling: Currently, errors from network requests are logged using
os_log. Enhance user experience by providing UI feedback in case of errors, such as displaying an error message or a retry option.
| struct NotificationRow: View { | ||
| var notification: OpenHABNotification | ||
|
|
||
| // App wide data access | ||
| var appData: OpenHABDataObject? { | ||
| AppDelegate.appDelegate.appData | ||
| } | ||
|
|
||
| var body: some View { | ||
| HStack { | ||
| KFImage(iconUrl) | ||
| .placeholder { | ||
| Image("openHABIcon").resizable() | ||
| } | ||
| .resizable() | ||
| .frame(width: 40, height: 40) | ||
| .cornerRadius(8) | ||
| VStack(alignment: .leading) { | ||
| Text(notification.message) | ||
| .font(.body) | ||
| if let timeStamp = notification.created { | ||
| Text(dateString(from: timeStamp)) | ||
| .font(.caption) | ||
| .foregroundColor(.gray) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .padding(.vertical, 8) | ||
| } | ||
|
|
||
| private var iconUrl: URL? { | ||
| if let appData { | ||
| return Endpoint.icon( | ||
| rootUrl: appData.openHABRootUrl, | ||
| version: appData.openHABVersion, | ||
| icon: notification.icon, | ||
| state: "", | ||
| iconType: .png, | ||
| iconColor: "" | ||
| ).url | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| private func dateString(from date: Date) -> String { | ||
| let formatter = DateFormatter() | ||
| formatter.dateStyle = .medium | ||
| formatter.timeStyle = .medium | ||
| formatter.timeZone = TimeZone.current | ||
| return formatter.string(from: date) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of NotificationRow Subview
The NotificationRow subview is effectively implemented to display individual notifications with an image and text. Here are some points for potential improvement:
- Image Handling: The use of
Kingfisherfor image loading is efficient. Ensure that the placeholder image is appropriate and consider handling error states where the image cannot be loaded. - Date Formatting: The method
dateString(from:)is well-implemented. Consider using a static date formatter to improve performance, as creating a new formatter for each cell can be resource-intensive.
[performance]
| public enum IconType: Int, CaseIterable, Identifiable, CustomStringConvertible { | ||
| case png | ||
| case svg | ||
|
|
||
| public var id: Self { self } | ||
|
|
||
| public var description: String { | ||
| switch self { | ||
| case .png: | ||
| "PNG" | ||
| case .svg: | ||
| "SVG" | ||
| } | ||
| } |
There was a problem hiding this comment.
Enum IconType enhancements are well-implemented.
The additions to the IconType enum are correctly implemented and follow Swift best practices. However, consider using a more descriptive string for the .svg case to align with the .png case's clarity.
Consider refining the description property for clarity:
- "SVG"
+ "Scalable Vector Graphics (SVG)"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public enum IconType: Int, CaseIterable, Identifiable, CustomStringConvertible { | |
| case png | |
| case svg | |
| public var id: Self { self } | |
| public var description: String { | |
| switch self { | |
| case .png: | |
| "PNG" | |
| case .svg: | |
| "SVG" | |
| } | |
| } | |
| public enum IconType: Int, CaseIterable, Identifiable, CustomStringConvertible { | |
| case png | |
| case svg | |
| public var id: Self { self } | |
| public var description: String { | |
| switch self { | |
| case .png: | |
| "PNG" | |
| case .svg: | |
| "Scalable Vector Graphics (SVG)" | |
| } | |
| } |
| dismiss() | ||
| onDismiss(.notifications) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| .listStyle(.inset) | ||
| .onAppear(perform: loadData) | ||
| } | ||
|
|
||
| private func loadData() { | ||
| // TODO: Replace network calls with appropriate @EnvironmentObject or other state management | ||
| loadSitemaps() | ||
| loadUiTiles() | ||
| } | ||
|
|
||
| private func loadSitemaps() { | ||
| // Perform network call to load sitemaps and decode | ||
| // Update the sitemaps state | ||
|
|
||
| NetworkConnection.sitemaps(openHABRootUrl: appData?.openHABRootUrl ?? "") { response in | ||
| switch response.result { | ||
| case let .success(data): | ||
| os_log("Sitemap response", log: .viewCycle, type: .info) | ||
|
|
||
| sitemaps = deriveSitemaps(data) | ||
|
|
||
| if sitemaps.last?.name == "_default", sitemaps.count > 1 { | ||
| sitemaps = Array(sitemaps.dropLast()) | ||
| } | ||
|
|
||
| // Sort the sitemaps according to Settings selection. | ||
| switch SortSitemapsOrder(rawValue: Preferences.sortSitemapsby) ?? .label { | ||
| case .label: sitemaps.sort { $0.label < $1.label } | ||
| case .name: sitemaps.sort { $0.name < $1.name } | ||
| } | ||
|
|
||
| drawerItems.removeAll() | ||
| case let .failure(error): | ||
| os_log("%{PUBLIC}@", log: .default, type: .error, error.localizedDescription) | ||
| drawerItems.removeAll() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func loadUiTiles() { | ||
| // Perform network call to load UI Tiles and decode | ||
| // Update the uiTiles state | ||
| NetworkConnection.uiTiles(openHABRootUrl: appData?.openHABRootUrl ?? "") { response in | ||
| switch response.result { | ||
| case .success: | ||
| os_log("ui tiles response", log: .viewCycle, type: .info) | ||
| guard let responseData = response.data else { | ||
| os_log("Error: did not receive data", log: OSLog.remoteAccess, type: .info) | ||
| return | ||
| } | ||
| do { | ||
| uiTiles = try JSONDecoder().decode([OpenHABUiTile].self, from: responseData) | ||
| } catch { | ||
| os_log("Error: did not receive data %{PUBLIC}@", log: OSLog.remoteAccess, type: .info, error.localizedDescription) | ||
| } | ||
| case let .failure(error): | ||
| os_log("%{PUBLIC}@", log: .default, type: .error, error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mutating func loadSettings() { | ||
| openHABUsername = Preferences.username | ||
| openHABPassword = Preferences.password | ||
| } | ||
| } | ||
|
|
||
| struct ImageView: View { | ||
| let url: String | ||
|
|
||
| // App wide data access | ||
| var appData: OpenHABDataObject? { | ||
| AppDelegate.appDelegate.appData | ||
| } | ||
|
|
||
| var body: some View { | ||
| if !url.isEmpty { | ||
| switch url { | ||
| case _ where url.hasPrefix("data:image"): | ||
| let provider = Base64ImageDataProvider(base64String: url.deletingPrefix("data:image/png;base64,"), cacheKey: UUID().uuidString) | ||
| return KFImage(source: .provider(provider)).resizable() | ||
| case _ where url.hasPrefix("http"): | ||
| return KFImage(URL(string: url)).resizable() | ||
| default: | ||
| let builtURL = Endpoint.resource(openHABRootUrl: appData?.openHABRootUrl ?? "", path: url.prepare()).url | ||
| return KFImage(builtURL).resizable() | ||
| } | ||
| } else { | ||
| // This will always fallback to placeholder | ||
| return KFImage(URL(string: "bundle://openHABIcon")).placeholder { Image("openHABIcon").resizable() } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Struct DrawerView is well-structured for SwiftUI.
DrawerView is well-implemented with appropriate use of SwiftUI's state management and environment properties. However, consider improving the data loading methods to ensure they are more robust and handle errors effectively.
Improve the data loading methods by adding error handling and possibly using a more reactive approach with Combine or similar frameworks.
Tools
SwiftLint
[Warning] 117-117: TODOs should be resolved (Replace network calls with app...)
(todo)
| Button { | ||
| let websiteDataTypes = NSSet(array: [WKWebsiteDataTypeDiskCache, WKWebsiteDataTypeMemoryCache]) | ||
| let date = Date(timeIntervalSince1970: 0) | ||
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {} |
There was a problem hiding this comment.
Issue: Avoid force casting at line 234.
Using force casting (as!) can lead to runtime crashes if the cast is not valid. It's safer to use conditional casting (as?) and handle the case where the cast fails gracefully.
- let websiteDataTypes = NSSet(array: [WKWebsiteDataTypeDiskCache, WKWebsiteDataTypeMemoryCache])
- let date = Date(timeIntervalSince1970: 0)
- WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {}
+ let websiteDataTypes = Set([WKWebsiteDataTypeDiskCache, WKWebsiteDataTypeMemoryCache])
+ let date = Date(timeIntervalSince1970: 0)
+ WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes, modifiedSince: date) {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {} | |
| let websiteDataTypes = Set([WKWebsiteDataTypeDiskCache, WKWebsiteDataTypeMemoryCache]) | |
| let date = Date(timeIntervalSince1970: 0) | |
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes, modifiedSince: date) {} |
Tools
SwiftLint
[Error] 234-234: Force casts should be avoided
(force_cast)
| private func handleDismiss(mode: TargetController) { | ||
| switch mode { | ||
| case .webview: | ||
| // Handle webview navigation or state update | ||
| print("Dismissed to WebView") | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) | ||
| switchView(target: .webview) | ||
| case .settings: | ||
| print("Dismissed to Settings") | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) { | ||
| self.modalDismissed(to: .settings) | ||
| } | ||
| case let .sitemap(sitemap): | ||
| Preferences.defaultSitemap = sitemap | ||
| appData?.sitemapViewController?.pageUrl = "" | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) { | ||
| self.modalDismissed(to: .sitemap(sitemap)) | ||
| } | ||
| case .notifications: | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) { | ||
| self.modalDismissed(to: .notifications) | ||
| } | ||
| case let .browser(urlString): | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) { | ||
| self.modalDismissed(to: .browser(urlString)) | ||
| } | ||
| case let .tile(urlString): | ||
| SideMenuManager.default.rightMenuNavigationController?.dismiss(animated: true) { | ||
| self.modalDismissed(to: .tile(urlString)) | ||
| } |
There was a problem hiding this comment.
Approved: Updated handleDismiss method.
The expansion of the handleDismiss method to handle new cases from the TargetController enum is well-implemented. Consider adding unit tests to verify the behavior of this method under various conditions.
Would you like me to help by writing some unit tests for this method?
Summary by CodeRabbit
New Features
ClientCertificatesViewfor managing and displaying client certificates.DrawerViewfor enhanced navigation within the application.NotificationsViewfor displaying notifications with pull-to-refresh functionality.RTFTextViewfor displaying Rich Text Format files.SettingsViewfor comprehensive application settings management.Bug Fixes
Refactor
OpenHABRootViewControllerto enhance modal view management.Chores