TableView2: Check cell visibility before add/remove listener #1362
Conversation
…the continuous listener add/remove and garbage collection
|
Hi lord41ph4, Welcome to ControlsFX and thank you for taking time to contribute to this project. We do not recognise you as a contributor. Can you please sign ControlsFX Individual Contributor Agreement: https://cla.controlsfx.org ? |
|
Hi @lord41ph4 , Can you add a Test to verify this change? |
abhinayagarwal
changed the title
|
@abhinayagarwal |
…for selection under load better readable and more open to extensions
…r load to have same test outcome for windows and mac
|
Could you point out where the notable changes are? It seems that the diff is quite large because of a change in indenting. Also, is this issue reproduced in the SpreadsheetView? |
|
Hi @Maxoudela, |
| produceUpdateLoad(); | ||
|
|
||
| Duration timeTaken = measure(this::changeSelection); | ||
| assertThat(timeTaken, is(lessThan(MAX_DURATION_FOR_SELECTION_CHANGES))); |
There was a problem hiding this comment.
What is the logic behind the time taken by changeSelection always being less than 20s?
There was a problem hiding this comment.
Yes, I gave a maximum of 1 second for each selection change (defined in line 70).
| } | ||
|
|
||
| @Test | ||
| public void shouldHaveNoArtifacts_When_ScrollingVertically() { |
There was a problem hiding this comment.
This test passes even when the fix is not applied. Not sure if its supposed to.
There was a problem hiding this comment.
Yes, it is supposed to be that way. I played around with the code and I thought this test could at least guarantee that the scrolling is not broken. I think it does not harm to have such tests, as they make the life easier when modifying the code
| } | ||
|
|
||
| @Test | ||
| public void shouldNotFreeze_When_ScrollingVerticallyUnderLoad() { |
There was a problem hiding this comment.
This test passes even when the fix is not applied. Not sure if its supposed to.
There was a problem hiding this comment.
Yes, it is also supposed to be that way. I first thought I could make the GUI freeze with this test, because I could see that the cells have not been reused before (8339de5). But I guess to make it freeze I need to use heavier cells, like with buttons or something like that. But as it didn't really freeze and the test was anyway written I just left it there.
| } | ||
| } | ||
|
|
||
| if (isVisible) { |
There was a problem hiding this comment.
Why is this check removed?
There was a problem hiding this comment.
Because of line 213 (continue statement) the isVisible variable was always true at this point.
|
I'll modify the SpreadsheetView later on to remove the The problem should not appear in the SpreadsheetView since the |
3 participants
Fixes #1361
The non-responsiveness came from continuous listener adding while creating the cell that is not visible and be garbage collected immediately. Moving the getCell(..) call further down to be performed after the visibility check is avoiding unnessesary creation of elements.