-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
rewrite the Change Password view in SwiftUI #1183
base: develop
Are you sure you want to change the base?
Conversation
Just use the relatively new You can see it in action in
No, that's not a bug. The name "connectedAccounts" is merely historic. The right name would be "enabledAccounts" as that's what it is. You can use the |
That said: even if the account isn't currently connected due to some network outage, the password change IQ will still be sent out on the next connect after doing a smacks resume. If the resume isn't possible, the invalidation handler of the IQ will be called, if there is any. That can be used to trigger a notification that the password change failed due to network problems and should be retried. That way it won't matter if the account is connected the moment the user tries to change the password. |
Monal/Classes/ChangePassword.swift
Outdated
hideLoadingOverlay(overlay) | ||
if success { | ||
successAlert(title: Text("Success"), message: Text("The password has been changed")) | ||
MLXMPPManager.sharedInstance().updatePassword(newPass, forAccount: 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.
Rewrite changePassword:
to use an MLHandler and make that handler fulfill the pending promise (if there is one). That way you will have the promising behaviour usable for the UI while still handling the UI result even if it comes in after the UI was closed or even swiped away (the handler and invalidation handler I already mentioned).
You could show a success or error notification if there is no promise to fulfill (meaning the user closed the UI instead of waiting) and show something in the UI otherwise. I think that would be the most robust solution and provide the best UX :)
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 has been a bit hard for me, especially as there are no examples in the codebase of a handler fulfilling a promise. Can I set this aside?
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 proper solution would be to make promises somehow "serializable". Since promises can't truly survive a process switch/restart, adding all such promises to a global dictionary mapping some random uuid to the promise and use the uuid as serializable argument for MLHandlers, those handlers could simply ask some helper function to provide the corresponding promise, if any, and only fulfill that promise, if one is returned.
--> we need some (small) new framework for this.
Regarding promises in general: -(AnyPromise*) checkJidType:(NSString*) jid
in xmpp.m
might give you some inisght on how to return a promise and fulfill it, the usage of that function (in conjunction with the promising loading overlay) can be seen in AddContactMenu.swift
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.
--> we need some (small) new framework for this.
But this is a bit out of scope for this PR, don't you think? 🙂
Regarding promises in general
The latest version I pushed already uses promises
f09b500
to
0ba9f38
Compare
BTW: The handler framework is documented over here: https://github.com/monal-im/Monal/wiki/Handler-Framework/ Or in this blog article: https://monal-im.org/post/00007-monal-internals-handlers/ |
0ba9f38
to
23e2b33
Compare
c414304
to
82bb2b6
Compare
68452f7
to
5454976
Compare
194940c
to
41be78e
Compare
00e6271
to
a1c6acc
Compare
(I rebased without introducing any new changes, so you can easily git diff when I do make changes) |
01d4cb4
to
7071d13
Compare
7071d13
to
21a351d
Compare
Your code has a flaw: initiating a password change while either being offline or in a slow network condition and then swiping the app away will change the password on the server, but not in monal's password storage. Use |
...and update the password store in the supplied handler (I'd add the handler to MLIQProcessor.m) To inform the UI of success/error, we'll need a generalized mapping of uuids to PMKResolver functions. That uuid can be bound to the handler when its created and the handler can later on retrieve the PMKResolver corresponding to that uuid and resolve it, so that the ui is informed properly. |
That way, |
Another possibility would be using a normal showLoadingOverlay, and posting a notification from the Handler's code, containing any eventual error message. |
No, that would mean we'd have to invent new notifications for every signal we want to send to the UI. We need a generalized way to do this and what I suggested is exactly this. @matthewrfennell could you build that "promise registry" I described in #1183 (comment) (and the following comment)? That would be really nice! Unfortunately my own time is still very limited :( |
7225d2d
to
9f9f350
Compare
449a010
to
b380df6
Compare
Notable changes:
So it's clearer that we're talking about the "Current Password" field, not the "New Password" one (e.g. a server may have a restriction on the minimum length.)
Other suggestions: "Incorrect Password"
This string is displayed when the account we're editing is offline, even if other accounts are online. (the old string is wrong)
Notes:
MLXMPPManager.sharedInstance().getConnectedAccount(forID: accountId)
returns our account instead of returningnil
, which is presumably a bugMoreover, the checking of the old password is currently done using the local database:
MLXMPPManager.sharedInstance().isValidPassword(oldPass, forAccount: accountId)
Combined together, these two things might cause confusion to users in edge-cases. (I can provide an example if you're curious)
Potential improvements:
Feel free to suggest / make improvements.