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

[inspector] Rework pagination and datafy rendering #253

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Apr 29, 2024

Problem

Pagination operations apply only to the primary object being inspected. You can't prev/next page large metadata or datafied representation. With datafied, you actually can, but only by luck – if the primary object is pageable, then the datafied representation will also page along. If metadata or datafied value are larger than page size, but the primary value is not, then you only get one page of meta and datafied and no way to jump to next page or full representation.

There are a few bugs around pagination where pressing Space on a non-paged object will raise an exception.

ARef inspection (atoms, for example) also reuses the paginated rendering but doesn't respond to prev/next page.

Solution

Rework pagination. Pagination data (whether object is pageable, whether it should be paged, chunk to be displayed, last-page) is computed once and early, rendering commands reuse this information. Pagination commands only apply if the inspected value is a collection or array.

Rework datafy. This is a bit tricky because the problem is tricky. Two different cases here:

  1. (datafy inspected-value) returns something different from the value (so, there is a meaningful datafy implementation for it). Regardless of what is returned, we treat it as detached from the structure of inspected-value. So, it doesn't follow pagination rules of the inspected value. If it is small enough (< page-size), then render it completely, otherwise render it as a single clickable value that the user can navigate to and have a full-featured inspector for it.

  2. The inspected value is a map or some sequence. We know that we will paginate it, and currently display only one page (chunk). If none of the items displayed on the page datafy into something meaningful, then don't display datafy section at all. Otherwise, render datafied section. As user pages through the collection, the datafied section follows along.

If meta section is < page-size, then render it as a colelction in full, otherwise render it as a single value.

If atom contents is a collection < page-size, then render it as a map in full, otherwise render it as a single value. Don't render datafy section for atoms (we basically do the same manually in the Deref section, but with one less click for the user).

@vemv
Copy link
Member

vemv commented Apr 29, 2024

Looks very much reasonable to me.

I'm not sure I follow though as for the pagination logic for all cases.

Would you be able to share screenshots for the logic you describe?

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Apr 29, 2024

Meta doesn't follow current page. Small meta is rendered in full: (with-meta (repeat :data) (zipmap (range 5) (range)))

image

Large meta is rendered as value: (with-meta (repeat :data) (zipmap (range 20) (range)))

image

If datafy returns something on the value, it doesn't follow paging. Small is rendered in full: (with-meta (repeat :data) {'clojure.core.protocols/datafy (fn [_] (repeat 5 :datafy))})

image

Large is abbreved: (with-meta (repeat :data) {'clojure.core.protocols/datafy (fn [_] (repeat 20 :datafy))})

image

If not the whole collection is datafiable, but some of the collection elements, track the current page and show datafy section if there are any datafied element on current page. (assoc (vec (repeat 20 :data)) 15 (with-meta (reify Cloneable) {'clojure.core.protocols/datafy (fn [_] :datafy)})). No datafy on first page:

image

Datafy on second page:

image

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Apr 29, 2024

To me, the whole dance with datafying and then datafying the elements seem a bit like an overkill – I'm not sure what real-world usecases this intends to cover, but I made the rework maximally faithful to the current master behavior.

@vemv
Copy link
Member

vemv commented Apr 29, 2024

Thanks much - I could follow.

I like the proposed behavior and it seems pretty optimal to me.

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Apr 29, 2024

Maybe, the whole "datafy elements of the collection" should go away, and we'll instead optionally datafy the value representation (in orchard.print). So, basically, showing datafied elements inline in the primary collection, not in a separate section. But this needs more thought, while the current PR retains the current behavior meanwhile.

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

A possible issue is that we call nav far more often than before?

If nav is defined as "perform an http call", then that could fire back for users.

@alexander-yakushev
Copy link
Member Author

I don't think it should be called more often. If anything, it would be called twice fewer times because before we performed nav on all keys twice – in render-datafy? and render-datafy-content. Do I miss something?

@vemv
Copy link
Member

vemv commented Apr 29, 2024

You're right, as of master we also call nav on each collection sub-item.

(as an aside: I've always sensed that the Inspector is non-standard here - too eager. Being lazier would enable datafy/nav -based workflows for arbitrary user intents)

@alexander-yakushev
Copy link
Member Author

Well, it will be lazier now in the sense that only one page of data will be datafy/naved.

Otherwise, again, I'm not much of a datafy user, so I have no idea what the usecases of aggressive "datafication" are. Haven't seen many third-party datafy implementors in the wild.

@vemv
Copy link
Member

vemv commented Apr 29, 2024

Well, it will be lazier now in the sense that only one page of data will be datafy/naved.

👍

Otherwise, again, I'm not much of a datafy user, so I have no idea what the usecases of aggressive "datafication" are. Haven't seen many third-party datafy implementors in the wild.

An existing example is https://github.com/seancorfield/next-jdbc/blob/f03b1ba3168168d1cad2580a42fcf4b604310c49/doc/datafy-nav-and-schema.md - the navigable objects close over the connection

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2024 via email

@alexander-yakushev alexander-yakushev merged commit bf28a20 into master Apr 29, 2024
55 checks passed
@alexander-yakushev alexander-yakushev deleted the pagination-datafy branch April 30, 2024 05:42
@alexander-yakushev alexander-yakushev added the enhancement Improvement to an existing feature label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants