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

signal_vec::Flatten handles some updates to its outer SignalVec incorrectly #86

Open
1Mango1Lime opened this issue Sep 15, 2024 · 0 comments

Comments

@1Mango1Lime
Copy link

Hi, I've been having some trouble with signal_vec::Flatten when its outer SignalVec updates. Specifically it looks like UpdateAt, RemoveAt, Move and Pop aren't generating all the right updates. The current tests only seem to look at Replace and Clear.

For example, I think this test (if you add it to tests\signal_vec.rs) describes the expected behaviour of RemoveAt correctly, but it currently fails:

#[test]
fn flatten_outer_remove_at() {
    let input = util::Source::new(vec![
        Poll::Ready(VecDiff::Replace {
            values: vec![
                util::Source::new(vec![
                    Poll::Ready(VecDiff::Replace { values: vec![ 0 ] }),
                ]),
                util::Source::new(vec![
                    Poll::Ready(VecDiff::Replace { values: vec![ 1 ] }),
                ]),
                util::Source::new(vec![
                    Poll::Ready(VecDiff::Replace { values: vec![ 2 ] }),
                ]),
                util::Source::new(vec![
                    Poll::Ready(VecDiff::Replace { values: vec![ 3 ] }),
                ]),
            ],
        }),
        Poll::Pending,
        // [ [ 0 ], [ 1 ], [ 2 ], [ 3 ] ]
        Poll::Ready(VecDiff::RemoveAt { index: 0 }),
        Poll::Pending,
        // [ [ 1 ], [ 2 ], [ 3 ] ]
        Poll::Ready(VecDiff::RemoveAt { index: 1 }),
        Poll::Pending,
        // [ [ 1 ], [ 3 ] ]
        Poll::Ready(VecDiff::RemoveAt { index: 1 }),
        // [ [ 1 ] ]
    ]);

    let output = input.flatten();

    util::assert_signal_vec_eq(output, vec![
        Poll::Ready(Some(VecDiff::Replace {
            values: vec![0, 1, 2, 3],
        })),
        Poll::Pending,
        // [ [ 0 ], [ 1 ], [ 2 ], [ 3 ] ]
        Poll::Ready(Some(VecDiff::RemoveAt { index: 0 })),
        // [ [ 1 ], [ 2 ], [ 3 ] ]
        Poll::Ready(Some(VecDiff::RemoveAt { index: 1 })),
        // [ [ 1 ], [ 3 ] ]
        Poll::Ready(Some(VecDiff::RemoveAt { index: 1 })),
        // [ [ 1 ] ]
        Poll::Ready(None),
    ]);
}

And this is a possible, but not particularly elegant, fix. A more elegant version would require more re-work (e.g., to FlattenState), which I think might be a bit beyond me.

    fn poll_vec_change(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Option<VecDiff<Self::Item>>> {
        let mut this = self.project();

        if let Some(diff) = this.pending.pop_front() {
            return Poll::Ready(Some(diff));
        }

        let mut pending: PendingBuilder<VecDiff<<A::Item as SignalVec>::Item>> = PendingBuilder::new();

        let top_done = loop {
            break match this.signal.as_mut().as_pin_mut().map(|signal| signal.poll_vec_change(cx)) {
                Some(Poll::Ready(Some(diff))) => {
                    match diff {
                        VecDiff::Replace { values } => {
                            *this.inner = values.into_iter().map(FlattenState::new).collect();

                            let values = this.inner.iter_mut()
                                .flat_map(|state| state.poll_values(cx))
                                .collect();

                            return Poll::Ready(Some(VecDiff::Replace { values }));
                        },
                        VecDiff::InsertAt { index, value } => {
                            this.inner.insert(index, FlattenState::new(value));
                        },
                        VecDiff::UpdateAt { index, value } => {
                            let removed_len = this.inner[index].len;
                            this.inner[index] = FlattenState::new(value);
                            let start_ind: usize = this.inner[..index].iter().map(|state| state.len).sum();
                            for index in (0..removed_len).rev() {
                                pending.push(VecDiff::RemoveAt { index: start_ind + index });
                            }
                        },
                        VecDiff::RemoveAt { index } => {
                            let removed_len = this.inner.remove(index).len;
                            let start_ind: usize = this.inner[..index].iter().map(|state| state.len).sum();
                            for index in (0..removed_len).rev() {
                                pending.push(VecDiff::RemoveAt { index: start_ind + index });
                            }
                        },
                        VecDiff::Move { old_index, new_index } => {
                            if old_index != new_index {
                                let old_start_ind: usize = this.inner[..old_index].iter().map(|state| state.len).sum();
                                let value = this.inner.remove(old_index);
                                let new_start_ind: usize = this.inner[..new_index].iter().map(|state| state.len).sum();
                                let moved_len = value.len;
                                this.inner.insert(new_index, value);

                                if new_index < old_index {
                                    (0..moved_len).for_each(|_| pending.push(VecDiff::Move {
                                        old_index: old_start_ind + moved_len - 1,
                                        new_index: new_start_ind,
                                    }));
                                } else {
                                    (0..moved_len).for_each(|_| pending.push(VecDiff::Move {
                                        old_index: old_start_ind,
                                        new_index: new_start_ind + moved_len - 1,
                                    }));
                                }
                            }
                        },
                        VecDiff::Push { value } => {
                            this.inner.push(FlattenState::new(value));
                        },
                        VecDiff::Pop {} => {
                            let removed_len = this.inner.pop().unwrap().len;
                            (0..removed_len).for_each(|_| pending.push(VecDiff::Pop { }));
                        },
                        VecDiff::Clear {} => {
                            this.inner.clear();
                            return Poll::Ready(Some(VecDiff::Clear {}));
                        },
                    }

                    continue;
                },
                Some(Poll::Ready(None)) => {
                    this.signal.set(None);
                    true
                },
                Some(Poll::Pending) => {
                    false
                },
                None => {
                    true
                },
            };
        };

        let mut inner_done = true;

        let mut prev_len = 0;

        for state in this.inner.iter_mut() {
            let done = state.poll_pending(cx, prev_len, &mut pending);

            if !done {
                inner_done = false;
            }

            prev_len += state.len;
        }

        if let Some(first) = pending.first {
            *this.pending = pending.rest;
            Poll::Ready(Some(first))

        } else if inner_done && top_done {
            Poll::Ready(None)

        } else {
            Poll::Pending
        }
    }

Sorry, I'm a total novice at using GitHub, so I thought I'd raise this as an issue first.
I've written a couple of other tests as well, if that's useful.
Thanks!

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

No branches or pull requests

1 participant