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

Fix android DAITA strings and translations #7329

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

albin-mullvad
Copy link
Collaborator

@albin-mullvad albin-mullvad commented Dec 11, 2024

This PR aims to align and fix some of DAITA related strings and translations.

TODO:

  • Double check whether it should be Go to %s settings (like the desktop app) or Open %s settings.

This change is Reviewable

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

linear bot commented Dec 11, 2024

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.

I get a lot of java.util.MissingFormatArgumentException: Format specifier '%s' for several strings with arguments. I am not sure why.

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


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

    <string name="feature_server_ip_override">Server IP override</string>
    <string name="feature_custom_mtu">MTU</string>
    <string name="feature_daita">DAITA</string>

This needs to be changed in FeatureIndicatorsPanel


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

                stringResource(
                    R.string.daita_description_slide_1_third_paragraph,
                    stringResource(id = R.string.daita),

This does not require argument

@albin-mullvad albin-mullvad force-pushed the fix-android-daita-translations-droid-1642 branch 2 times, most recently from 5eb7615 to 7c1bed9 Compare December 12, 2024 10:11
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 21 of 25 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 112 at r2 (raw file):

        Text(
            text =
                stringResource(

🙈


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

        message="Inconsistent number of arguments in formatting string `daita_description_slide_2_second_paragraph`; found both 1 here and 2 in values/strings.xml"
        errorLine1="    &lt;string name=&quot;daita_description_slide_2_second_paragraph&quot;>Daher verwenden wir automatisch Multihop, um %1$s mit jedem Server zu aktivieren.&lt;/string>"
        errorLine2="    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">

Have we tried that this works fine? Some languages only using 2 arguments while others are using 1.

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 all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DaitaScreen.kt line 235 at r2 (raw file):

                    R.string.daita_description_slide_2_second_paragraph,
                    stringResource(id = R.string.daita),
                    stringResource(id = R.string.daita)

Probably nicer to have %1$s in the string, but maybe that won't work with the translation script?

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: 21 of 30 files reviewed, 5 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DaitaScreen.kt line 217 at r1 (raw file):
You sure? 🤔 This is the string:

If an observer monitors these data packets, %s makes it significantly harder for them to identify which websites you are visiting or with whom you are communicating.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DaitaScreen.kt line 235 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Probably nicer to have %1$s in the string, but maybe that won't work with the translation script?

I can try that 👍


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 112 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

🙈

Indeed...


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

Previously, Pururun (Jonatan Rhodin) wrote…

This needs to be changed in FeatureIndicatorsPanel

Done.


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Have we tried that this works fine? Some languages only using 2 arguments while others are using 1.

The lint warning seemed to suggest that it's fine, but will double check by running the app

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 6 of 6 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Rawa)


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

Previously, albin-mullvad wrote…

You sure? 🤔 This is the string:

If an observer monitors these data packets, %s makes it significantly harder for them to identify which websites you are visiting or with whom you are communicating.

Weird, it seems my comment got in the wrong place, it should referer to

stringResource(  
    R.string.daita\_description\_slide\_1\_second\_paragraph,  
    stringResource(id = R.string.daita),  
)

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 1 of 6 files at r3, 2 of 3 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

Previously, albin-mullvad wrote…

The lint warning seemed to suggest that it's fine, but will double check by running the app

👍 @Pururun tested, and it didn't crash the app. But yeah we should address this in the near future.

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 1 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)


.github/workflows/android-app.yml line 243 at r4 (raw file):

          - gradle-task: :test:arch:test --rerun-tasks
          - gradle-task: detekt
          - gradle-task: :app:lint

Should this change? or mistake?

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 4 of 6 files at r3, 1 of 3 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)

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: 29 of 30 files reviewed, 3 unresolved discussions (waiting on @Pururun and @Rawa)


.github/workflows/android-app.yml line 243 at r4 (raw file):
Explained in git commit:

Limit linting to the app module in CI since it has the checkDependencies option set.


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

Previously, Pururun (Jonatan Rhodin) wrote…

Weird, it seems my comment got in the wrong place, it should referer to

stringResource(  
    R.string.daita\_description\_slide\_1\_second\_paragraph,  
    stringResource(id = R.string.daita),  
)

Done.


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

👍 @Pururun tested, and it didn't crash the app. But yeah we should address this in the near future.

@Pururun did it look correct as well (apart from not crashing)?

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 all commit messages.
Reviewable status: 29 of 30 files reviewed, 2 unresolved discussions (waiting on @Rawa)


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

Previously, albin-mullvad wrote…

@Pururun did it look correct as well (apart from not crashing)?

Yes looked fine, with quotation marks and everything.

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: 28 of 30 files reviewed, 2 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DaitaScreen.kt line 235 at r2 (raw file):

Previously, albin-mullvad wrote…

I can try that 👍

Seems to work well, I've added indices to all related strings


android/lib/resource/lint-baseline.xml line 8 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Yes looked fine, with quotation marks and everything.

Perfect, thanks! 🎉

@albin-mullvad albin-mullvad marked this pull request as ready for review December 12, 2024 16:15
Limit linting to the app module in CI since it has
the `checkDependencies` option set.
See the baseline added as part of this commit for details.

The issue itself has been reported to Crowdin so we
expect it to be addressed sooner rather than later.
@albin-mullvad albin-mullvad force-pushed the fix-android-daita-translations-droid-1642 branch from 24e1ef0 to 0a15d44 Compare December 12, 2024 17:38
@albin-mullvad
Copy link
Collaborator Author

This PR has now been rebased on top of #7337 in order to clarify the single baseline addition that this PR itself introduces.

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 10 of 10 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (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 8 of 10 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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 2 of 10 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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