Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Window.positionAt should always return exact position of element #3070

Closed
christophstrobl opened this issue Mar 25, 2024 · 1 comment
Closed
Assignees
Labels
type: enhancement A general enhancement

Comments

@christophstrobl
Copy link
Member

christophstrobl commented Mar 25, 2024

The current implementation of the positionAt method of Window conceptually differs for keyset and offset-based windows.
With keyset, the returned ScrollPosition contains the exact coordinates used to identify an element.
The offset-based variant behaves a little differently in the sense of being 1-based (adding +1 to the actual offset of the element, pointing not to the element itself but to the next one in the list):

               keyset                  |                offset  
---------------------------------------+---------------------------------------
[                                      | [
  0: { id: 1, name: luke }             |    0: { id: 1, name: luke }
  1: { id: 2, name: han }              |    1: { id: 2, name: han }
]                                      | ]
---------------------------------------+---------------------------------------
positionAt(0) - { id: 1, name: luke }  | positionAt(0) - { offset: 1 }
positionAt(1) - { id: 2, name: han }   | positionAt(1) - { offset: 2 }

The various store implementations currently follow the 1-based offsets. This results in a mixed approach when loading entries from the database by including the element at a given ScrollPosition if ScrollPosition.isInitial() and excluding the position otherwise. There's no way to differentiate between ScrollPosition.offset() and ScrollPosition.offset(0).

Going forward, we need to tell implementing stores if the result should contain the element at the ScrollPosition or not.
Current implementations are using greater than & less than filters for keyset scrolling which means if the element at a given position needs to be part of the result set, an additional entry identifying the element needs to be added to the or concatenated filter.

@rstoyanchev
Copy link

rstoyanchev commented Mar 27, 2024

One small observation. I believe the way to find out if there are previous results is to check if isInitial() on the position at index 0 is true. Given that offsets start at 1 the following never works:

Window<String> window = Window.from(List.of("a", "b", "c"), OffsetScrollPosition.positionFunction(0));
ScrollPosition pos = window.positionAt(0);
boolean hasPrevious = !pos.isInitial();

assertThat(hasPrevious).isFalse();

@mp911de mp911de linked a pull request Apr 10, 2024 that will close this issue
@mp911de mp911de added the type: enhancement A general enhancement label Apr 10, 2024
@mp911de mp911de changed the title Window.positionAt should always return exact position of element. Window.positionAt should always return exact position of element Apr 10, 2024
@mp911de mp911de added this to the 3.3 RC1 (2024.0.0) milestone Apr 10, 2024
mp911de added a commit that referenced this issue Apr 10, 2024
Introduce method to obtain a position function from OffsetScrollPosition. Tweak documentation wording.

See #3070
Original pull request: #3072
natedanner pushed a commit to natedanner/spring-projects__spring-data-commons that referenced this issue May 20, 2024
natedanner pushed a commit to natedanner/spring-projects__spring-data-commons that referenced this issue May 20, 2024
Introduce method to obtain a position function from OffsetScrollPosition. Tweak documentation wording.

See spring-projects#3070
Original pull request: spring-projects#3072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants