On Fri, 22 Sep 2023 09:43:52 GMT, Marius Hanl <mh...@openjdk.org> wrote:
>> Fixes the regression by basically reverting one change introduced in >> https://bugs.openjdk.org/browse/JDK-8311983. >> The problem is that it is actually required to get the size from a cell with >> the index -1, which technically does not exist (the accumCell is used then). >> >> Furthermore, unlike the name suggests, the call to `addLeadingCells()` is >> always needed, even if there are none. >> This is because the method does much more than what you would think first. >> >> In future, it would probably be good to revisit this code. Also performance >> wise, since this looks like something that can be optimized. But that is >> another story, not related to this fix. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8316590: More tests Thanks, those tests look good (although the assert in the positiontest could be more relaxed, we are ok if the position is -0.01 but not when it is -20). I agree the first test is a good regression test too. One of the difficulties in this area is that the position of the cell is not always the value we ask it to be. e.g. if we want it to be at -0.01, its layout will be at 0 (rounded in the layout phase) and subsequently the value of getPosition will be 0 as well, which makes it hard to be consistent during calculations (asking what cell is at -0.01 will give a different result than asking what cell is at 0 for example). I agree that there are many optimizations possible, and some of them are very easy. It is hard though to prove that even the simplest optimization is not causing any regression at all. Therefore, I think there is a huge value in PR's like this because they add tests. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1246#issuecomment-1732534324
