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

Add daita and multihop pages to the root of the settings view #7232

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Nov 25, 2024

Adds the new settings pages for both DAITA and multhop to the settings view.


This change is Reviewable

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

linear bot commented Nov 25, 2024

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch 3 times, most recently from d2f6154 to efa6e2c Compare November 25, 2024 14:58
acb-mv
acb-mv previously approved these changes Nov 25, 2024
Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

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

Reviewed 44 of 61 files at r1, 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 244 at r2 (raw file):

    /// - Parameter route: the route for which to request the new view controller or child coordinator.
    /// - Returns: a result of creating a child for the route.
    // swiftlint:disable:next function_body_length

Perhaps it's a good time to break this up into multiple functions and calling them, i.e.,

case .multihop:
    return makeMultihopViewController(tunnelManager:interactorFactory.tunnelManager)
case .daita:
  ...

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch from efa6e2c to 996efad Compare November 26, 2024 08:13
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: 43 of 70 files reviewed, 1 unresolved discussion (waiting on @acb-mv)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 244 at r2 (raw file):

Previously, acb-mv wrote…

Perhaps it's a good time to break this up into multiple functions and calling them, i.e.,

case .multihop:
    return makeMultihopViewController(tunnelManager:interactorFactory.tunnelManager)
case .daita:
  ...

Agreed

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch 10 times, most recently from 0149272 to 5dc221e Compare December 2, 2024 13:37
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.

The new images added for Daita and Multihop appear quite pixelated (especially the internet icon somehow)
Screenshot 2024-12-02 at 13.24.15.jpeg

Reviewed 40 of 61 files at r1, 15 of 27 files at r4, 10 of 14 files at r5, 6 of 10 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 69 of 72 files reviewed, 3 unresolved discussions (waiting on @acb-mv and @rablador)


a discussion (no related file):
We are using the incorrect image for the multihop illustration, this should be a phone, not a desktop
IMG_AB8D8F273F48-1.jpeg


ios/MullvadVPN/Coordinators/Settings/SettingsViewControllerFactory.swift line 16 at r5 (raw file):

struct SettingsViewControllerFactory {
    /// The result of creating a child representing a route.
    enum MakeChildResult {

nit
If it's intended to be a Result, why not simply use the Swift.Result type ?
This would avoid leaking SettingsViewControllerFactory.MakeChildResult in SettingsCoordinator

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch from 5dc221e to 4dfad7c Compare December 2, 2024 13:58
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.

Fixed!

Reviewable status: 68 of 72 files reviewed, 3 unresolved discussions (waiting on @acb-mv and @buggmagnet)


a discussion (no related file):

Previously, buggmagnet wrote…

We are using the incorrect image for the multihop illustration, this should be a phone, not a desktop
IMG_AB8D8F273F48-1.jpeg

Fixed!


ios/MullvadVPN/Coordinators/Settings/SettingsViewControllerFactory.swift line 16 at r5 (raw file):

Previously, buggmagnet wrote…

nit
If it's intended to be a Result, why not simply use the Swift.Result type ?
This would avoid leaking SettingsViewControllerFactory.MakeChildResult in SettingsCoordinator

Sometimes we return a viewcontroller, sometimes a coordinator. How would we help the compiler know which is which? I'm guessing that's why MakeChildResult was introduced in the first place (I simply refactored it).

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch from 4dfad7c to 9e05364 Compare December 2, 2024 15:04
@acb-mv acb-mv requested a review from buggmagnet December 3, 2024 13:00
Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 21 files at r8, all commit messages.
Reviewable status: 65 of 86 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)


ios/MullvadVPN/Coordinators/Settings/SettingsViewControllerFactory.swift line 16 at r5 (raw file):

Previously, rablador (Jon Petersson) wrote…

Sometimes we return a viewcontroller, sometimes a coordinator. How would we help the compiler know which is which? I'm guessing that's why MakeChildResult was introduced in the first place (I simply refactored it).

Not every result is a Result.

Another option would be to call it a MakeChildProduct or similar, omit .failed and wrap it in an optional or Result (or a throwing call) if it's failable

buggmagnet
buggmagnet previously approved these changes Dec 3, 2024
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.

Reviewed 3 of 10 files at r6, 21 of 21 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acb-mv)

@pinkisemils
Copy link
Collaborator

We should update the changelog here, otherwise looks good to me.

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch 3 times, most recently from 70d7a5a to e69e8ba Compare December 4, 2024 09:55
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.

Done.

Reviewable status: 47 of 87 files reviewed, all discussions resolved (waiting on @acb-mv and @buggmagnet)

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 1 of 3 files at r11.
Reviewable status: 48 of 87 files reviewed, all discussions resolved (waiting on @acb-mv and @buggmagnet)

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 48 of 87 files reviewed, all discussions resolved (waiting on @acb-mv and @buggmagnet)

@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch from e69e8ba to 5d7af0f Compare December 4, 2024 13:04
@rablador rablador force-pushed the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch from 5d7af0f to fc659c6 Compare December 4, 2024 13:16
@rablador rablador merged commit b3d61b2 into main Dec 4, 2024
13 of 14 checks passed
@rablador rablador deleted the add-daita-page-to-the-root-of-the-settings-view-ios-922 branch December 4, 2024 13:18
Copy link

github-actions bot commented Dec 4, 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.

5 participants