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.

Sign up for GitHub

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

Jump to bottom

Added confirmation dialog for "Distrust System Certificates" #1307

Open
wants to merge 4 commits into
base: main-ose
Choose a base branch
from
Open

Added confirmation dialog for "Distrust System Certificates" #1307

wants to merge 4 commits into from
+51 −3

Conversation

ArnyminerZ
Copy link
Member

Purpose

See #1294

Short description

  • Added a confirmation dialog for enabling the "Distrust System Certificates" option.
  • Disabling the option doesn't show any warning.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ linked an issue Feb 11, 2025 that may be closed by this pull request
[UI] Too easy to activate "Distrust system certs" #1294
Open
@ArnyminerZ ArnyminerZ self-assigned this Feb 11, 2025
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Feb 11, 2025
@ArnyminerZ
Copy link
Member Author

image

@ArnyminerZ
Automatic dialog dismiss
c93c327 
Signed-off-by: Arnau Mora <[email protected]>
@ArnyminerZ ArnyminerZ marked this pull request as ready for review February 11, 2025 19:05
@ArnyminerZ ArnyminerZ changed the title Added confirmation dialog for dsc Added confirmation dialog for "Distrust System Certificates" Feb 11, 2025
sunkup
sunkup requested changes Feb 12, 2025
View reviewed changes
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well :)

I would probably write something different, though - but we can also stick to your version. The most important is to have the user understand they are doing some potentially functionality breaking changes.

Title: "Attention!"
Text: "Enabling this option will cause DAVx5 to distrust system certificates. You will need to manually install the required certificates. This option is intended for advanced users."

Screenshot

image

What do you think? Also how about a preview of the card?

app/src/main/kotlin/at/bitfire/davdroid/ui/AppSettingsScreen.kt Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@ArnyminerZ
Copy link
Member Author

Added preview:

Image

image

@ArnyminerZ ArnyminerZ requested a review from sunkup February 13, 2025 08:51
@ArnyminerZ
Copy link
Member Author

I've also updated the text as you've said. @devvv4ever anything to change? See screenshot above

@devvv4ever
Copy link
Member

I'd prefer it to be an even shorter text ;-) Maybe: "This can prevent DAVx5 to connect to your server, unless you install additional certificates manually". What do you think @rfc2822 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devvv4ever devvv4ever Awaiting requested review from devvv4ever

@sunkup sunkup Awaiting requested review from sunkup

Requested changes must be addressed to merge this pull request.

Assignees

@ArnyminerZ ArnyminerZ

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UI] Too easy to activate "Distrust system certs"
3 participants
@ArnyminerZ @devvv4ever @sunkup