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

Clarify lockdown limitations in guide #6293

Merged

Conversation

albin-mullvad
Copy link
Collaborator

@albin-mullvad albin-mullvad commented May 29, 2024

This PR aims to clarify the lockdown information in the in-app guide.


This change is Reviewable

@albin-mullvad albin-mullvad added the Android Issues related to Android label May 29, 2024
@albin-mullvad albin-mullvad self-assigned this May 29, 2024
Copy link

linear bot commented May 29, 2024

@albin-mullvad albin-mullvad force-pushed the clarify-lockdown-limitations-in-in-app-guide-droid-962 branch 2 times, most recently from 9fbef9d to 12eca2d Compare May 29, 2024 12:45
Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 24 files reviewed, 4 unresolved discussions


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 264 at r1 (raw file):

}

private enum class PAGES(

This PAGES structure doesn't feel optimal, but don't want to make too big changes at this point


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 328 at r1 (raw file):

                    }
                    append(" ")
                    pushUrlAnnotation(UrlAnnotation(stringResource(id = R.string.lockdown_url)))

Will be replaced by https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/AnnotatedString.kt;l=1168


android/lib/resource/src/main/res/values/strings.xml line 92 at r1 (raw file):

        <![CDATA[The Lockdown mode is called <b>Block connections without VPN</b> in the Android system settings. It helps minimize leaks, however it has some known limitations which you can read more about it]]>
    </string>
    <string name="auto_connect_carousel_third_slide_top_text_website_link">here.</string>

Should we include the dot or not? Is there a better way to build this string?

Code quote:

here.

android/lib/resource/src/main/res/values/strings_non_translatable.xml line 9 at r1 (raw file):

    <string name="faqs_and_guides_url" translatable="false">https://mullvad.net/help/tag/mullvad-app/</string>
    <string name="privacy_policy_url" translatable="false">https://mullvad.net/help/privacy-policy/</string>
    <string name="lockdown_url" translatable="false">https://mullvad.net/app-links/android-vpn-settings</string>

This URL needs to be aligned before we merge

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad)


android/lib/resource/src/main/res/values/strings.xml line 92 at r1 (raw file):

Previously, albin-mullvad wrote…

Should we include the dot or not? Is there a better way to build this string?

Is there a way ot include a format specifier (like %s) here because I guess it is possible that the translation would put the "here" somwhere else in the sentence. Or clarify to the translators that the link word should be at the end of the sentence?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 271 at r1 (raw file):

    FIRST(
        annotatedTopText =
            @Composable {

Feels like we should contemplate refactoring these buildAnnotatedString out, the depth of them is a ton, and gets even worse with the enum class. Maybe also some of the internal functions can be broken out for readability as well.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 343 at r1 (raw file):

                        )
                    }
                    pop()

What is it we pop here? pushUrlAnnotation? can we do something similar as withStyle where it automatically resets?


android/lib/resource/src/main/res/values/strings.xml line 92 at r1 (raw file):

Previously, albin-mullvad wrote…

Should we include the dot or not? Is there a better way to build this string?

Generally I don't think the dot should be included, but yeah it would be a bit of pain to just have a new string, unless we insert it in the other string as argument?

@albin-mullvad albin-mullvad requested review from Pururun and Rawa May 29, 2024 15:39
Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 271 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels like we should contemplate refactoring these buildAnnotatedString out, the depth of them is a ton, and gets even worse with the enum class. Maybe also some of the internal functions can be broken out for readability as well.

Since we probably want to replace this enum thing with separate composables I don't think we need to make this prettier at this point. We also want to ship this asap.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 343 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

What is it we pop here? pushUrlAnnotation? can we do something similar as withStyle where it automatically resets?

Yes, the pop for pushUrlAnnotation. Will not be needed in compose ui 1.7.0. I linked to the upstream change in my other comment: https://reviewable.io/reviews/mullvad/mullvadvpn-app/6293#-Nz3OKfz5E0H9NbI5aXk


android/lib/resource/src/main/res/values/strings.xml line 92 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Generally I don't think the dot should be included, but yeah it would be a bit of pain to just have a new string, unless we insert it in the other string as argument?

I'll see if I can find a better way, but I think we're a bit limited due to how we build the annotated string 🤔

@albin-mullvad albin-mullvad force-pushed the clarify-lockdown-limitations-in-in-app-guide-droid-962 branch 2 times, most recently from ee11850 to 40b58e8 Compare June 3, 2024 08:47
@Rawa Rawa added the On hold Means the PR is paused for some reason. No need to review it for now label Jun 10, 2024
@albin-mullvad albin-mullvad force-pushed the clarify-lockdown-limitations-in-in-app-guide-droid-962 branch from 40b58e8 to 6a96acd Compare June 13, 2024 11:36
@albin-mullvad albin-mullvad removed the On hold Means the PR is paused for some reason. No need to review it for now label Jun 13, 2024
Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 25 files reviewed, 4 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 271 at r1 (raw file):

Previously, albin-mullvad wrote…

Since we probably want to replace this enum thing with separate composables I don't think we need to make this prettier at this point. We also want to ship this asap.

Made it a bit prettier without too much work. Didn't change the enum class though since we can attack that later. Please have another look!


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 343 at r1 (raw file):

Previously, albin-mullvad wrote…

Yes, the pop for pushUrlAnnotation. Will not be needed in compose ui 1.7.0. I linked to the upstream change in my other comment: https://reviewable.io/reviews/mullvad/mullvadvpn-app/6293#-Nz3OKfz5E0H9NbI5aXk

Added a temporary withLink function. Please have a look!


android/lib/resource/src/main/res/values/strings.xml line 92 at r1 (raw file):

Previously, albin-mullvad wrote…

I'll see if I can find a better way, but I think we're a bit limited due to how we build the annotated string 🤔

Moved the dot out of this string and instead append it. Eventually it would be nice to do something like: https://github.com/Aghajari/AnnotatedText?tab=readme-ov-file#url-onclick

Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 25 files reviewed, 4 unresolved discussions (waiting on @Pururun and @Rawa)


android/lib/resource/src/main/res/values/strings_non_translatable.xml line 9 at r1 (raw file):

Previously, albin-mullvad wrote…

This URL needs to be aligned before we merge

Using https://mullvad.net/help/using-mullvad-vpn-on-android until we've settled on which URL to use due to problems with hide_nav in the shortened URL.

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r2, 24 of 24 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rawa)

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AutoConnectAndLockdownModeScreen.kt line 343 at r1 (raw file):

Previously, albin-mullvad wrote…

Added a temporary withLink function. Please have a look!

Nice 👍

Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


android/lib/resource/src/main/res/values/strings_non_translatable.xml line 9 at r1 (raw file):

Previously, albin-mullvad wrote…

Using https://mullvad.net/help/using-mullvad-vpn-on-android until we've settled on which URL to use due to problems with hide_nav in the shortened URL.

Will merge this as-is. URL will be updated in: https://linear.app/mullvad/issue/DROID-1060

@albin-mullvad albin-mullvad force-pushed the clarify-lockdown-limitations-in-in-app-guide-droid-962 branch from 6a96acd to bae524d Compare June 13, 2024 15:01
@albin-mullvad albin-mullvad merged commit 24810d5 into main Jun 13, 2024
34 checks passed
@albin-mullvad albin-mullvad deleted the clarify-lockdown-limitations-in-in-app-guide-droid-962 branch June 13, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants