Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lissine0
Copy link
Member

@lissine0 lissine0 commented Aug 2, 2024

Notable changes:

  • Disable the "Change Password" button if at least one of the input fields is empty (instead of having a check and an error message)
  • Don't prefill the old password for non-Quicksy users (see a426c9a)
  • Changing some of the strings:
    • "Invalid Password" -> "Wrong Password"
      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"
    • "No accounts connected" -> "Account offline"
      This string is displayed when the account we're editing is offline, even if other accounts are online. (the old string is wrong)

Notes:

  • Changing the password can be so fast that the LoadingOverlay disappears pretty quickly. Should we add a small waiting to make it stay on the screen a bit longer?
  • Currently, if we're not connected to the internet, but our account on Monal is enabled, MLXMPPManager.sharedInstance().getConnectedAccount(forID: accountId) returns our account instead of returning nil, which is presumably a bug
    Moreover, 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.

@tmolitor-stud-tu
Copy link
Member

Just use the relatively new showPromisingLoadingOverlay() instead of the old pait of showLoadingOverlay() and hideLoadingOverlay().
The promising loading overlay will not only return a promise, but will make that promise wait at least for one second before it gets resolved (it can take longer, if the underlying action (a promise itself) takes longer, though, so that's a minimum bound).

You can see it in action in MemberList.swift for example.

Currently, if we're not connected to the internet, but our account on Monal is enabled, MLXMPPManager.sharedInstance().getConnectedAccount(forID: accountId) returns our account instead of returning nil, which is presumably a bug

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 accountState property of an xmpp object and check if it is at state kStateBound to know if the account is currently connected and logged in etc.

@tmolitor-stud-tu
Copy link
Member

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 Show resolved Hide resolved
hideLoadingOverlay(overlay)
if success {
successAlert(title: Text("Success"), message: Text("The password has been changed"))
MLXMPPManager.sharedInstance().updatePassword(newPass, forAccount: accountId)
Copy link
Member

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 :)

Copy link
Member Author

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?

Copy link
Member

@tmolitor-stud-tu tmolitor-stud-tu Sep 17, 2024

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

Copy link
Member Author

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

@tmolitor-stud-tu
Copy link
Member

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/

@lissine0
Copy link
Member Author

(I rebased without introducing any new changes, so you can easily git diff when I do make changes)

@lissine0 lissine0 force-pushed the change-password branch 3 times, most recently from 7071d13 to 21a351d Compare September 16, 2024 20:23
@tmolitor-stud-tu
Copy link
Member

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 sendIq:withHandler: instead of sendIq:withResponseHandler:andErrorHandler:

@tmolitor-stud-tu
Copy link
Member

...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.

@tmolitor-stud-tu
Copy link
Member

That way, -(AnyPromise*) changePassword:(NSString*) newPass would still return a promise, but swiping away the app will just kill that promise instead of killing the handling of the iq response...

@lissine0
Copy link
Member Author

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.

Another possibility would be using a normal showLoadingOverlay, and posting a notification from the Handler's code, containing any eventual error message.
The ChangePassword view would listen for that notification, and when it arrives, it would hide the LoadingOverlay, and either show success alert or error alert, depending on the metadata in the notification.

@tmolitor-stud-tu
Copy link
Member

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 :(

@lissine0
Copy link
Member Author

lissine0 commented Nov 2, 2024

Recent changes:

  • Rebase on the develop branch.
  • Dismiss the view on success of changing password.
  • Don't show the password in cleartext for Quicksy users. But prefill it still.
    Instead, it will be shown in cleartext in the Account Settings -> Login Credentials

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants