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 state to new connection view #7302

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Dec 9, 2024

Connection details and buttons should change depending on the tunnel state, just like the old connection view.


This change is Reviewable

@rablador rablador self-assigned this Dec 9, 2024
Copy link

linear bot commented Dec 9, 2024

@rablador rablador changed the title Add state to connection view ios 960 Add state to new connection view Dec 9, 2024
@rablador rablador force-pushed the add-state-to-connection-view-ios-960 branch 7 times, most recently from ff60cf0 to c0b676d Compare December 11, 2024 13:53
@rablador rablador added the iOS Issues related to iOS label Dec 11, 2024
@rablador rablador marked this pull request as ready for review December 11, 2024 13:54
@rablador rablador force-pushed the add-state-to-connection-view-ios-960 branch from c0b676d to 95e1928 Compare December 12, 2024 07:24
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.

Somehow, when building MockRelease I cannot get multihop PQ to work, and I'm not sure why.

Reviewed 15 of 16 files at r1, all commit messages.
Reviewable status: 15 of 16 files reviewed, 5 unresolved discussions (waiting on @rablador)


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

import UIKit

class STunnelViewController: UIViewController, RootContainment {

This single letter change broke Release builds 🙈 🫠


ios/MullvadVPN/Views/SplitMainButton.swift line 15 at r1 (raw file):

    var image: ImageResource
    var style: MainButtonStyle.Style
    var disabled = false

Same comment about default values, this also guarantees that we cannot have default values sneakingly doing the wrong thing when we don't expect them.


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FI_TunnelViewController.swift line 191 at r1 (raw file):

        guard let connectionViewProxy = connectionController.view else {
            fatalError("Invalid view state")

Given that the view property on UIHostingController is declared as UIView! we can do the same here and force unwrap.
The documentation also says that the view will be loaded at runtime if it's nil so we should be safe here too.

The same goes for the mapView above


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionViewViewModel.swift line 137 at r1 (raw file):

}

extension ConnectionViewViewModel {

nit
I wonder if we want to have both the ViewModel and its ViewBuilder in the same file.
I think it'd be nice to separate concerns (data / UI) and have the extension either in ConnectionView because it's used there, or just as a standalone extension.

What do you think ?


ios/MullvadVPN/Views/MainButton.swift line 14 at r1 (raw file):

    var text: LocalizedStringKey
    var style: MainButtonStyle.Style
    var disabled = false

Let's remove that default value and specify it in the #Preview macro below instead

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: 15 of 16 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)


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

Previously, buggmagnet wrote…

This single letter change broke Release builds 🙈 🫠

Oh man, I did that to test things out at some point, but forgot to change back... 🙈


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionViewViewModel.swift line 137 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I wonder if we want to have both the ViewModel and its ViewBuilder in the same file.
I think it'd be nice to separate concerns (data / UI) and have the extension either in ConnectionView because it's used there, or just as a standalone extension.

What do you think ?

Yeah, I think you're right. I wanted to keep the view itself very clean, but I went a bit too far.


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FI_TunnelViewController.swift line 191 at r1 (raw file):

Previously, buggmagnet wrote…

Given that the view property on UIHostingController is declared as UIView! we can do the same here and force unwrap.
The documentation also says that the view will be loaded at runtime if it's nil so we should be safe here too.

The same goes for the mapView above

Done.


ios/MullvadVPN/Views/MainButton.swift line 14 at r1 (raw file):

Previously, buggmagnet wrote…

Let's remove that default value and specify it in the #Preview macro below instead

I didn't want to make them mandatory since that's not how buttons tend to work. They're usually enabled by default, letting you disable them if you explicitly want to.

@rablador rablador force-pushed the add-state-to-connection-view-ios-960 branch 2 times, most recently from ebf37a7 to 1d78bc5 Compare December 13, 2024 10:06
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.

:lgtm:

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the add-state-to-connection-view-ios-960 branch from 1d78bc5 to c61b7d4 Compare December 13, 2024 13:31
@rablador rablador merged commit 37e90ed into main Dec 13, 2024
11 checks passed
@rablador rablador deleted the add-state-to-connection-view-ios-960 branch December 13, 2024 13:39
Copy link

🚨 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.

2 participants