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 [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 [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
