Skip to content

Lanes view filtering#3903

Merged
Udumft merged 3 commits into
release-v2.5from
vk/lanes-view-check
May 20, 2022
Merged

Lanes view filtering#3903
Udumft merged 3 commits into
release-v2.5from
vk/lanes-view-check

Conversation

@Udumft
Copy link
Copy Markdown
Contributor

@Udumft Udumft commented May 20, 2022

Description

PR removes hiding lanes view when all of the suggested lanes are valid for the current route. This is done for parity with Android and for additional user info since it may still matter and affect lane selection to avoid slow left/right turning lanes on the way

Implementation

Replaced filtering check with a sanity check that at least 1 of the proposed lanes is valid.

@Udumft Udumft requested a review from a team May 20, 2022 12:05
@Udumft Udumft self-assigned this May 20, 2022
}

guard !subviews.isEmpty && subviews.contains(where: { !$0.isValid }) else {
guard !subviews.isEmpty && subviews.contains(where: { $0.isValid }) else {
Copy link
Copy Markdown
Contributor

@bamx23 bamx23 May 20, 2022

Choose a reason for hiding this comment

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

Are we checking that "all subviews are valid" or "any subview is valid"?

Suggested change
guard !subviews.isEmpty && subviews.contains(where: { $0.isValid }) else {
guard !subviews.isEmpty && subviews.allSatisfy({ $0.isValid }) else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are checking that at least 1 subview is valid

Comment thread CHANGELOG.md Outdated
Co-authored-by: Nikolay Volosatov <mikalai.valasatau@mapbox.com>
@Udumft Udumft merged commit b8dd271 into release-v2.5 May 20, 2022
@Udumft Udumft deleted the vk/lanes-view-check branch May 20, 2022 12:30
@Udumft Udumft mentioned this pull request May 20, 2022
@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented May 20, 2022

The regression fixed here was originally introduced in 992f456 for #2149. I don’t see any discussion around it from that time, so I think we can be reasonably confident that it was just an unnecessary optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants