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 logic for showing DAITA filter pill #6944

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Oct 8, 2024

The filter pill in the filter view should be shown under the following circumstances:

Singlehop:

  • If DAITA is ON, Direct only is ON

Multihop:

  • DAITA is ON, Direct only is ON, entry tab i selected

This change is Reviewable

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

linear bot commented Oct 8, 2024

@rablador rablador force-pushed the filters-should-be-updated-ios-835 branch 3 times, most recently from f87bff4 to 3feb1df Compare October 14, 2024 08:02
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 17 of 26 files at r1.
Reviewable status: 3 of 27 files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 242 at r1 (raw file):

            let controller = SettingsViewController(
                interactor: interactorFactory.makeSettingsInteractor(),
                alertPresenter: AlertPresenter(context: self)

AlertPresenter doesn't hold a weak reference to its context, I wonder if we're not creating a retain cycle here 🤔
It probably should

@rablador rablador marked this pull request as ready for review October 14, 2024 08:54
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: 3 of 27 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 242 at r1 (raw file):

Previously, buggmagnet wrote…

AlertPresenter doesn't hold a weak reference to its context, I wonder if we're not creating a retain cycle here 🤔
It probably should

We're doing this a lot. Should we make context weak?

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: 3 of 27 files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 242 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We're doing this a lot. Should we make context weak?

I think so yes, out of precaution, it shouldn't have any negative impact

@rablador rablador force-pushed the filters-should-be-updated-ios-835 branch from 3feb1df to ba89bf6 Compare October 14, 2024 10:49
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: 3 of 28 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift line 242 at r1 (raw file):

Previously, buggmagnet wrote…

I think so yes, out of precaution, it shouldn't have any negative impact

Done.

@rablador rablador force-pushed the filters-should-be-updated-ios-835 branch from ba89bf6 to 9c033f9 Compare October 15, 2024 09:01
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 2 of 26 files at r1, 16 of 21 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 23 of 28 files reviewed, all discussions resolved

@rablador rablador force-pushed the filters-should-be-updated-ios-835 branch from 9c033f9 to 0aa4f44 Compare October 15, 2024 09:32
@buggmagnet buggmagnet merged commit a21b702 into main Oct 15, 2024
10 of 11 checks passed
@buggmagnet buggmagnet deleted the filters-should-be-updated-ios-835 branch October 15, 2024 09:47
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