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

Paginate scrolling in test result screen #511

Closed
wants to merge 17 commits into from
Closed

Conversation

aanorbel
Copy link
Member

@aanorbel aanorbel commented Apr 18, 2022

Fixes ooni/probe#1430

Proposed Changes

Sample Pager logs

V/FlowLog: Executing query: SELECT * FROM `Result` ORDER BY `start_time` DESC LIMIT 20 OFFSET 0 
V/FlowLog: Executing query: SELECT * FROM `Result` ORDER BY `start_time` DESC LIMIT 20 OFFSET 20 

@aanorbel aanorbel requested review from lorenzoPrimi, bassosimone and hellais and removed request for lorenzoPrimi April 18, 2022 08:29
.limit(20)
.offset(pageNumber * 20)
.orderBy(Result_Table.start_time, false)
.queryList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If while you have fetched the first page of data, another result is written to the results table (because there is a test currently running), fetching the next page will be off by one.

You could instead use the result_id as a pagination handler and query WHERE result_id < $last_result_id to get the next page worth of data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query mechanism has been updated to use result_id based pagination

@hellais hellais self-requested a review July 25, 2022 13:55

Integer prevKey;
try {
prevKey = (!response.isEmpty()) ? Objects.requireNonNull(Iterables.getFirst(response, null)).id - 20 : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you actually need the previous key? In which circumstances are you using this?

I don't think you can derive this by just subtracting 20, because some test results may have been removed and so the list of IDs may have "holes" in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the previous key to return it and allow navigation, IIUC the code.

I don't know how the code behaves when there are holes. It may be that, while you are scrolling back, the Android code will continue to query until it has enough elements to fill the screen, so it may be that we don't need to be more precise at the cost of executing more queries.

I think @aanorbel should clarify this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To optimize the load time and memory usage, the records loaded will not always stay in memory, so the previous key is used when the records previously loaded are no longet in memory.

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly comments related to how querying the database works

.github/workflows/archive.yml Outdated Show resolved Hide resolved
}
// This API defines that it's out of data when a page returns empty. When out of
// data, we return `null` to signify no more pages should be loaded
Integer nextKey = (!response.isEmpty()) ? Iterables.getLast(response).id : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Integer nextKey = (!response.isEmpty()) ? Iterables.getLast(response).id : null;
Integer nextKey = (!response.isEmpty()) ? Iterables.getLast(response).id + 1 : null;

Conceptually, the next page seems to start at index $last + 1 rather than at index $last. That said, I can see how smooth scrolling means that probably we want to keep the last element in scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operand used for the query is lessThan so using Iterables.getLast(response).id + 1 will mean we lose a record.


Integer prevKey;
try {
prevKey = (!response.isEmpty()) ? Objects.requireNonNull(Iterables.getFirst(response, null)).id - 20 : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the previous key to return it and allow navigation, IIUC the code.

I don't know how the code behaves when there are holes. It may be that, while you are scrolling back, the Android code will continue to query until it has enough elements to fill the screen, so it may be that we don't need to be more precise at the cost of executing more queries.

I think @aanorbel should clarify this.

@aanorbel aanorbel requested review from bassosimone and hellais April 24, 2023 07:55
@aanorbel
Copy link
Member Author

The scroll back will need to use the prevKey only when there are no records in memory are don't meet the scroll back parameter since previous records are not kept in memory after a certain threshold is reached.

@aanorbel
Copy link
Member Author

Stale

@aanorbel aanorbel closed this Jun 21, 2024
@aanorbel aanorbel deleted the issues/1430 branch October 21, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paginate scrolling in test result screen
3 participants