Coder Social home page Coder Social logo

Comments (15)

kamarton avatar kamarton commented on July 18, 2024 1

No, closeable.

from data.

samdark avatar samdark commented on July 18, 2024

Currently, paginator classes have no common part, so they cannot be handled uniformly.

That is intentional since widgets for offset pagination and keyset pagination are meant to be different.

With offset pagination it's a "classic" 1, 2, 3, 4, 5 ... 324. We are specifying page number and can jump to any page right away. With keyset pagination it's usually previous / next. We need to know first or last item ID in order to go to next or to previous page. Not only links are formed differently, there's slight difference in UI as well.

from data.

kamarton avatar kamarton commented on July 18, 2024

That is intentional since widgets for offset pagination and keyset pagination are meant to be different.

This is very wrong ideology.

The minimum condition for a paginator is to be able to the next page. This is already stated in DataReaderInterface, since the iterator can only move forward. As with reading almost all technical tools, e.g. cassette tape, database scanner, file readers.

public function read(): iterable;

With offset pagination it's a "classic" 1, 2, 3, 4, 5 ... 324. We are specifying page number and can jump to any page right away. With keyset pagination it's usually previous / next. We need to know first or last item ID in order to go to next or to previous page. Not only links are formed differently, there's slight difference in UI as well.

Error in this ideology through real examples.

Example 1.: ListView + auto scroll down pagination view.

I use a third-party API like OffsetPaginator. It makes perfect sense to use the OffsetPaginator class as there may be an end page where I use "normal"' paging as well. But I can't use it, because the auto scroll down view does support only KeysetPaginator. This is a meaningless restriction because the next and previous pages have the same meaning in both cases, and the current paginator classes know this.

nextPage(KeysetPaginator) == nextPage(OffsetPaginator) The result is a list of the following elements, regardless of the input parameter requirement.

Example 2.: ListView + "traditional" pagination view

I want to scroll through a large dataset, e.g. database but displaying a "traditional" pagination view configured as follows:

Can't I just use the much more optimal KeysetPaginator because it is wired to the link pager view widget at the OffsetPaginator framework level?

In summary:

No matter what kind of paging, relative paging should be supported by all pagination, even if limited.

Suggest

interface Paginator {
  // It supports both relative paging and absolute paging, covering all paginator implementations.
  function toParamters(int $pageNumber, bool $asRelative): ?array;   
  // This is always useful, as you must be able to process your own data.
  function withParameters(aray $data);
}

Normally, using relative paging is recommended as it is more widely supported. PHP_INT_MIN and PHP_INT_MAX can be used for the first and last pages, and zero for the current page.

A dropdown selector view pager, on the other hand, only supports pagers like OffsetPaginator, because I can specify the current page number from the drop-down list, which requires absolute paging.

from data.

samdark avatar samdark commented on July 18, 2024

No matter what kind of paging, relative paging should be supported by all pagination, even if limited.

OK, that's a good goal. Makes sense.

The interface suggested won't work though because for keyset paginator we have no idea on how to get parameters from page number. We can, of course, get all pages one by one starting from the first page but that'd defeat the purpose of such paginator. I'd suggest the following interfaces:

interface Paginator
{
    public function read(): iterable;
    public function getNextPageParameter(): ?string;
    public function getPreviousPageParameter(): ?string;
    public function withNextPageParameter(string $parameter): self;
    public function withPreviousPageParameter(string $parameter): self;
}

What do you think?

from data.

kamarton avatar kamarton commented on July 18, 2024

This interface is limited.

The interface suggested won't work though because for keyset paginator we have no idea on how to get parameters from page number. We can, of course, get all pages one by one starting from the first page but that'd defeat the purpose of such paginator. I'd suggest the following interfaces:

I admit there are shortcomings in my idea, but I still want to make the pagantor interface data completely transparent.

paginator.withParameters(paginator.toParameters(...)) == paginator

This statement should always be true regardless of the complexity of the data used by the paginator. The paginator must handle its parameters by itself. The other layers should not initiate any specific application.

I'm still thinking because the only problem with the current one is that you can't get the absolute page number. But it could be defined in the Offsetable interface.

from data.

samdark avatar samdark commented on July 18, 2024

This interface is limited.

That's the goal to make it both not too vague allowing anything and flexible at the same time.

The paginator must handle its parameters by itself. The other layers should not initiate any specific application.

How would GridView know which parameters are to pass to paginator to get to next page then? In my interface when generating next/previous links it would use getNextPageParameter() and getPreviousPageParameter(). We could extend that to be parameters instead of parameter and use associative arrays but I see no use for it.

from data.

kamarton avatar kamarton commented on July 18, 2024

The importance of array management is that the paginator can pass on its own data keys and parse them.

$paginator = $paginator->withParameters($_GET);
$data = $paginator->read();
$previousUrl = 'prefix...'.http_build_query($paginator->toParameters(-1, true));
// previous url with 'firstValue' => ...
$nextUrl = 'prefix...'.http_build_query($paginator->toParameters(1, true));
// next url with 'lastValue' => ...

// in next request
$paginator = $paginator->withParameters($_GET);
$data = $paginator->read();

In addition, it is possible to eg. to transform the keys, obviously must be reset beforehand.

