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

Allow drag-and-dropping multiple containers and views in the blueprint tree #8334

Merged
merged 22 commits into from
Dec 11, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Dec 5, 2024

Related

What

This PR makes it possible to drag multi-selection of views and containers within the blueprint tree. It lays the foundation of a system that will be extended to other drag payload and UI sections.

Specifically:

  • ctx.handle_select_hover_drag_interactions() (formerly ctx.select_hovered_on_click()) is now able to initiate drag interactions. This is opt-in for now, as dragging from most place isn't supported yet (to implemented in follow-up PRs).
  • Introduce a new DragAndDropPayload type to interoperate between various part of the UI. This type also enforce the grouping of items that can meaningfully be dragged together (e.g. it's ok to drag a view and a container together, because there exist somewhere they can be dropped to, but it's not ok to drag a view and an entity together).
  • When a drag is successfully initiated, a "pill" is displayed along the cursor which indicates the content of what's being dragged.
  • Introduces a very hack mechanism for a black list of undraggable items (aka the root container).
  • Update blueprint tree to support multiple selection and the new drag and drop payload type.
  • Updates egui to latest master.

@abey79 abey79 added enhancement New feature or request ui concerns graphical user interface do-not-merge Do not merge this PR include in changelog labels Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
245210a https://rerun.io/viewer/pr/8334 +nightly +main

Note: This comment is updated whenever you push a commit.

@abey79 abey79 force-pushed the antoine/dnd-multi-select-content branch from a07e4e5 to 219b298 Compare December 5, 2024 18:25
@nikolausWest
Copy link
Member

I haven't been able to actually drop any dragged entities yet. Is that intended to work here or is it just dragging?

@abey79
Copy link
Member Author

abey79 commented Dec 6, 2024

I haven't been able to actually drop any dragged entities yet. Is that intended to work here or is it just dragging?

Yeah this PR is just about views/container but lays the groundwork. Support for dragging entities will come next.

@nikolausWest
Copy link
Member

Ignore for now if this is just super premature design feedback but I don't like the experience of showing an error all the time. If we want to show an error on the drag-pill it feels like that should be reserved for situations where I clearly want to drop it somewhere but that doesn't work.

drag-and-drop.mp4

@nikolausWest
Copy link
Member

I expected the result of both these kinds of drags to have an effect:

no-ordering-effect.mp4
no-effect-2.mp4

@abey79
Copy link
Member Author

abey79 commented Dec 6, 2024

I expected the result of both these kinds of drags to have an effect:

Yeah it broke it apparently. I'll fix it before this gets undrafted.

Base automatically changed from antoine/color-table-and-token to main December 6, 2024 15:42
@abey79 abey79 force-pushed the antoine/dnd-multi-select-content branch from 002b15f to 1c09ca9 Compare December 6, 2024 15:53
@abey79 abey79 removed the do-not-merge Do not merge this PR label Dec 6, 2024
@abey79 abey79 marked this pull request as ready for review December 9, 2024 15:20
@teh-cmc teh-cmc requested review from emilk and Wumpf December 11, 2024 08:47
# Conflicts:
#	crates/viewer/re_view_bar_chart/src/view_class.rs
#	crates/viewer/re_view_graph/src/ui/draw.rs
#	crates/viewer/re_view_map/src/map_view.rs
#	crates/viewer/re_viewer/src/app_state.rs
#	crates/viewer/re_viewer_context/src/contents.rs
#	examples/rust/custom_view/src/color_coordinates_view.rs
@emilk emilk removed the request for review from Wumpf December 11, 2024 09:32
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Code mostly LGTM, but I don't know when to pass true or false to handle_select_hover_drag_interactions. I would like to either have clear instructions, or better yet: have it be automatic (e.g. based on the items passed to it)

crates/viewer/re_blueprint_tree/src/blueprint_tree.rs Outdated Show resolved Hide resolved
crates/viewer/re_ui/src/ui_ext.rs Show resolved Hide resolved
&response,
Item::DataResult(query.view_id, instance_path.clone()),
false,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding false and true as argument, did you consider having two different functions, for increased readability? In particular, it is very hard for me reviewing this code to know what false means.

I.e. you could have kept the old select_hovered_on_click with the previous meaning, and added handle_select_hover_drag_interactions (without a bool) for draggable things. But maybe its better to force each users to consider wether or not an item is draggable in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I want people to think of draggability and make a call when using this function.

It's also a toggle that allows me to progressively implement draggability one UI area at a time (e.g. next PR will switch some items to true in the streams tree).

There is an ideal version of the future where this is true everywhere (and thus the arg can be removed), but I'm not sure this is realistic. There are cases where stuff should hover/select highlight, but not be draggable. For example, entity buttons should probably not be draggable—or at least not always (e.g. from the add/remove entity modal).

In generally, we should make something draggable only if it makes sense, e.g if there is an obvious drop destination.

I'll improve the docstring to expand on the above. Let me know if you have any further suggestion on this.

crates/viewer/re_viewer_context/src/drag_and_drop.rs Outdated Show resolved Hide resolved
crates/viewer/re_viewer_context/src/drag_and_drop.rs Outdated Show resolved Hide resolved
crates/viewer/re_viewer_context/src/drag_and_drop.rs Outdated Show resolved Hide resolved
crates/viewer/re_viewer_context/src/viewer_context.rs Outdated Show resolved Hide resolved
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Code mostly LGTM, but I don't know when to pass true or false to handle_select_hover_drag_interactions. I would like to either have clear instructions, or better yet: have it be automatic (e.g. based on the items passed to it)

@emilk
Copy link
Member

emilk commented Dec 11, 2024

Sometimes when you drag things you get a pill, sometimes you don't

inconsistent-drag

@abey79
Copy link
Member Author

abey79 commented Dec 11, 2024

Sometimes when you drag things you get a pill, sometimes you don't

That's a design decision we made because the red pill was worse. The lack of pill is feedback that the current selection is not draggable.

As for this specific view: the next PR will enable dragging entities, so with it, every item in the blueprint tree will be individually draggable (or in homogenous multiple selection). This is not generally true however. For example, in the streams tree the entities will be draggable, but not the components.

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback!

Comment on lines 84 to 85
/// If a selection contains any `undraggable_items`, it may not be dragged.
pub undraggable_items: &'a ItemCollection,
Copy link
Member

Choose a reason for hiding this comment

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

You can at least document it here exactly what it is was introduced for, and maybe with a TODO for "this is ugly; if you have a better idea please rewrite this" :]

@abey79 abey79 merged commit 704438f into main Dec 11, 2024
31 checks passed
@abey79 abey79 deleted the antoine/dnd-multi-select-content branch December 11, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request include in changelog ui concerns graphical user interface
Projects
None yet
3 participants