Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v6]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Try to keep visible cells at their original position when improving the estimate. This reduces annoying effects when re-layouting cells without relevant changes. This should not be done when we don't have a valid estimate yet/anymore, or when the absoluteOffset is set to infinity (which may happen if the position is set to infinity, which is the case in some calls in Skin classes). - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/aa08ba26..c7d722d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=04-05 Stats: 121 lines in 2 files changed: 75 ins; 29 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Sun, 8 May 2022 18:56:45 GMT, Johan Vos wrote: >> @johanvos >> As requested, we've made a unit test, which tests this bug. >> It's based on your test and our original test class. It can be added to the >> ListViewTest. >> You can find it, in the JDK ticket. >> >> Btw, adding the cells incrementally seems to make a difference - which is >> why the new test class tests both cases. >> >> It accepts various inputs of cell sizes - and should work with any inputs. >> It should catch all the cases from the original test class. >> Because it works with all possible inputs - one could even make a random >> test-cases generator to make sure it works in every case. >> >> It basically only works well, when the sizes are only 20s - in most other >> cases it fails. > > The later reported issues were due to the fact that the estimatedSize got > updated during the layoutChildren call. That makes things much more > complicated and leads to discrepancies. I disabled the updates to the > estimated size during the layoutChildren call, so that all operations that > are happening in that call are using a consistent value of the estimated > size. We now pass all the tests added by @FlorianKirmaier on the JBS issue . > Those tests are great regression tests. Hi @johanvos, I've updated the TestClass once more. You can find it on the ticket. There are 2 cases that seem to not work properly yet. 1.) If the elements are very big, the tests seem to fail - but it only happens if the scene is layouted again. 2.) On Click, the element gets selected and the VirtualFlow "jumps around". I've adapted the tests to simulate the click with the selectionModel. I've also added another assert, to check whether our cell is visible - which seems to be not always the case. Both cases were also reproducible with the original test application. Btw, can I somehow become an official reviewer for this PR? - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Sun, 8 May 2022 18:56:45 GMT, Johan Vos wrote: >> @johanvos >> As requested, we've made a unit test, which tests this bug. >> It's based on your test and our original test class. It can be added to the >> ListViewTest. >> You can find it, in the JDK ticket. >> >> Btw, adding the cells incrementally seems to make a difference - which is >> why the new test class tests both cases. >> >> It accepts various inputs of cell sizes - and should work with any inputs. >> It should catch all the cases from the original test class. >> Because it works with all possible inputs - one could even make a random >> test-cases generator to make sure it works in every case. >> >> It basically only works well, when the sizes are only 20s - in most other >> cases it fails. > > The later reported issues were due to the fact that the estimatedSize got > updated during the layoutChildren call. That makes things much more > complicated and leads to discrepancies. I disabled the updates to the > estimated size during the layoutChildren call, so that all operations that > are happening in that call are using a consistent value of the estimated > size. We now pass all the tests added by @FlorianKirmaier on the JBS issue . > Those tests are great regression tests. @johanvos Hi Johan, Great to see all these unit tests getting green so fast! Unfortunately, the provided Test Application still behaves very buggy - I would even say it still is equally buggy, which is quite a suprise. Nevertheless, this is a great step forward. So there must be something the tests are missing. I haven't found out yet what. Maybe they will behave different, if they are executed as a system test, with a real window? I will keep you posted, when we've updated the tests. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Mon, 25 Apr 2022 09:00:54 GMT, Florian Kirmaier wrote: >> I agree with that observation. The mathematical perfect situation would be >> to pre-calculate the height of all items, so that the scrolbar position can >> be exact, and the content placing can be exact as well. That would be at the >> price of a major performance overhead for large lists. For small lists, this >> overhead is more acceptable. >> >> I agree that this is something that could be rather application-defined >> instead of having heuristics in the ListView. >> >> As a historical note, before >> https://bugs.openjdk.java.net/browse/JDK-8089589 was fixed, all items were >> considered of equali height for the position of the scrollbar. In the >> current case, if that precondition holds, the estimated size will be the >> real size, so in that case there is no need to calculate the height of every >> item before getting the correct total size. >> This is again something the application knows best -- even with non-fixed >> cell heights, the expected variations might be small enough and if they are >> randomly spread, the estimate will soon be "good enough". > > @johanvos > As requested, we've made a unit test, which tests this bug. > It's based on your test and our original test class. It can be added to the > ListViewTest. > You can find it, in the JDK ticket. > > Btw, adding the cells incrementally seems to make a difference - which is why > the new test class tests both cases. > > It accepts various inputs of cell sizes - and should work with any inputs. > It should catch all the cases from the original test class. > Because it works with all possible inputs - one could even make a random > test-cases generator to make sure it works in every case. > > It basically only works well, when the sizes are only 20s - in most other > cases it fails. The later reported issues were due to the fact that the estimatedSize got updated during the layoutChildren call. That makes things much more complicated and leads to discrepancies. I disabled the updates to the estimated size during the layoutChildren call, so that all operations that are happening in that call are using a consistent value of the estimated size. We now pass all the tests added by @FlorianKirmaier on the JBS issue . Those tests are great regression tests. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v5]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Don't recalculate estimated size while doing a layout cycle. There are a number of paths possible that first calculate an offset/position, and later do a layoutChildren pass. The offset/position calculations (e.g. in scrollTo calls) assume a specific estimatedSize. If that size is changed between the first call and the layoutChildren logic, the result will not look as intended. Hence, we should avoid updating the estimated size at the start of the layout phase. We can do it safely at the end. Thanks to Zeiss and Florian Kirmaier for providing additional tests. - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/2b1b4bdc..aa08ba26 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=03-04 Stats: 116 lines in 2 files changed: 115 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote:
>> When the size of a ListCell is changed and a scrollTo method is invoked
>> without having a layout calculation in between, the old (wrong) size is used
>> to calculcate the total estimate. This happens e.g. when the size is changed
>> in the `updateItem` method.
>> This PR will immediately resize the cell and sets the new value in the cache
>> containing the cellsizes.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Don't shift cells if we are already showing the lowest index cell.
I noticed this bug as well, but in my case even when the `CellHeight` stays the
same (so I'm not sure if it's the same or something else)... A minimal repro
example:
public class ApplicationUI extends VBox {
private final ObservableList elements =
FXCollections.observableArrayList();
public ApplicationUI() {
Button button = new Button("Add New Element");
ListView listView = new ListView<>(elements);
setVgrow(listView, Priority.ALWAYS);
getChildren().addAll(listView, button);
button.setOnAction(event -> {
elements.add(String.valueOf(elements.size()));
});
elements.addListener((ListChangeListener) c -> {
while (c.next()) {
listView.scrollTo(c.getFrom());
}
});
}
}
Click the button until there is a scroll bar, it always seems to scroll down to
the second last element in the list, instead of the last.
Interestingly, changing the line from `listView.scrollTo(c.getFrom());` to
`listView.scrollTo(c.getFrom()-1);` or `listView.scrollTo(c.getFrom()+1);`
results in the same behavior, however changing it to
`listView.scrollTo(c.getFrom()-2);` does scroll correctly to the last item in
the list?
Just thought I would share this in case this would help find the issue.
-
PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Thu, 14 Apr 2022 08:52:27 GMT, Johan Vos wrote: >> Johan Vos has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't shift cells if we are already showing the lowest index cell. > > I agree with that observation. The mathematical perfect situation would be to > pre-calculate the height of all items, so that the scrolbar position can be > exact, and the content placing can be exact as well. That would be at the > price of a major performance overhead for large lists. For small lists, this > overhead is more acceptable. > > I agree that this is something that could be rather application-defined > instead of having heuristics in the ListView. > > As a historical note, before https://bugs.openjdk.java.net/browse/JDK-8089589 > was fixed, all items were considered of equali height for the position of the > scrollbar. In the current case, if that precondition holds, the estimated > size will be the real size, so in that case there is no need to calculate the > height of every item before getting the correct total size. > This is again something the application knows best -- even with non-fixed > cell heights, the expected variations might be small enough and if they are > randomly spread, the estimate will soon be "good enough". @johanvos As requested, we've made a unit test, which tests this bug. It's based on your test and our original test class. It can be added to the ListViewTest. You can find it, in the JDK ticket. Btw, adding the cells incrementally seems to make a difference - which is why the new test class tests both cases. It accepts various inputs of cell sizes - and should work with any inputs. It should catch all the cases from the original test class. Because it works with all possible inputs - one could even make a random test-cases generator to make sure it works in every case. It basically only works well, when the sizes are only 20s - in most other cases it fails. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Don't shift cells if we are already showing the lowest index cell. I agree with that observation. The mathematical perfect situation would be to pre-calculate the height of all items, so that the scrolbar position can be exact, and the content placing can be exact as well. That would be at the price of a major performance overhead for large lists. For small lists, this overhead is more acceptable. I agree that this is something that could be rather application-defined instead of having heuristics in the ListView. As a historical note, before https://bugs.openjdk.java.net/browse/JDK-8089589 was fixed, all items were considered of equali height for the position of the scrollbar. In the current case, if that precondition holds, the estimated size will be the real size, so in that case there is no need to calculate the height of every item before getting the correct total size. This is again something the application knows best -- even with non-fixed cell heights, the expected variations might be small enough and if they are randomly spread, the estimate will soon be "good enough". - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote:
>> When the size of a ListCell is changed and a scrollTo method is invoked
>> without having a layout calculation in between, the old (wrong) size is used
>> to calculcate the total estimate. This happens e.g. when the size is changed
>> in the `updateItem` method.
>> This PR will immediately resize the cell and sets the new value in the cache
>> containing the cellsizes.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Don't shift cells if we are already showing the lowest index cell.
I don't believe that it is possible to position the content and scrollbar
**exactly**, if the VirtualFlow **estimates** the size of the cells.
For that case we need an external cell size provider, that I, as an application
developer, can provide to the e.g. ListView.
I thought of something like this. The existing estimation logic can then be
implemented as a default cell size provider.
interface CellSizeProvider {
/**
* Returns the size of all cells.
* IMPORTANT: Be carefully by implementing this method. It can drastically
degrade the performance of a component.
*/
double getTotalSize();
/**
* Returns the size of the cell for an item given by its index.
* IMPORTANT: Be carefully by implementing this method. It can drastically
degrade the performance of a component.
*/
double getCellSize(int);
/**
* The callback which is set by the VirtualFlow. It allows to calculate the
cell size for an item given by its index.
* IMPORTANT: This callback returns the exact value, but it may be slow.
*/
void setCellSizeCalculator(Function cellSizeCalculator);
}
-
PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Don't shift cells if we are already showing the lowest index cell. After some investigating, it also doesn't properly scroll to the Bottom. I've added a minimal modified button to the TestApplication. https://user-images.githubusercontent.com/6547435/162963628-41daf63b-755d-449a-be85-ca0aa0fd9d15.png;> Is this conceptional doable, to get properly working scrollTo implementation for varying cell sizes? - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Don't shift cells if we are already showing the lowest index cell. I've added another testbutton to the testclass. When we jump to somewhere in the middle, the selected index is not at the top, but somewhere else. Depending on the application, this can be quite a bit. In the affected application - it sometimes still jumps to places closer to the wrong cell compared to the right cell. The cell #2 should be at the top, but isnt. https://user-images.githubusercontent.com/6547435/162953335-39d40eed-35a7-4728-912b-18a8acce5256.png;> - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v3]
On Fri, 25 Mar 2022 16:26:34 GMT, Kevin Rushforth wrote: > It looks like there are some failing unit tests now. I fixed them. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Don't shift cells if we are already showing the lowest index cell. - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/a931ba75..2b1b4bdc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v3]
On Thu, 24 Mar 2022 10:06:34 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Add test to check if there are no empty cells at the end of a List in case > there are enough leading cells available. It looks like there are some failing unit tests now. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v2]
On Tue, 22 Mar 2022 08:47:42 GMT, Johan Vos wrote: >> When the size of a ListCell is changed and a scrollTo method is invoked >> without having a layout calculation in between, the old (wrong) size is used >> to calculcate the total estimate. This happens e.g. when the size is changed >> in the `updateItem` method. >> This PR will immediately resize the cell and sets the new value in the cache >> containing the cellsizes. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Add the option to shift all cells down in case the current calculations > would lead to empty cells at the end of the view, > while there are available cells before the beginning of the view. There was a problem in ListView, in which cells are shifted down in case there are empty cells being rendered while there are leading cells available. I also added a testcase for this that fails before and succeeds after. - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v3]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Add test to check if there are no empty cells at the end of a List in case there are enough leading cells available. - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/05935373..a931ba75 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=01-02 Stats: 35 lines in 1 file changed: 35 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v2]
> When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Add the option to shift all cells down in case the current calculations would lead to empty cells at the end of the view, while there are available cells before the beginning of the view. - Changes: - all: https://git.openjdk.java.net/jfx/pull/712/files - new: https://git.openjdk.java.net/jfx/pull/712/files/d1ab8afb..05935373 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=712=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=712=00-01 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed
On Sun, 9 Jan 2022 15:20:36 GMT, Johan Vos wrote: > When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. I've added a new button to the test programm. The 5th button now literally jumps into the "Nothingness" https://user-images.githubusercontent.com/6547435/149306510-3b6348d2-6f3a-442c-bd01-80a57dbb0245.png;> . - PR: https://git.openjdk.java.net/jfx/pull/712
Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed
On Sun, 9 Jan 2022 15:20:36 GMT, Johan Vos wrote: > When the size of a ListCell is changed and a scrollTo method is invoked > without having a layout calculation in between, the old (wrong) size is used > to calculcate the total estimate. This happens e.g. when the size is changed > in the `updateItem` method. > This PR will immediately resize the cell and sets the new value in the cache > containing the cellsizes. I've just tested it with the TestProgram of the ticket, and it is now less wrong, but still not correct. There is a small empty are, of the size of about 5 pixels. https://user-images.githubusercontent.com/6547435/148739867-dd16ba38-0378-42ce-b6f6-9700e699471d.png;> This happens with the 1, 2, and 3rd button, but not with the 4th button. The order of the clicks doesn't seem to matter. - PR: https://git.openjdk.java.net/jfx/pull/712
RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed
When the size of a ListCell is changed and a scrollTo method is invoked without having a layout calculation in between, the old (wrong) size is used to calculcate the total estimate. This happens e.g. when the size is changed in the `updateItem` method. This PR will immediately resize the cell and sets the new value in the cache containing the cellsizes. - Commit messages: - resize cell when getting it from the pile, and recalculate total estimated size Changes: https://git.openjdk.java.net/jfx/pull/712/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=712=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277785 Stats: 63 lines in 2 files changed: 63 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/712.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/712/head:pull/712 PR: https://git.openjdk.java.net/jfx/pull/712
