Index out of range when binding a Slider value to a nested array in EnvironmentObject
Asked Answered
M

1

0

Description:

I have a model that has the following hierarchy:

  • Recipe
  • ...steps (an array)
  • ...currentStep
  • ......parameters (an array)
  • .........minimum
  • .........maximum
  • .........default
  • .........current

The model works well. I can add steps, parameters, and set the current step to an @EnvironmentObject called recipe.

I've created a sample project here with I two lists of steps and parameters, along with three buttons to add a single step among three hard-coded ones, each containing an array of 0, 1, or 3 parameters.

The top list is the step rows, each being a button to populate the bottom list. The bottom list is the parameter list, each containing a label and a slider in a VStack.

All works fine, except when i am (a) binding the slider to my model and (b) the list contains more sliders (row) than the current step now has. I get an index out of range error.

If I bind the slider value to a local variable, it all works. Here's the relevant code:

class Recipe: BindableObject {
    var didChange = PassthroughSubject<Void, Never>()
    var currentStep = Step() {
        didSet {
            didChange.send(())
        }
    }
}

struct Parameter: Identifiable {
    var id:Int = 0
    var name = ""
    var minimum:Float = 0
    var maximum:Float = 100
    var `default`:Float = 30
    var current:Float = 30
}

struct StepRow: View {
    @EnvironmentObject var recipe: Recipe
    var step: Step!

    init(step: Step) {
        self.step = step
    }
    var body: some View {
        Button(action: {
            self.setCurrentStep()
        }) {
            HStack {
                Text(step.name).font(Font.body.weight(.bold))
            }.frame(height: 50)
        }
    }
    func setCurrentStep() {
        recipe.currentStep = step
    }
}
struct ParameterRow: View {
    @EnvironmentObject var recipe: Recipe
    @State var sliderValue:Float = 30
    var parameter: Parameter!

    init(parameter: Parameter) {
        self.parameter = parameter
    }

    var body: some View {
        VStack {
            Text(parameter.name)
            Slider(

                // This works, swap these two lines to duplicate the index out of range error by:
                // - Adding step 1, step 2, step 3, and finally step 4
                // - Tapping each step in the step list in order, the first three will work but the last one won't

                //value: $recipe.currentStep.parameters[parameter.id].current,
                value: self.$sliderValue,

                from: parameter.minimum,
                through: parameter.maximum,
                by: 1.0
            )
        }
    }
}
struct ContentView : View {
    @EnvironmentObject var recipe: Recipe
    var body: some View {
        VStack {
            List {
                ForEach(recipe.steps) { step in
                    StepRow(step: step)
                }
            }
            List {
                ForEach(recipe.currentStep.parameters) { parameter in
                    ParameterRow(parameter: parameter)
                }
            }
        }
    }
}

Again, a working example of this is project here.

Macdougall answered 12/7, 2019 at 13:44 Comment(7)
I'm still going through your question. But just a quick comment. You can use default, you simply need to use reverse quotes in the definition. I've seen Apple doing it all the time in SwiftUI declaration file: var `default`: String. Then you can use the variable without the quotes.Archducal
Thanks for the tip! I'll be sure to implement it - this is a CoreImage app and that's the more correct way to use it.Macdougall
@kontiki, I do have a local sample project. It might take me 30 minutes to clean up a few things and tie in all my tries. It'll be my first use of GitHub in Xcode 13. Let me know if you want it.Macdougall
That would be great. I'm more fluent in code, than english ;-)Archducal
@kontiki, it's uploaded at github.com/justdfd/ListBug I'll update the question also.Macdougall
got it, I'll have a look later today, when I'm back at my desk.Archducal
@kontiki, thanks. If you find the issue I'll most certainly edit my question by shortening the wordiness and adding the specific code!Macdougall
A
1

I'm still going through your code. But I'd like to comment on something that caught my eye in your functions addStepX():

    func addStep1() {
        let newStep = Step(id: UUID(), name: "Step #1", parameters: [Parameter]())
        currentStep = newStep
        steps.insert(newStep, at: steps.count)
    }

Are you aware that the steps.insert() will not trigger a didSet, and so the didChange.send() will not execute? I propose you invert the order and first insert the step, and later you update currentStep. This way you call didChange.send() exactly once, after all your changes are done.

    func addStep1() {
        let newStep = Step(id: UUID(), name: "Step #1", parameters: [Parameter]())
        steps.insert(newStep, at: steps.count)
        currentStep = newStep
    }

