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.

Sign up for GitHub

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

[ios] pull down InstructionView handle to show remaining steps #276

Merged
merged 6 commits into from
Oct 2, 2024
Merged

[ios] pull down InstructionView handle to show remaining steps #276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dc60c64
[iOS] Pull down cue sheet shows remaining steps
michaelkirk Sep 20, 2024
8815b49
Move test factories and mark them internal
ianthetechie Oct 1, 2024
1fb29b7
Make expanded properties bindings
ianthetechie Oct 1, 2024
a941753
Add TODO for discussion
ianthetechie Oct 1, 2024
d0dfc82
fixup! Make expanded properties bindings
michaelkirk Oct 2, 2024
1015d92
More small cleanup
ianthetechie Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion apple/Sources/FerrostarMapLibreUI/Views/Overylays/LandscapeNavigationOverlayView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ struct LandscapeNavigationOverlayView: View, CustomizableNavigatingInnerGridView
private var navigationState: NavigationState?

@State private var isInstructionViewExpanded: Bool = false
// TODO: Same as portrait?

var topCenter: (() -> AnyView)?
var topTrailing: (() -> AnyView)?
Expand Down
8 changes: 7 additions & 1 deletion apple/Sources/FerrostarSwiftUI/Views/InstructionsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,14 @@ public struct InstructionsView: View {
_sizeWhenNotExpanded = sizeWhenNotExpanded
}

/// The visual instructions *after* the one for the current maneuver,
/// paired with the route step.
///
/// Note that in the case of steps with multiple instructions,
/// only the first is returned in this list.
/// This is sufficient for display of the upcoming maneuver list.
private var nextVisualInstructions: [(VisualInstruction, RouteStep)] {
guard let remainingSteps, !remainingSteps.isEmpty else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer your change because it's clearer, but I don't see how the previous version was a bug.

If remaining steps had one thing in it, then remainingSteps[1...] == [] so the result is the same, right?

Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I went off to code up a Swift Fiddle, because OBVIOUSLY Swift is zero-indexed, and accessing index 1 of an array of length 1 is out of bounds access...

You can subscript an array with any integer from zero up to, but not including, the count of the array. Using a negative number or an index equal to or greater than count triggers a runtime error.

But' here's a fiddle that proves your original code actually returns empty array as you claim: https://swiftfiddle.com/aw3i6znzvjcjzi3pdoefmu23sy. I've been writing Swift since the 0.x days and I have to say, this is the most surprised I've been in a very long time! 🤯

Scouring the Apple docs, I couldn't figure out why this works as it does, but your code was indeed correct 😂 If you've got any links explaining why this is the way it is, I'd be grateful to learn!

Copy link
Contributor Author

@michaelkirk michaelkirk Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

let foo = ["a"]
print("foo[0] => \(foo[0])") // "a"
// print("foo[1] => \(foo[1])") // Fatal error: Index out of range

print("foo[0...] => \(foo[0...])") // ["a"]
print("foo[1...] => \(foo[1...])") // []

// And my favorite - while this works (and seems like it should):
print("foo[0...] => \(foo[0...])") // ["a"]
print("foo[0..0] => \(foo[0...0])") // ["a"]

// This blows up
print("foo[1...] => \(foo[1...])") // []
// print("foo[1..1] => \(foo[1...1])") // Fatal error: Array index is out of range

// I think this is a hint to the riddle
// print("foo[2...] => \(foo[2...])") // Swift/Range.swift:754: Fatal error: Range requires lowerBound <= upperBound

// Note the error! 
// It's obvious where the `lowerBound` is coming from (2), 
// but where is the `upperBound` coming from which is greater than 2?

func example() {
  // My guess is that accessing a RandomAccessCollection with PartialRangeFrom, 
  // converts the PartialRangeFrom to a Range (not a ClosedRange!), using the Array.length as the `upperBound` of the Range. That would explain all the behavior we're seeing.

  do {
    let rangeFrom: PartialRangeFrom = 2...
    let arrayLength = 1
    // this will panic when rangeFrom.start is greater than arrayLength - e.g. 2..<1
    let range: Range = rangeFrom.lowerBound..<arrayLength 
  }

  do {
    // but this will not panic, it'll just be empty:
    let rangeFrom: PartialRangeFrom = 1...
    let arrayLength = 1
    let range: Range = rangeFrom.lowerBound..<arrayLength 
    assert(range == 1..<1)
  }
}

Link to fiddle: https://swiftfiddle.com/l4jhe6atjjgkdnih6ot6rsyzgy

guard let remainingSteps, remainingSteps.count > 1 else {
return []
}
return remainingSteps[1...].compactMap { step in
Expand Down
2 changes: 1 addition & 1 deletion apple/Sources/FerrostarSwiftUI/Views/TopDrawerView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct TopDrawerView<PersistentContent: View, ExpandedContent: View>: View {
expandedContent.padding(.bottom, 50)
}

var framedScrollView = if isExpanded {
let framedScrollView = if isExpanded {
AnyView(scrollView.frame(idealHeight: CGFloat.infinity))
} else {
AnyView(scrollView.frame(height: max(0, dragOffset)))
Expand Down
Loading