This approach better covers all the options, but I recognize that some refinement is needed to get it out in absolute page numbers. I think this method is much better at covering the use between different paginators and less at risk of the accumulation of interfaces and methods.

from data.

samdark avatar samdark commented on July 18, 2024

I see. Questions:

  1. On the third page, would you still pass -1 and 1 as $page? If so, that's not page number but a special value. In my case I have two separate methods for that and it's way cleaner about intent. How about making two separate methods?
  2. Do you have a real case for more than a single parameter?

from data.

kamarton avatar kamarton commented on July 18, 2024

Do you have a real case for more than a single parameter?

RandomKeysetPaginator extends KeysetPaginator like Google's I'am Feeling Lucky search button. Although this is a sort order, it is technically provide by DateReader (eg DataReader->withRandomSeed(...)). In this case e.g. it works from a separate dataset, where the items are already randomized. However, this feature is limited by top of the paginator layers, and GridView can't even do it.

However, another purpose of the array I find useful is the black box. I don't need to know what parameters it produces and why, just work with it. I think this is such a closed part of the layer that there is no point in moving the meaning of the parameters to the higher layers.

On the third page, would you still pass -1 and 1 as $page? If so, that's not page number but a special value. In my case I have two separate methods for that and it's way cleaner about intent. How about making two separate methods?

This is a relative number of pages, but the methods also support absolute page numbers.
Basically I like the interface.

Would that be something like that?

$paginator = $paginator->withNextPageParameter($_GET[lastValue])->withPreviousParameter($_GET[firstValue]);

$nextUrl = ... lastValue=$paginator->getNextPageParameter()
$previousUrl = ... fistValue=$paginator->getPreviousPageParameter()

I started the planning with the level of use: layer view with N page link.

  • previous page, current, next page ([-1,0,1])
  • first page, previous page, current, next page, last page ([PHP_INT_MIN, -1, 0, 1, PHP_INT_MAX])
  • first page, N-2, N-1, current, N+1, N+2, last page ([PHP_INT_MIN, -2, -1, 0, 1, 2, PHP_INT_MAX])

I feel like I accept your interface proposal because it is really better as an interface. I suggest replacing the parameter to token.

interface PaginatorInterface
{
    public function read(): iterable;
    public function getNextPageToken(): ?string;
    public function getPreviousPageToken(): ?string;
    public function withNextPageToken(string $token): self;
    public function withPreviousPageToken(string $token): self;
    // or more useful (?string)
    // $paginator = $paginator->withNextPageParameter($_GET[lastValue] ?? null)->withPreviousParameter($_GET[firstValue] ?? null);
    public function withNextPageToken(?string $token): self;
    public function withPreviousPageToken(?string $token): self;
}

And another interface (I'm not convinced that all of them are based on the OffsetPaginator class.):

// This isolation for OffsetPaginator style.
interface PaginatorWithTotalPageNumberInterface {
    public function getTotalPageNumber(): int
    public function getCurrentPageNumber(): int
    public function withDirectPageNumber(int): self;
   // or more useful (?int)
    public function withDirectPageNumber(?int): self;
}

UPD: OffsetPaginator does not need an interface yet and will be worth planning on later.

from data.

kamarton avatar kamarton commented on July 18, 2024
interface PaginatorInterface {
   public function withPageSize(int): self;
   ....
}

from data.

kamarton avatar kamarton commented on July 18, 2024

What do you think?

interface PaginatorInterface
{
    public function read(): iterable;
    public function getIsLastPage(): bool;
    public function getIsFirstPage(): bool;
    public function getNextPageToken(): ?string;
    public function getPreviousPageToken(): ?string;
    public function withNextPageToken(?string $token): self;
    public function withPreviousPageToken(?string $token): self;
    public function withPageSize(int): self;
}

from data.

samdark avatar samdark commented on July 18, 2024

I'm feeling lucky

Am I correct that "I'm feeling lucky" thing is directing you to the first result from the whole result set? Then I don't get why there are any parameters or pagination needed at all... That is a custom data reader with a single read() method.

Interfaces

  • Glad we came to agreement that having separate methods is better than using magic number with multiple meanings as parameter.
  • Replacing parameter with a token sounds fine.
  • Planning OffsetPaginator interface later (if needed) is OK.
  • Interface above is fine. Let's introduce it 👍

from data.

kamarton avatar kamarton commented on July 18, 2024

That is a custom data reader with a single read() method.

If we were to implement these two methods from DataReaderInterface?

public function withPageSize(int): self;
public function read(): iterable;
interface PaginatorInterface extends DataReaderInterface {
...
}

from data.

samdark avatar samdark commented on July 18, 2024

I don't think PaginatorInterface should extend DataReaderInterface. There's no need to manipulate limit directly in paginators. The interface above, as I said, looks fine.

As for I'm feeling lucky, implementation should be quite straightforward:

final class IAmFeelingLuckyDataReader implements DataReaderInterface
{
    private $dataReader;

    public function __construct(DataReaderInterface $dataReader)
    {
        $this->dataReader = $dataReader->withLimit(1);
    }
    public function withLimit(int $limit)
    {
        throw new NotSupportedException('I am feeling lucky has limit = 1 all the time.');
    }

    public function read(): iterable
    {
        return parent::read();
    }
}

from data.

samdark avatar samdark commented on July 18, 2024

@kamarton anything left in the issue that is not done yet?

from data.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.