[inspector] Rework inspector index, reimplement path tracking and sibling*, fix navigation with metadata

[alexander-yakushev]

Problem

Path tracking is a big ball of hacks and assumptions. When the user goes down an item (= clicks on it in the UI), the path to the item is inferred from the positional idx of active items on the page. This assumption is broken (and there are bugs related to it) if there are other active elements on the page before the main collection items (e.g. rendered metadata values).

Sibling navigation relies on path tracking, so it is affected as well.

Sibling navigation uses inspector index (= rendered elements) to fetch prev/next element. This created problems if navigation goes beyond the current page, so down has been recently reworked to handle indices not rendered on the screen currently. This also introduced a bug that down works incorrectly if there are extra items rendered on the screen (again, metadata values #247, or arrays where the inspector renders a few extra clickable objects above the main contents #248). down is a presentation-level function and shouldn't be used for low-level model navigation.

Solution

Index (:index in the inspector) is modified to keep not only the values rendered on the screen but also their provenance (key if they come from the map, position if they come from a sequence). Path tracking now relies on that information instead of double-guessing the element key/position.

Path tracking now only tracks going to map values and to sequence items. Navigating to class or map keys (which produced (find :somekey) key) no longer results in a valid path. This feature (tracking path for such navigations) was a gimmick anyway, and not having it simplifies implementation somewhat.

Introduce low-leveldown* that allows to go to an arbitrary child of the current value (not necessarily one that is currently displayed on the screen given the pagination). Sibling functions now nth the neighbor element and use down* and don't care about pages.

down again works only with the presentation level and accepts only valid indices of items really currently rendered. If idx is incorrect, down is a no-op (like all presentation-level functions that perform illegal operations – up at the root value, prev-page on the first page, etc.). down now again works correctly with collections that contain metadata.

Ensure that :current-page is always correct even if user goes across page boundaries with sibling ops. Recompute the correct current page early, don't do ad-hoc fixes for it during rendering. Because of that, if the user goes down an element, then walks to a sibling that is on another page, and goes up, he ends up on the correct page for the child element he upped from.

Some duplicate work and ad-hoc fixes become unneeded and are now removed.

BONUS! Arrays now also support sibling navigation and path tracking.

This PR can be read commit-by-commit.

[bbatsov]

Path tracking now only tracks going to map values and to sequence items. Navigating to class or map keys (which produced (find :somekey) key) no longer works. They were a gimmick anyway, and not having them simplifies implementation somewhat.

That sounds good to me.

I skimmed over the code and I don't see anything concerning in the proposed implementation.

[alexander-yakushev]

Rebased on top of master.

[vemv]

Thanks! I agree on the problem statement - code was hard to work with and occasionally one would see bugs in navigation.

Navigating to class no longer works

I believe I use that all the time, if it's what I think it is. e.g. I'm seeing an instance of an exotic Java object -> I'll want to know more about its class.

Additional question - are map keys iterated in a deterministic order? I've always had the impression that the Inspector was a bit lax in this aspect - users would see map keys in a random-looking order which is both not particularly usable, and internally, potentially error-prone.

Might be a good chance to tackle that.

[alexander-yakushev]

I believe I use that all the time, if it's what I think it is. e.g. I'm seeing an instance of an exotic Java object -> I'll want to know more about its class.

Very poor wording from my side. I meant that when navigating to a class, the path is no longer rendered. Previously, it will say Path: :a :b (nth 3) class in the inspector. Of course, jumping to the class still works, it is an important feature.

Additional question - are map keys iterated in a deterministic order? I've always had the impression that the Inspector was a bit lax in this aspect - users would see map keys in a random-looking order which is both not particularly usable, and internally, potentially error-prone.

Do you mean iterating keys with sibling functions? This still does not work, and I'm not sure how to do it properly in the future. Only indexed collections that can do a deterministic nth supports prev/next sibling.

The current order of rendering key-values is implementation-dependent. We can try to sort the keys lexicographically, and that can be then iterated deterministally. Worth thinking about as a separate feature.

[vemv]

Of course, jumping to the class still works, it is an important feature.

👍

The current order of rendering key-values is implementation-dependent. We can try to sort the keys lexicographically, and that can be then iterated deterministally. Worth thinking about as a separate feature.

Yes, would be happy to leave that for later. I suggested the idea in case it had overlap with the current changes.

[bbatsov]

🚀