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

More context menu 2: add support to show/hide DataResults #5397

Merged
merged 3 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 76 additions & 2 deletions crates/re_viewport/src/context_menu/actions.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use re_entity_db::InstancePath;
use re_log_types::{EntityPath, EntityPathFilter};
use re_space_view::{DataQueryBlueprint, SpaceViewBlueprint};
use re_viewer_context::{ContainerId, Item, SpaceViewClassIdentifier, SpaceViewId};
Expand All @@ -18,6 +19,9 @@ impl ContextMenuAction for ShowAction {
.is_contents_visible(&Contents::Container(*container_id))
&& ctx.viewport_blueprint.root_container != Some(*container_id)
}
Item::DataResult(space_view_id, instance_path) => {
data_result_visible(ctx, space_view_id, instance_path).is_some_and(|vis| !vis)
}
_ => false,
})
}
Expand Down Expand Up @@ -45,9 +49,16 @@ impl ContextMenuAction for ShowAction {
true,
);
}
}

// ---
fn process_data_result(
&self,
ctx: &ContextMenuContext<'_>,
space_view_id: &SpaceViewId,
instance_path: &InstancePath,
) {
set_data_result_visible(ctx, space_view_id, instance_path, true);
}
}

pub(super) struct HideAction;

Expand All @@ -62,6 +73,9 @@ impl ContextMenuAction for HideAction {
.is_contents_visible(&Contents::Container(*container_id))
&& ctx.viewport_blueprint.root_container != Some(*container_id)
}
Item::DataResult(space_view_id, instance_path) => {
data_result_visible(ctx, space_view_id, instance_path).unwrap_or(false)
}
_ => false,
})
}
Expand Down Expand Up @@ -89,6 +103,66 @@ impl ContextMenuAction for HideAction {
false,
);
}

fn process_data_result(
&self,
ctx: &ContextMenuContext<'_>,
space_view_id: &SpaceViewId,
instance_path: &InstancePath,
) {
set_data_result_visible(ctx, space_view_id, instance_path, false);
}
}

fn data_result_visible(
ctx: &ContextMenuContext<'_>,
space_view_id: &SpaceViewId,
instance_path: &InstancePath,
) -> Option<bool> {
if instance_path.is_splat() {
if let Some(space_view) = ctx.viewport_blueprint.space_view(space_view_id) {
let query_result = ctx
.viewer_context
.lookup_query_result(space_view.query_id());
query_result
.tree
.lookup_result_by_path(&instance_path.entity_path)
.map(|data_result| {
data_result
.recursive_properties()
.map_or(true, |prop| prop.visible)
})
} else {
None
}
} else {
None
}
abey79 marked this conversation as resolved.
Show resolved Hide resolved
}

fn set_data_result_visible(
ctx: &ContextMenuContext<'_>,
space_view_id: &SpaceViewId,
instance_path: &InstancePath,
visible: bool,
) {
if let Some(space_view) = ctx.viewport_blueprint.space_view(space_view_id) {
let query_result = ctx
.viewer_context
.lookup_query_result(space_view.query_id());
if let Some(data_result) = query_result
.tree
.lookup_result_by_path(&instance_path.entity_path)
{
let mut recursive_properties = data_result
.recursive_properties()
.cloned()
.unwrap_or_default();
recursive_properties.visible = visible;

data_result.save_recursive_override(ctx.viewer_context, Some(recursive_properties));
}
}
}

// ---
Expand Down
16 changes: 13 additions & 3 deletions crates/re_viewport/src/viewport_blueprint_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ impl Viewport<'_, '_> {
default_open,
|_, ui| {
// Always show the origin hierarchy first.
Self::space_view_entity_hierarchy_ui(
self.space_view_entity_hierarchy_ui(
ctx,
ui,
query_result,
Expand Down Expand Up @@ -323,7 +323,7 @@ impl Viewport<'_, '_> {
ui.label(egui::RichText::new("Projections:").italics());

for projection in projections {
Self::space_view_entity_hierarchy_ui(
self.space_view_entity_hierarchy_ui(
ctx,
ui,
query_result,
Expand Down Expand Up @@ -365,7 +365,9 @@ impl Viewport<'_, '_> {
);
}

#[allow(clippy::too_many_arguments)]
fn space_view_entity_hierarchy_ui(
&self,
ctx: &ViewerContext<'_>,
ui: &mut egui::Ui,
query_result: &DataQueryResult,
Expand Down Expand Up @@ -491,7 +493,7 @@ impl Viewport<'_, '_> {
continue;
};

Self::space_view_entity_hierarchy_ui(
self.space_view_entity_hierarchy_ui(
ctx,
ui,
query_result,
Expand All @@ -517,6 +519,14 @@ impl Viewport<'_, '_> {
let query = ctx.current_query();
re_data_ui::item_ui::entity_hover_card_ui(ui, ctx, &query, store, entity_path);
});

context_menu_ui_for_item(
ctx,
self.blueprint,
&item,
&response,
SelectionUpdateBehavior::UseSelection,
);
ctx.select_hovered_on_click(&response, item);
}

Expand Down
20 changes: 12 additions & 8 deletions tests/python/release_checklist/check_context_menu.py
Copy link
Member

Choose a reason for hiding this comment

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

This check does way too much imo; it needs to be broken down into smaller, atomic checks.

At the very least each of these sub-sections should be its own check, though I'm sure it can be made even more fine-grained than that.

I should be able to go through a check with my brain turned off. I shouldn't have to keep track of my progress or some mental state, and if the check fails I shouldn't have to explain where and how, because it should be small and focused enough that it doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I'll do that as a follow-up PR 👍🏻

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

* Reset the blueprint.
* Right-click on any space view and check for context menu content:
- Hide
- Hide (or Show, depending on visibility)
- Remove
- Clone
- Move to new container
Expand All @@ -26,19 +26,17 @@
- Add Space View
* Add a container via the context menu, check the container appears at then end of the list.
* Right-click on the container and check for context menu content:
- Hide
- Hide (or Show, depending on visibility)
- Remove
- Add Container
- Add Space View
- Move to new container
* Right-click on a data result and check for the context menu content:
- Hide (or Show, depending on visibility)
* Select a container and, in the Selection Panel, right click on either space view or container children:
- The context menu action should be the same as before.
- The selection state is not affected by the right-click.

### Show/Hide

* Hide a space view and check that its context menu shows "Show" instead of "Hide".
* Select multiple space views with homogeneous or heterogeneous visibility states. Check that either or both of "Show All" and "Hide All" are showed as appropriate.

### Selection behavior

Expand All @@ -52,12 +50,18 @@
### Multi-selection checks

* Select multiple space views, right-click, and check for context menu content:
- Hide
- Hide All (if any are visible)
- Show All (if any are hidden)
- Remove
- Move to new container
* Same as above, but with only containers selected.
* Same as above, but with both space views and containers selected.
* Same as above, but with the viewport selected as well. The context menu should be identical, but none of its actions should apply to the viewport.
* Same as above, but with the viewport selected as well, and check for context menu content. These should not affect the Viewport.
- Hide All (if any are visible)
- Show All (if any are hidden)
* Select a mix of containers, space views, and data results, and check for context menu content:
- Hide All (if any are visible)
- Show All (if any are hidden)

### Invalid sub-container kind

Expand Down