Add accessors for the index of the first and last visible cell.

[davmac314]

Save the index of the first and last visible cell at layout time, and make them available via accessors. This allows much more efficient retrieval of this information.

[JordanMartinez]

Makes sense. Can you add some tests for this?

[davmac314]

@JordanMartinez ok, I've added a test and also updated some other parts of the code to use the cached values (in a separate commit). I can undo that last change, if you'd prefer, and put it in a separate pull request later - or is it ok here?

[JordanMartinez]

I think it should stay since this PR is more of an optimization PR which includes the last commit you've added.

I'm almost ready to merge. Howver, I have one question since I'm not as familiar with ReactFX's MemoizedList as I used to be. (It's been a while since I wrote the documentation for that in my PR for it).

In getCurrentPosition(), what does the following line translate to in plain English? if (cellListManager.getLazyCellList().getMemoizedCount() == 0)

Since the original implementation of this method handles the logic a bit differently, I want to insure that newer implementation is only faster, not changed.

[davmac314]

@JordanMartinez If I'm understanding correctly, cellListManager.getLazyCellList().getMemoizedCount() == 0 should mean "if there are no cells currently visible". (That should probably only be the case when the virtual flow is empty, there are no nodes to display). So, I see that as being equivalent to the previous positioner.getFirstVisibleIndex().isPresent(). I.e. the memoized cells are the visible cells, the first one should be the first visible cell and would only not be present if no cells were visible. I hope this makes sense.

[davmac314]

(To be honest it has taken me some time to understand the code myself. I am about 99% certain that what I've written above is correct, but if you know someone who understands the code better - it wouldn't hurt to check with them).

[JordanMartinez]

Since it's been a while since I've looked at this code, I typed comments into both implementations to process through the question myself.

Here's the original code with my comments

// goal: get the current position in the viewport, which is defined by
//  1) the index of the cell to use as the viewport's anchor cell and
//  2) the offset of an anchor cell from 
//   either the top of the viewport (StartOffStart object)
//   or the bottom of it (EndOffStart object)
private TargetPosition getCurrentPosition() {
    // method: 
    //    return the starting offset of the first visible cell (if it exists)
    //    otherwise, return "BEGINNING"

     OptionalInt firstVisible = positioner.getFirstVisibleIndex();
    // if the viewport is displaying 1 or more cells
     if(firstVisible.isPresent()) {
        // then use the first visible one as the anchor cell
         int idx = firstVisible.getAsInt();
         C cell = positioner.getVisibleCell(idx);
         return new StartOffStart(idx, orientation.minY(cell));
     } else {
     // otherwise (displaying no cells), return BEGINNING
         return TargetPosition.BEGINNING;
    }
}

Here is your implementation and my reading of it:

private TargetPosition getCurrentPosition() {
   // return "Beginning" if there are no visible cells
   // otherwise, use the first visible cell as the target position
    if (cellListManager.getLazyCellList().getMemoizedCount() == 0) {
         return TargetPosition.BEGINNING;
     } else {
         C cell = positioner.getVisibleCell(firstVisibleIndex);
         return new StartOffStart(firstVisibleIndex, orientation.minY(cell));
     }
}

So, with that being processed, yes, it seems your code does the exact same thing just through a slightly different way.

[JordanMartinez]

Thanks for your work