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

Update connection details on main view to indicate that DAITA is used #6719

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Sep 2, 2024

When DAITA is being used, the main view should indicate as much.

The main view should be updated to follow desktop and add extra text to the relay line to indicate if DAITA is being used. For now, multihop and singlehop cases shouldn't be differentiated.

Whether DAITA is used or not should be inferred from the communication between the client and the packet tunnel.


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Sep 2, 2024
@rablador rablador self-assigned this Sep 2, 2024
Copy link

linear bot commented Sep 2, 2024

@rablador rablador marked this pull request as draft September 2, 2024 13:17
Copy link
Contributor

@buggmagnet buggmagnet 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 56 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 239 at r1 (raw file):

            if observedState.connectionState?.isDaitaEnabled == true {
                connectedRelayName += " using DAITA"

Should we use localised strings here ?


ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 18 at r1 (raw file):

      "location" : "https://github.com/mullvad/wireguard-apple.git",
      "state" : {
        "revision" : "fb9a34f15e47b167866af3257a8dcc9901d9e7c1"

Please remove this change

Copy link
Contributor

@buggmagnet buggmagnet 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 56 files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 239 at r1 (raw file):

Previously, buggmagnet wrote…

Should we use localised strings here ?

Or rather, I think this should be the localised string instead of setting it below, since those work with lookup tables, we'd need a different localised string for every possible "relay name + using daita" combination, if you see what I mean

@rablador rablador force-pushed the update-connection-details-on-main-view-to-indicate-that-ios-775 branch 2 times, most recently from f370501 to 19103d7 Compare September 3, 2024 11:00
Copy link
Contributor Author

@rablador rablador 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 56 files reviewed, 1 unresolved discussion


ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 239 at r1 (raw file):

Previously, buggmagnet wrote…

Or rather, I think this should be the localised string instead of setting it below, since those work with lookup tables, we'd need a different localised string for every possible "relay name + using daita" combination, if you see what I mean

I see indeed, however, we should come up with something more clever than simply adding the strings as they are. On desktop, the Swedish translation is grammatically wrong since it's directly translated. 😕

@rablador rablador force-pushed the update-connection-details-on-main-view-to-indicate-that-ios-775 branch 3 times, most recently from a687981 to 40fd83b Compare September 5, 2024 13:55
@rablador rablador changed the base branch from main to change-packettunnel-to-support-daita-ios-776 September 5, 2024 13:56
@rablador rablador force-pushed the update-connection-details-on-main-view-to-indicate-that-ios-775 branch from 40fd83b to 8d08e8c Compare September 5, 2024 13:57
Copy link
Contributor Author

@rablador rablador 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 56 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 18 at r1 (raw file):

Previously, buggmagnet wrote…

Please remove this change

Done.

@rablador rablador marked this pull request as ready for review September 5, 2024 13:59
@pinkisemils pinkisemils force-pushed the change-packettunnel-to-support-daita-ios-776 branch from 166e7ef to 678f653 Compare September 6, 2024 10:37
Base automatically changed from change-packettunnel-to-support-daita-ios-776 to main September 6, 2024 10:59
@rablador rablador force-pushed the update-connection-details-on-main-view-to-indicate-that-ios-775 branch from 8d08e8c to 97ffc00 Compare September 6, 2024 11:11
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 56 of 56 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

@rablador rablador force-pushed the update-connection-details-on-main-view-to-indicate-that-ios-775 branch from 97ffc00 to aab3dd7 Compare September 6, 2024 12:42
@pinkisemils pinkisemils merged commit b0b7db0 into main Sep 6, 2024
8 of 9 checks passed
@pinkisemils pinkisemils deleted the update-connection-details-on-main-view-to-indicate-that-ios-775 branch September 6, 2024 12:47
Copy link

github-actions bot commented Sep 6, 2024

🚨 End to end tests failed. Please check the failed workflow run.

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

Successfully merging this pull request may close these issues.

3 participants