-
-
Notifications
You must be signed in to change notification settings - Fork 120
Rewrite the Settings view in SwiftUI #1317
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
base: develop
Are you sure you want to change the base?
Conversation
Revert this commit after the Account Settings are rewritten in SwiftUI (and are linked to from Settings.swift)
|
That's really ugly and quite strange. I'd be curious what causes this distortion. As for your PR itself: I just tried it and I'm okay with the Even though I liked to have a split-view for our settings, I'm okay with removing that and only having a single-view in that case. But closing the settings should always be possible, split-view or not. |
On iPads (at least on the iPad 6th generation simulator), you can close the modal by clicking the area outside it, or by swiping it down. |
|
I don't like it if swiping down/tapping outside is the only method to close things. there should always be a button to do the same. |
tmolitor-stud-tu
left a comment
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.
My main point is: I'd like the xmpp class to be a data model just like the MLContact class is and then be used in conjunction with the ObservableKVOWrapper to get an auto-updating ui.
| return MLXMPPManager.sharedInstance().connectedTime(for: self.accountID) | ||
| } | ||
| var avatar: UIImage { | ||
| return MLImageManager.sharedInstance().getIconFor(MLContact.createContact(fromJid: self.jid, andAccountID: self.accountID)) ?? UIImage(named: "noicon")! |
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.
You should use the avatar property of MLContact to get auto-updates (wrap the MLContact in ObservableKVOWrapper.
| Text(account.jid) | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
|
|
||
| Text(self.connectionStatusString) |
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.
see above
| uptimeFormatter.doesRelativeDateFormatting = true | ||
| } | ||
|
|
||
| var connectionStatusString: String { |
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.
use the accountState of our xmpp object (in a ObservableKVOWrapper) and a switch statement to stringify the values of the xmppState enum.
| return MLXMPPManager.sharedInstance().isAccount(forIdConnected: self.accountID) | ||
| } | ||
| var connectedTime: Date { | ||
| return MLXMPPManager.sharedInstance().connectedTime(for: self.accountID) |
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.
use connectedTime property of our xmpp class, wrapped by a ObservableKVOWrapper.
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 connectedTimeFor: method of MLXMPPManager can even be removed afterwards (no other code is using it if I don't miss something).
| #endif | ||
| } | ||
| Section(header: Text("App")) { | ||
| NavigationLink(destination: LazyClosureView(GeneralSettings())) { |
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 general settings should now be embedded in this settings rather than being a submenu (I only used a submenu because the settings weren't yet ported to swiftui).
| UIPasteboard.general.setValue(HelperTools.appBuildVersionInfo(for: MLVersionType.IQ), forPasteboardType: UTType.utf8PlainText.identifier) | ||
| #if !DEBUG | ||
| tappedVersionInfo += 1 | ||
| if tappedVersionInfo > 16 { |
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.
| if tappedVersionInfo > 16 { | |
| if tappedVersionInfo >= 16 { |
|
|
||
| struct Settings: View { | ||
| @State private var tappedVersionInfo = 0 | ||
| @State private var showDebugEntry = HelperTools.defaultsDB().bool(forKey: "showLogInSettings") |
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.
Use a new class with @defaultsDB annotated properties to hold and autoupdate these settings, see the implementation of GeneralSettingsDefaultsDB in GeneralSettings.swift
| #if !DEBUG | ||
| tappedVersionInfo += 1 | ||
| if tappedVersionInfo > 16 { | ||
| HelperTools.defaultsDB().set(true, forKey: "showLogInSettings") |
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.
You don't need this, if you use the @defaultsDB annotation (see comment above).
| } | ||
|
|
||
| @objc | ||
| func makeSettingsView() -> UIViewController { |
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.
Since makeSettingsView does not need any special argument, you should add the settings view to our makeView function below.
| } | ||
| } | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: NSNotification.Name(kMonalAccountStatusChanged)).receive(on: RunLoop.main)) { notification in |
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 won't be needed anymore, once the xmpp class is a proper data model class used in conjunction with the ObservableKVOWrapper.
622ff1d to
7b95939
Compare
b1bf422 to
3c4d40b
Compare
518a17d to
eaba046
Compare
bc01177 to
6e8eeac
Compare
f5da846 to
f31652b
Compare
57224b1 to
3196274
Compare
130e72b to
3741841
Compare
I was going to link the UIKit AccountEdit view from the SwiftUI settings, so alpha users can use this PR, but it turned out to be ugly:
AccountEdit.mp4
As a result, the UIKit views rewritten in this PR are still being used. This should be the case until the account settings are in SwiftUI.
In order to test this PR, you need to revert lissine0@60839b8c
Notes:
mailto:anditms-apps://) which should open the relevant apps externally (I did not test this because it's not possible on the Simulator, but it should work).Please indicate if you want these things added to the SwiftUI rewrite. I think the second one makes sense, as the modal is nicer than opening an app externally, but I'm not sure about the split view.