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

add paging support #5518

Merged
merged 8 commits into from
Nov 8, 2019
Merged

add paging support #5518

merged 8 commits into from
Nov 8, 2019

Conversation

JoHaHu
Copy link
Contributor

@JoHaHu JoHaHu commented Oct 26, 2019

Before i implement this, i want to discuss the interfaces i created.
#5507

@JoHaHu JoHaHu changed the title added paging interface add paging support Oct 26, 2019
@tobiasdiez
Copy link
Member

Cool that you provide a first draft of the interface before jumping ahead and changing everything!

The proposed interface looks good to me. The only suggestion I have is that the different Page<BibEntry> performSearchPagedX methods can be replaced by a single method List<BibEntry> performSearchPaged(String query, int page) throws FetcherException;. The fetcher api's that might support paging always except a start index and a number of items to fetch. These information can be calculated once you have the page number.
It's then the responsibility of the GUI to calculate the correct page number to get the entries for the previous/next page.

@JoHaHu JoHaHu marked this pull request as ready for review November 5, 2019 08:37
@tobiasdiez
Copy link
Member

This looks good. I'm not sure if the Page class is really needed, but this will probably seen when the UI is changed to incorporate the page support.

How do you want to continue? Add the UI support as part of this PR or as a next one?

@JoHaHu
Copy link
Contributor Author

JoHaHu commented Nov 6, 2019

I think I will open a separate one. And I will open a issue with a list of fetchers that have to implement the new interfaces.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

In general: LGTM. -- Some nitpick comments remain 😇

import org.jabref.model.entry.BibEntry;
import org.jabref.model.paging.Page;

public interface PagedSearchBasedFetcher extends SearchBasedFetcher {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some JavaDoc here about the idea?

I see that one can

a) jump to arbitrary pages
b) the page size supported is pre-defined

Would there also be a method setPageSize() make sense? Maybe a sub interface PagedSearchBasedFetcherWithVariablePageSize?

The alternative solution could have been iterate as jcabi github does it. However, with your approach, one can directly jump to a certain page and is not forced to query sequentially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to keep the fetcher as stateless as possible. If thats is not required, i can provide an interface for that purpose.
It probably depends where we want configure the page size. In the application settings or as a dropdown?

Copy link
Member

Choose a reason for hiding this comment

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

I would like to see it in a drop down. Internally, it can be persisted as preference, so that at a restart of JabRef, the value is the same.

I can imagine that with pagination we need to be stateful. If I as user go back and forth in the UI, I want to have quick responses. If there is a request sent to the server each time, this will be slow.

I am aware that this will require a "refresh search results" button in the UI, which just resets the factory to trigger new searches.

I would even use Google Guava Cache for caching different search results over different fetchers. -- The search results for publications won't differ from hour to hour, but merely from day to day or even week to week. Having the possibility to limit the cache by memory size. - I also find the loading pretty nice. see https://github.com/google/guava/wiki/CachesExplained#from-a-callable:

CacheLoader<Key, Graph> loader = CacheLoader.from(key -> createExpensiveGraph(key));

Source: https://guava.dev/releases/snapshot/api/docs/com/google/common/cache/CacheLoader.html

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I would let each fetcher decide the appropriate page size. Some fetchers might not support a variable page size, and even if they do there might be restrictions (for example some fetchers get a list of ids and then fetch details for every id; in this case you probably want to have only a small page size).

Copy link
Contributor Author

@JoHaHu JoHaHu Nov 8, 2019

Choose a reason for hiding this comment

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

I would like to see it in a drop down. Internally, it can be persisted as preference, so that at a restart of JabRef, the value is the same.

Then I think we dont need a setSize method, because how the getSize is implemented is up to the fetcher. The fetcher can then use the Preference API.

I can imagine that with pagination we need to be stateful. If I as user go back and forth in the UI, I want to have quick responses. If there is a request sent to the server each time, this will be slow.

I would argue that the fetcher doesn't need to be stateless but should be stateless. It's solely responsibility is to fetch results. The ViewModel can then cache the results. Caching is possible in the fetcher as well, but the Viewmodel is statefull anyway, therefore we don't add too much new complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would even use Google Guava Cache for caching different search results over different fetchers. -- The search results for publications won't differ from hour to hour, but merely from day to day or even week to week. Having the possibility to limit the cache by memory size. - I also find the loading pretty nice. see https://github.com/google/guava/wiki/CachesExplained#from-a-callable:

That's another reason to do it in the viewmodel

Copy link
Contributor Author

@JoHaHu JoHaHu Nov 8, 2019

Choose a reason for hiding this comment

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

Maybe I can implement a cache decorator for the fetchers. That way caching becomes very easy. But instanceof checks will fail

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking, that query optimization can be done best at the result provider. Thus, if I query 1000 elements at once as JabRef, following can happen:

  1. Provider supports 1000 entrie at once --> they are returned -(query optimization there)
  2. Provider supports 50 entries at once --> jabref does 20 calls to the provider (nearly no query optimization at the provider)

My thoughts with the factory were driven with the idea that the first setting should be supported.

That leads me to the idea that we might need maxPageSize and pageSize. Maybe even different page sizes for the view and for the fetcher...

We can surely separate the the basic fetcher and some layer inbetween.

UI --> caching layer --> fetcher

Copy link
Member

Choose a reason for hiding this comment

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

Caching is a nice idea, but not thaaaat important. I would say, first focus on the paging UI and then we can have a look at how to implement caching.

src/main/java/org/jabref/model/paging/Page.java Outdated Show resolved Hide resolved
@Test
public void performSearchByQueryPaged_twoPagesNotEqual() throws Exception {
Page<BibEntry> page = fetcher.performSearchPaged("author:\"A\"", 0);
Page<BibEntry> page2 = fetcher.performSearchPaged("author:\"A\"", 1);
Copy link
Member

Choose a reason for hiding this comment

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

I think, in the UI it will be hard to always pass the query. Why not adding an intermediate layer searchPagesFactory = fetcher.searchPagesFactory("author:...") and then searchPagesFactory.getPage(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stateless argument applies here too. I would prefer the viewmodel to handle state

@tobiasdiez
Copy link
Member

I've create a branch pagedFetcher for you (in the JabRef repo). Could you please target that branch. As soon as the UI is implemented we can then merge this branch in the master. In this way, you can add code incrementally by different PRs and the master branch contains only "complete" features.

@JoHaHu JoHaHu changed the base branch from master to pagedFetcher November 8, 2019 10:53
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I'll merge this now. I guess further changes are necessary but that will depend on the GUI implementation. For the moment this is good to go.

@tobiasdiez tobiasdiez merged commit a4e5ad4 into JabRef:pagedFetcher Nov 8, 2019
@koppor koppor mentioned this pull request Apr 2, 2020
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.

3 participants