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

Rewrite Paging system a bit #1944

Merged
merged 5 commits into from
Aug 14, 2024
Merged

Rewrite Paging system a bit #1944

merged 5 commits into from
Aug 14, 2024

Conversation

Masterjun3
Copy link
Collaborator

@Masterjun3 Masterjun3 commented Aug 12, 2024

Okay, this PR has quite a few things to talk about.

First of all, what is the purpose of this PR? It's about solving the duplicate properties problem we had with our previous paging code.

  • The problem:
    Pages like the Games List or the Subs List allow Custom Filtering like "Starts With Letter" that needs to apply before paging. The query is bound using a custom Request class. However, the paging system itself needs to know about those queries as well, otherwise it can't make the page buttons link to the proper url with the proper query parameters.
  • The previous solution:
    Make a custom PageOf implementation like SubmissionPageOf that has all the same properties of SubmissionRequest. The issue being that refactorings will miss this and break paging, along with the usual problems of unnecessarily duplicating code.
  • This new solution in this PR:
    Always include the Request in the PageOf itself. So instead of making a SubmissionPageOf<SubmissionEntry> we instead make a PageOf<SubmissionEntry, SubmissionRequest>. Inside the PageOf there is a .Request property that contains the original request data. The pager then uses this to make the pager buttons.

This PR implements this change.
But it does a bit more. Every PageOf now needs an additional type, so this would require a lot of additional code refactorings everywhere we use a page. But most of our pages don't even use Custom Filtering! They just use the default implementation of a request: the PagingModel.
So this PR not only changes the PageOf<T> into a PageOf<T, T2>, it also implements a "fallback" PageOf<T> which is just a PageOf<T, PagingModel>, so that most places don't need to deal with the additional type.

I also had the idea of renaming our PagingModel class into something like DefaultPagingRequest, since it's now an IRequest and every custom Request implementation inherits it. But I wanted a somewhat minimal solution first, and renaming can come after.

I have two things left that irk me about this PR:

  1. In the Paginator.cs, the PageOf method (not the class) that turns a query into a PageOf along with the SortedPageOf method are now duplicated. This too is for the purpose of giving a "fallback" implementation. I wasn't able to combine their codes.
  2. The Request object has a reference in two places on Custom Filtering pages. First it needs to be in a Search property with a [FromQuery] attribute so that binding works. And once the paging is queried and generated, it's now also in the PageOf object in the .Request property. This Search and PageOf.Request are the same objects. I tried to come up with a way to only have a single reference always, so that developers aren't confused whether to use Search or PageOf.Request. But I wasn't able to, because the [FromQuery] needs to be somewhere.

I separated this PR into commits to look at in an isolated way.

  • The first commit is the functional change. The actual meat of this PR.
  • The second commit are the results that we can now do. The commit is mostly deleted code. It allows removing things like SubmissionPageOf and SystemPageOf.
  • The rest are minor commits.

That's all.

@adelikat adelikat merged commit 66e8484 into main Aug 14, 2024
1 check passed
@adelikat adelikat deleted the try-improving-paging-system branch August 14, 2024 15:48
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.

2 participants