Note that changing that still does not fix the problem, but I though I should mention, as it is definitely a problem.

UPDATE

After your changes, the code looks much cleaner. And I seem to have found a way of preventing the out of bounds.

It seems the problem is due to a timing issue. Your array gets updated, but the parameter passed by the List is still old. Eventually, it will catch up, but because of the out of bound crash... it never does. So why not make it conditional?

Note that I also added the slider value to the Text() view, to make it evident that the binding is successful:

struct ParameterRow: View {
    @EnvironmentObject var recipe: Recipe
    @State var sliderValue:Float = 30
    var parameter: Parameter!

    init(parameter: Parameter) {
        self.parameter = parameter
    }

    var body: some View {
        VStack {
            Text("\(parameter.name) = \(parameter.id < recipe.currentStep.parameters.count ? recipe.currentStep.parameters[parameter.id].current : -1)")
            Slider(
                value: parameter.id < recipe.currentStep.parameters.count ? $recipe.currentStep.parameters[parameter.id].current : .constant(0),
                from: parameter.minimum,
                through: parameter.maximum,
                by: 1.0
            )
        }
    }
}
Archducal answered 12/7, 2019 at 15:47 Comment(9)
That may be the issue in terms of the index out of bounds error. (It still ins't binding correctly. Eventually I need to ties the two lists together - have a step row set the current step - and wanted to deal with one thing at a time in the "master" app. I have an idea on how to change to place of setting currentStep. I'll see if that helps eliminate the first issue. Thanks for noticing, and I agree, this doesn't mean I am binding the sliders correctly.Macdougall
Almost there. I now can bind to my model (with an index out of range error) or bind locally and not have an index out of range error. You set me on the right path moving the update of currentStep. I'll update my question and the sample project.Macdougall
I updated my answer, with a fix. Let me know if it works.Archducal
It works, now I need to understand some details. I'm new with working in a reactive environment, much less Compose but I do get timing. Looking at your code, are you using the conditional in Text to check the name (and my initializing as an empty string is required)? As for the slider, are you suggesting that the UI, being updated so fast, is actually showing the wrong parameters in the list but the conditional helps make it so fast that the human eye doesn't see it? :-) Finally, and with thanks, do you think this is something I could do better? Those conditionals seem a bit non-intuitive.Macdougall
You're welcome. I do think that your code should have work. The timing issue, I think it's a bug, and my solution, a workaround. But I cannot be sure if that behaviour is in fact a bug, or if it works as intended. The conditional in the Text is more of the same. It is to prevent the out of bounds, we are accessing the same element after all. You are right about the visual update. I do not know if it is so fast that we cannot see it, or if it is just some internal logic that never gets to be drawn. In any case, it's proven to be too fast to be a bother. A final suggestion... (cont'd).Archducal
You know better than anyone what's your goal, so this suggestion looks right to me, but maybe in the overall logic of your app it doesn't:. Have you consider making currentStep an Int, where its value points to the element in the steps array. Duplicating the Step works fine, but may complicate things down the road if your code gets more complex. Remember they are structs (value types), so they do occupy different spacesArchducal
Tough to say, and I appreciate your criticisms. I've been a coder since 1984 and learning reactive programming - which this issue really ends up being about - is something I'll learn more about this summer. My earlier question (and example) did, however incorrectly, use a pointer. How the eventual app uses the model is not yet complete, and the main thing (for now) is to use SwiftUI and target iPad 13. I'll mark some code as a possible bug to check future betas out. Right now I'm wanting things to work. I think you have a firm grasp of Combine/SwiftUI and your last comment helped (cont'd)Macdougall
Last comment, with thanks. In 2004 I was a "go to" because I could get things working. But one time they gave me something on a new platform for me - SAP (ugh) - that I got everything but one small piece to work. We were just implementing SAP and had consultants. The one I went to said "maybe we could try this" - a tool i never knew existed that showed me a technique that helped me to solve a few other issues down the road. You just did that - I never thought of adding conditionals like you did. Again, thanks!Macdougall
It seems we might be from the same generation. I started coding when I was 10 years old (1982). So much water went down the bridge. Adapt or die! Glad I could help. Cheers.Archducal

© 2022 - 2024 — McMap. All rights reserved.