netopyr / reduxfx

Functional Reactive Programming (FRP) for JavaFX
Apache License 2.0
112 stars 11 forks source link

fix IndexOutOfBoundException in Patcher on RemovePatch #58

Closed manuel-mauky closed 7 years ago

manuel-mauky commented 7 years ago

I've found a possible IndexOutOfBoundException in Patcher.java.

Think of the following example: Before the an Action is applied a TreeView has 3 Children. After the Action, the TreeView is empty. The Differ calculates that 3 RemovePatches have to be applied to he VSceneGraph.

This is done in Differ.java (Line 108-110). However, the problem here is the iteration. The for loop runs from 0 to 2 and adds the RemovePatches in this order. Now we have:

RemovePatch(index=0,...) RemovePatch(index=1,...) RemovePatch(index=2,...)

In Patcher 148-155 these patches are applied. However, the third patch can't be applied because there is no element with index=2 left anymore. Instead an IndexOutOfBoundException is thrown.

To fix this the order of the patches has to be reversed. The patch with the highest index has to be applied first. To do this I've reversed the for-loop in Differ.java to count from 2 to 0 instead.

I've tried out this fix with a small app and it solved the problem without noticing any negative side effects. However, I don't know if this breaks some other use-cases that I haven't thought about.

netopyr commented 7 years ago

I think an even better solution would be that RemovePatch contains not only a single entry, but a range. We can easily calculate the range in the Differ and also in the Patcher we could remove all children at once.

netopyr commented 7 years ago

Oh, and nice catch btw. ;)

netopyr commented 7 years ago

Fixed with https://github.com/netopyr/reduxfx/commit/9ee06616f2e04bdf9e3409bf0e08cb9a63560259