use 'Cancel' instead of 'Back arrow' for server settings #2247
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
r10s
changed the title
r10s
force-pushed
the
r10s/cancel-server-settings
branch
from
ec6200c to
c436748
Compare
r10s
force-pushed
the
r10s/cancel-server-settings
branch
from
70dcb23 to
4ca9eaa
Compare
zeitschlag
approved these changes
Jul 31, 2024
deltachat-ios/Controller/AccountSetup/SecuritySettingsController.swift
Outdated
Show resolved
Hide resolved
Comment on lines
+737
to
+747
| class AccountSetupSecurityValue: SecuritySettingsDelegate { | ||
| var value: Int | ||
|
|
||
| init(initValue: Int) { | ||
| value = initValue | ||
| } | ||
|
|
||
| func onSecuritySettingsChanged(newValue: Int) { | ||
| value = newValue | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the reason this implementation of SecuritySettingsDelegate is a dedicated class, while CertificateCheckDelegate is implemented as extension of AccountSetupController?
There was a problem hiding this comment.
we need two different delegates for SecuritySettingsDelegate - i did not find out a smart way to do that as an extension. one could add some state to the delegate - but that seems to work against the idea of the delegate
|
thanks a lot for approval - and also, let me note, that the notes and comments atop of the approval are very welcome and on point, thanks for that as well 🙏 |
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.
this PR fixes the issue that selecting "IMAP Security", "SMTP Security" and "Certificate Checks" are persisted in core even if no "Log In" was tried. this leads to inconsistent views. even more inconsistency was added as the text fields are persisted correctly on "Log In", so, the user never knows which field was cancelled and which not.
the reason was, that
SecuritySettingsControllerandCertificateChecksControllerpersisted the value too soon in core when hitting "OK" in the controller.this PR calls a delegate instead which sets a value in the
AccountSetupController. only if finally "Log In" is hit, the values are persisted in core together with the value from the text fields.the code of the whole controller is a little bit messy, this PR does not clean up there as things actually are working so far. instead, the PR fixes the issue on point, keeping other flows and conventions.
finally, the PR removes some unimportant summary which is available better in "view log", adds simple "back" buttons to subsequent dialogs and renames the main "back" button to cancel (which was the original reason of #2246, i thought the rename is easy :)
closes #2246