Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 18:47:24 GMT, Kevin Rushforth wrote: > The fix and new test looks fine. I added one comment/question regarding the > existing tests that check a count, but I'm OK with either answer. The suggestion to test for positive values of the counters is a good idea. The lower this counter the better, but if the counter is 0, this means there has been no evaluation of updateItem. This should cause other tests to fail, but it helps if it would trigger a failure in this test as well. I added these asserts on the places where previously the expected value was not 0. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> After (re)setting the number of elements, make sure to do at least some >> estimation of the total size. >> Added a testcase for this scenario. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 18:55:29 GMT, Michael Strauß wrote: > Would it be a good idea to fix the implementation of the `position` property > as part of this PR (it has the same problem as `cellCount` before the fix)? > If not, a follow-up ticket should be filed. A follow-up issue, [JDK-8278064](https://bugs.openjdk.java.net/browse/JDK-8278064), was filed a couple days ago. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> After (re)setting the number of elements, make sure to do at least some >> estimation of the total size. >> Added a testcase for this scenario. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) Would it be a good idea to fix the implementation of the `position` property as part of this PR (it has the same problem as `cellCount` before the fix)? If not, a follow-up ticket should be filed. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> After (re)setting the number of elements, make sure to do at least some >> estimation of the total size. >> Added a testcase for this scenario. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) The fix and new test looks fine. I added one comment/question regarding the existing tests that check a count, but I'm OK with either answer. Approved. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
> After (re)setting the number of elements, make sure to do at least some > estimation of the total size. > Added a testcase for this scenario. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: process reviewer comment (simplify call) - Changes: - all: https://git.openjdk.java.net/jfx/pull/683/files - new: https://git.openjdk.java.net/jfx/pull/683/files/b3fe2756..1a25a6d5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=683&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=683&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/683.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/683/head:pull/683 PR: https://git.openjdk.java.net/jfx/pull/683
