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

Present the root container as the top-level viewport container and differentiate it from other container #4909

Closed
abey79 opened this issue Jan 25, 2024 · 4 comments · Fixed by #4989
Assignees
Labels
ui concerns graphical user interface
Milestone

Comments

@abey79
Copy link
Member

abey79 commented Jan 25, 2024

The Blueprint Tree is currently presented with the root container at the top of the hierarchy:

image

This causes two issues:

  • The root container being, by definition, always alone at its level, it causes significant right-ward indentation for not much user benefit.
  • Since nothing can be inserted after or before the root container (again, by definition), it behaves somewhat unexpected with the drag and drop introduced in Add support for drag-and-drop in blueprint tree #4910.

In order to acknowledge the fundamental difference between the root container and all other container, and adjust the presentation to highlight that different and address the current shortcoming, we propose the following changes:

  1. The root container no longer has the the collapsing triangle and thus cannot be collapsed. This allows left-shifting the entire tree.
  2. The root container is named differently than other containers, for example "Viewport (Grid)"/"Viewport (Horizontal)"/etc.
  3. The root container is no longer draggable (though it is now draggable, it's impossible to find a suitable target drop anyways).

Appart from these changes, the root container would remain selectable/editable as it currently is (maybe with minor adjustment in the selection panel to acknowledge its difference).

Mock-up:

image

@abey79 abey79 added the ui concerns graphical user interface label Jan 25, 2024
@nikolausWest
Copy link
Member

Let's go with this approach. The alternative, which would be more similar to html or SwiftUI is to have an implicit vertical container at the root. However, I still think it's nice to have an explicit root to put global overrides on for instance so lets go with this

@abey79
Copy link
Member Author

abey79 commented Jan 26, 2024

Yes. If you want something else than a vertical container as viewport root, then you can set it and dont have to "manually" create a "sub-root" to achieve that effect.

@nikolausWest
Copy link
Member

Hmm... we actually need to think through how this interacts with the SDK.

  • Do we have a special Viewport container that you can use?
  • What happens if I set up a hierarchy without a Viewport, do we wrap it in a viewport or convert the outermost container to a viewport?
    • If we convert it, that means I can never put that container in another container after the fact. If I've set overrides and options on that container that's kind of annoying.
    • On the other hand if we just document that the root container will represent the viewport and can't later be moved that might just be fine

@abey79
Copy link
Member Author

abey79 commented Jan 26, 2024

Do we have a special Viewport container that you can use?

I don't think so. I'm not clear on the API details, but I'm anticipating that wherever we setup the viewport content, we can specify either one container (which will become the root), or one space view (which would then be the only viewport content, and will be added to an implicitly create root container).

that means I can never put that container in another container after the fact

By definition of a root container (whether or not we give it a special treatment), we cannot move it anywhere, because a container may not be moved inside of itself.

You may move item around (before/after) the root container, but performing that operation means implicitly creating a new root container and adding both the newly moved item and the former root container in it. That's what we want to avoid here.

On the other hand if we just document that the root container will represent the viewport and can't later be moved that might just be fine

That's exactly the spirit of this issue.

@abey79 abey79 self-assigned this Jan 30, 2024
@nikolausWest nikolausWest added this to the 0.13 milestone Jan 30, 2024
abey79 added a commit that referenced this issue Jan 30, 2024
### What

This PR adds support for drag-and-drop in the blueprint tree.

- Fixes #2652


https://github.com/rerun-io/rerun/assets/49431240/4d5f47ae-6d79-4ad0-a05d-b7eaaac18659


It has the following known limitations/future work:
- Only a single item may be dragged at a time. If multiple items are
selected when a drag starts, the selection is set to contain only the
dragged item.
  - #4887
- Only space view and containers may be dragged. The drag and drop
operations only operate on the view hierarchy (not the space view data).
- The root container has a special behaviour: it cannot be successfully
dragged, and nothing can be dragged before/after it. This is consistent
with it being the root container, but inconsistent with how the UI is
presented.
  - #4909
- Because of how Grid container work (they support "holes", and tend to
swap instead of reflow items when reordering), some drag operation have
additional order side effects within the destination (grid) container.
- #4916

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4910/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4910/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4910/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4910)
- [Docs
preview](https://rerun.io/preview/b4084fe32cd0466fe6acd4807a33545091c9e1ba/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/b4084fe32cd0466fe6acd4807a33545091c9e1ba/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Emil Ernerfeldt <[email protected]>
abey79 added a commit that referenced this issue Feb 1, 2024
…int tree (#4989)

### What

This PR changes how the root container is displayed and handled:
- It's labeled as "Viewport", since it defines the top-level
organisation of the viewport.
- It's not collapsible, so it doesn't have a triangle. This reduces the
right-ward drift.
- It (still) cannot be dragged (but now the difference with other
containers is more obvious). It now behaves better when dragged over
though.
- It cannot be removed.

Other than that, it still behaves mostly as other containers (in
particular, it can be selected and edited).

Some changes were needed in the drag-and-drop code, making the
"if-statement-of-death" incrementally more complicated, sadly.

* Fixes #4909

<img width="221" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/dc97a103-463d-4190-ba7c-03f6ddb502a4">


### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4989/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4989/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4989/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4989)
- [Docs
preview](https://rerun.io/preview/f0e2e0f5a8b95c99a75fa09038187289ff3a3173/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/f0e2e0f5a8b95c99a75fa09038187289ff3a3173/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui concerns graphical user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants